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