Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
> IMHO this function uses a bit too much labels and would be more easy to
> read, e.g. moving the above chunk of code in conditional branch.
Maybe. I was trying to put the fast path up at the top without the slow path
bits in it, but I can put the "insufficient_space" bit there.
> Even without such change, I think the above 'goto try_again;'
> introduces an unneeded conditional, as at this point we know 'fragsz <=
> fsize'.
Good point.
> > + cache->pfmemalloc = folio_is_pfmemalloc(spare);
> > + if (cache->folio)
> > + goto reload;
>
> I think there is some problem with the above.
>
> If cache->folio is != NULL, and cache->folio was not pfmemalloc-ed
> while the spare one is, it looks like the wrong policy will be used.
> And should be even worse if folio was pfmemalloc-ed while spare is not.
>
> I think moving 'cache->pfmemalloc' initialization...
>
> > + }
> > +
>
> ... here should fix the above.
Yeah. We might have raced with someone else or been moved to another cpu and
there might now be a folio we can allocate from.
> > + /* Reset page count bias and offset to start of new frag */
> > + cache->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
> > + offset = folio_size(folio);
> > + goto try_again;
>
> What if fragsz > PAGE_SIZE, we are consistently unable to allocate an
> high order page, but order-0, pfmemalloc-ed page allocation is
> successful? It looks like this could become an unbounded loop?
It shouldn't. It should go:
try_again:
if (fragsz > offset)
goto insufficient_space;
insufficient_space:
/* See if we can refurbish the current folio. */
...
fsize = folio_size(folio);
if (unlikely(fragsz > fsize))
goto frag_too_big;
frag_too_big:
...
return NULL;
Though for safety's sake, it would make sense to put in a size check in the
case we fail to allocate a larger-order folio.
> > do {
> > struct page *page = pages[i++];
> > size_t part = min_t(size_t, PAGE_SIZE - off, len);
> > -
> > - ret = -EIO;
> > - if (WARN_ON_ONCE(!sendpage_ok(page)))
> > + bool put = false;
> > +
> > + if (PageSlab(page)) {
>
> I'm a bit concerned from the above. If I read correctly, tcp 0-copy
Well, splice()-to-tcp will; MSG_ZEROCOPY is unaffected.
> will go through that for every page, even if the expected use-case is
> always !PageSlub(page). compound_head() could be costly if the head
> page is not hot on cache and I'm not sure if that could be the case for
> tcp 0-copy. The bottom line is that I fear a possible regression here.
I can put the PageSlab() check inside the sendpage_ok() so the page flag is
only checked once. But PageSlab() doesn't check the headpage, only the page
it is given. sendpage_ok() is more the problem as it also calls
page_count(). I could drop the check.
David
