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...
