On 2021/5/17 14:38, Ilias Apalodimas wrote:

[...]

quoted

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

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.

As my understanding:
1. If the driver is using page allocated from page allocator directly to
   form a skb, let's say the page is owned by skb(or not owned by anyone:)),
   when a skb is freed, the put_page() should be called.

2. If the driver is using page allocated from page pool to form a skb, let's
   say the page is owned by page pool, when a skb is freed, page_pool_put_page()
   should be called.

What page_pool_release_page() mainly do is to make page in case 2 return back
to case 1.

And page_pool_release_page() is replaced with skb_mark_for_recycle() in patch
4/5 to avoid the above "case 2" -> "case 1" changing, so that the page is still
owned by page pool, right?

So the point is that skb_mark_for_recycle() does not really do anything about
the owner of the page, it is still owned by page pool, so it makes more sense
to keep the page pool ptr instead of setting it every time when
skb_mark_for_recycle() is called?

quoted

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?

As above, does it not make more sense to call page_pool_release_page() if the
driver does not need the SKB recycling?

Even if when skb->pp_recycle is 1, pages allocated from page allocator directly
or page pool are both supported, so it seems page->signature need to be reliable
to indicate a page is indeed owned by a page pool, which means the skb->pp_recycle
is used mainly to short cut the code path for skb->pp_recycle is 0 case, so that
the page->signature does not need checking?

Thanks
/Ilias