Re: [PATCH net-next v7 4/4] skbuff: Optimization of SKB coalescing for page pool

[Mina Almasry <almasrymina@google.com>]

On Mon, Dec 11, 2023 at 11:32 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sun, 10 Dec 2023 20:21:21 -0800 Mina Almasry wrote:
> > Is it possible/desirable to add a comment to skb_frag_ref() that it
> > should not be used with skb->pp_recycle? At least I was tripped by
> > this, but maybe it's considered obvious somehow.
> >
> > But I feel like this maybe needs to be fixed. Why does the page_pool
> > need a separate page->pp_ref_count? Why not use page->_refcount like
> > the rest of the code? Is there a history here behind this decision
> > that you can point me to? It seems to me that
> > incrementing/decrementing page->pp_ref_count may be equivalent to
> > doing the same on page->_refcount.
>
> Does reading the contents of the comment I proposed here:
> https://lore.kernel.org/all/20231208173816.2f32ad0f@kernel.org/
> elucidate it? The pp_ref_count means the holder is aware that
> they can't release the reference by calling put_page().
> Because (a) we may need to clean up the pp state, unmap DMA etc.
> and (b) one day it may not even be a real page (your work).
>

Thank you, that makes sense.

> TBH I'm partial to the rename from patch 1, so I wouldn't delay this
> work any more :) But you have a point that we should inspect the code
> and consider making the semantics of skb_frag_ref() stronger all by
> itself, without the need to add a new flavor of the helper..
> Are you okay with leaving that as a follow up or do you reckon it's
> easy enough we should push for it now?

I think the rename from pp_frag_count -> pp_ref_count is a huge
improvement, and I think the fact that the netstack has a way to
obtain a reference on a pp frag is also a huge improvement. Please go
ahead mearging this if you like, I was asking questions for my own
education/follow up work to consider.

-- 
Thanks,
Mina