[...]

quoted

quoted

quoted

quoted

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.

The driver is meant to call skb_mark_for_recycle for the skb and
page_pool_store_mem_info() for the fragments (in order to store page->pp).
Nothing bad will happen if you call skb_mark_for_recycle on a frag though,
but in any case you need to store the page_pool pointer of each frag to
struct page.

Right. Nothing bad will happen when we keep the page_pool pointer in
page->pp while page pool owns the page too, even if the skb->pp_recycle
is not set, right?

Yep, nothing bad will happen. Both functions using this (__skb_frag_unref and
skb_free_head) always check the skb bit as well.

quoted

quoted

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?

Then you'd have to check the page pool config before saving the meta-data,

I am not sure what the "saving the meta-data" meant?

I was referring to struct page_pool* and the signature we store in struct
page.

quoted

and you would have to make the skb path aware of that as well (I assume you
mean replace pp_recycle with this?).

I meant we could set the in page->pp when the page is allocated from
alloc_pages() in __page_pool_alloc_pages_slow() unconditionally or
according to a newly add filed in pool->p, and only clear it in
page_pool_release_page(), between which the page is owned by page pool,
right?

quoted

If not and you just want to add an extra flag on page_pool_params and be able 
to enable recycling depending on that flag, we just add a patch afterwards.
I am not sure we need an extra if for each packet though.

In that case, the skb_mark_for_recycle() could only set the skb->pp_recycle,
but not the pool->p.

quoted

quoted

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?

Not really, the driver has 2 choices:
- call page_pool_release_page() once it receives the payload. That will
  clean up dma mappings (if page pool is responsible for them) and free the
  buffer

The is only needed before SKB recycling is supported or the driver does not
want the SKB recycling support explicitly, right?

This is needed in general even before recycling.  It's used to unmap the
buffer, so once you free the SKB you don't leave any stale DMA mappings.  So
that's what all the drivers that use page_pool call today.

quoted

- call skb_mark_for_recycle(). Which will end up recycling the buffer.

If the driver need to add extra flag to enable recycling based on skb
instead of page pool, then adding skb_mark_for_recycle() makes sense to
me too, otherwise it seems adding a field in pool->p to recycling based
on skb makes more sense?

The recycling is essentially an SKB feature though isn't it?  You achieve the
SKB recycling with the help of page_pool API, not the other way around.  So I
think this should remain on the SKB and maybe in the future find ways to turn
in on/off?

Thanks
/Ilias

quoted

If you call none of those, you'd leak a page, but that's a driver bug.
patches [4/5, 5/5] do that for two marvell drivers.
I really want to make drivers opt-in in the feature instead of always
enabling it.

Thanks
/Ilias

quoted

quoted

quoted

quoted

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

[...]

.