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

[Jakub Kicinski <kuba@kernel.org>]

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

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?