Messages in this thread | | | Date | Tue, 14 Jun 2022 18:12:22 +0100 | From | Al Viro <> | Subject | Re: [RFC][PATCH] fix short copy handling in copy_mc_pipe_to_iter() |
| |
On Tue, Jun 14, 2022 at 01:53:50AM +0100, Al Viro wrote:
> FWIW, I've got quite a bit of cleanups in the local tree; reordering and > cleaning that queue up at the moment, will post tonight or tomorrow. > > I've looked into doing allocations page-by-page (instead of single > push_pipe(), followed by copying into those). Doable, but it ends > up being much messier.
Hmm... Maybe not - a possible interface would be append_pipe(iter, size, &off)
that would either do kmap_local_page() on the last buffer (if it's anonymous and has space in it) or allocated and mapped a page and added a new buffer. Returning the mapped address and offset from it. Then these loops would looks like this:
while (left) { p = append_pipe(iter, left, &off); if (!p) break; chunk = min(left, PAGE_SIZE - off); rem = copy(p + off, whatever, chunk); chunk -= rem; kunmap_local(p);
copied += chunk; left -= chunk;
if (unlikely(rem)) { pipe_revert(i, rem); break; } } return copied;
with no push_pipe() used at all. For operations that can't fail, the things are simplified in an obvious way (rem is always 0).
Or we could have append_pipe() return a struct page * and leave kmap_local_page() to the caller...
struct page *append_pipe(struct iov_iter *i, size_t size, unsigned *off) { struct pipe_inode_info *pipe = i->pipe; unsigned offset = i->iov_offset; struct page_buffer *buf; struct page *page;
if (offset && offset < PAGE_SIZE) { // some space in the last buffer; can we add to it? buf = pipe_buf(pipe, pipe->head - 1); if (allocated(buf)) { size = min(size, PAGE_SIZE - offset); buf->len += size; i->iov_offset += size; i->count -= size; *off = offset; return buf->page; // or kmap_local_page(...) } } // OK, we need a new buffer size = min(size, PAGE_SIZE); if (pipe_full(.....)) return NULL; page = alloc_page(GFP_USER); if (!page) return NULL; // got it... buf = pipe_buf(pipe, pipe->head++); *buf = (struct pipe_buffer){.ops = &default_pipe_buf_ops, .page = page, .len = size }; i->head = pipe->head - 1; i->iov_offset = size; i->count -= size; *off = 0; return page; // or kmap_local_page(...) }
(matter of fact, the last part could use another helper in my tree - there the tail would be // OK, we need a new buffer size = min(size, PAGE_SIZE); page = push_anon(pipe, size); if (!page) return NULL; i->head = pipe->head - 1; i->iov_offset = size; i->count -= size; *off = 0; return page; )
Would that be readable enough from your POV? That way push_pipe() loses almost all callers and after the "make iov_iter_get_pages() advancing" part of the series it simply goes away...
It's obviously too intrusive for backports, though - there I'd very much prefer the variant I posted.
Comments?
PS: re local helpers:
static inline struct pipe_buffer *pipe_buf(const struct pipe_inode_info *pipe, unsigned int slot) { return &pipe->bufs[slot & (pipe->ring_size - 1)]; }
pretty much all places where we cache pipe->ring_size - 1 had been absolutely pointless; there are several exceptions, but back in 2019 "pipe: Use head and tail pointers for the ring, not cursor and length" went overboard with microoptimizations...
| |