On 2021/5/14 15:36, Ilias Apalodimas wrote: > [...] >>> + return false; >>> + >>> + pp = (struct page_pool *)page->pp; >>> + >>> + /* Driver set this to memory recycling info. Reset it on recycle. >>> + * This will *not* work for NIC using a split-page memory model. >>> + * The page will be returned to the pool here regardless of the >>> + * 'flipped' fragment being in use or not. >>> + */ >>> + page->pp = NULL; >> >> Why not only clear the page->pp when the page can not be recycled >> by the page pool? so that we do not need to set and clear it every >> time the page is recycled。 >> > > If the page cannot be recycled, page->pp will not probably be set to begin > with. Since we don't embed the feature in page_pool and we require the > driver to explicitly enable it, as part of the 'skb flow', I'd rather keep > it as is. When we set/clear the page->pp, the page is probably already in > cache, so I doubt this will have any measurable impact. The point is that we already have the skb->pp_recycle to let driver to explicitly enable recycling, as part of the 'skb flow, if the page pool keep the page->pp while it owns the page, then the driver may only need to call one skb_mark_for_recycle() for a skb, instead of call skb_mark_for_recycle() for each page frag of a skb. Maybe we can add a parameter in "struct page_pool_params" to let driver to decide if the page pool ptr is stored in page->pp while the page pool owns the page? Another thing accured to me is that if the driver use page from the page pool to form a skb, and it does not call skb_mark_for_recycle(), then there will be resource leaking, right? if yes, it seems the skb_mark_for_recycle() call does not seems to add any value? > >>> + page_pool_put_full_page(pp, virt_to_head_page(data), false); >>> + >>> C(end); > > [...]
