On Mon, Dec 11, 2023 at 11:32 AM Jakub Kicinski <kuba@xxxxxxxxxx> 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@xxxxxxxxxx/ > 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
