On 2021/5/6 20:58, Ilias Apalodimas wrote: >>>> >>> >>> Not really, the opposite is happening here. If the pp_recycle bit is set we >>> will always call page_pool_return_skb_page(). If the page signature matches >>> the 'magic' set by page pool we will always call xdp_return_skb_frame() will >>> end up calling __page_pool_put_page(). If the refcnt is 1 we'll try >>> to recycle the page. If it's not we'll release it from page_pool (releasing >>> some internal references we keep) unmap the buffer and decrement the refcnt. >> >> Yes, I understood the above is what the page pool do now. >> >> But the question is who is still holding an extral reference to the page when >> kfree_skb()? Perhaps a cloned and pskb_expand_head()'ed skb is holding an extral >> reference to the same page? So why not just do a page_ref_dec() if the orginal skb >> is freed first, and call __page_pool_put_page() when the cloned skb is freed later? >> So that we can always reuse the recyclable page from a recyclable skb. This may >> make the page_pool_destroy() process delays longer than before, I am supposed the >> page_pool_destroy() delaying for cloned skb case does not really matters here. >> >> If the above works, I think the samiliar handling can be added to RX zerocopy if >> the RX zerocopy also hold extral references to the recyclable page from a recyclable >> skb too? >> > > Right, this sounds doable, but I'll have to go back code it and see if it > really makes sense. However I'd still prefer the support to go in as-is > (including the struct xdp_mem_info in struct page, instead of a page_pool > pointer). > > There's a couple of reasons for that. If we keep the struct xdp_mem_info we > can in the future recycle different kind of buffers using __xdp_return(). > And this is a non intrusive change if we choose to store the page pool address > directly in the future. It just affects the internal contract between the > page_pool code and struct page. So it won't affect any drivers that already > use the feature. This patchset has embeded a signature field in "struct page", and xdp_mem_info is stored in page_private(), which seems not considering the case for associating the page pool with "struct page" directly yet? Is the page pool also stored in page_private() and a different signature is used to indicate that? I am not saying we have to do it in this patchset, but we have to consider it while we are adding new signature field to "struct page", right? > Regarding the page_ref_dec(), which as I said sounds doable, I'd prefer > playing it safe for now and getting rid of the buffers that somehow ended up > holding an extra reference. Once this gets approved we can go back and try to > save the extra space. I hope I am not wrong but the changes required to > support a few extra refcounts should not change the current patches much. > > Thanks for taking the time on this! Thanks all invovled in the effort improving page pool too:) > /Ilias > >>> >>> [1] https://lore.kernel.org/netdev/154413868810.21735.572808840657728172.stgit@firesoul/ >>> >>> Cheers >>> /Ilias >>> >>> . >>> >> > > . >
