On 2021/5/14 15:36, Ilias Apalodimas wrote:

[...]

quoted

quoted

+		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?

quoted

quoted

+	page_pool_put_full_page(pp, virt_to_head_page(data), false);
+
 	C(end);

[...]