Re: [PATCH net-next v5 3/5] page_pool: Allow drivers to hint on SKB recycling

[Yunsheng Lin]

On 2021/5/17 14:38, Ilias Apalodimas wrote:
> [...]
>>
>>>
>>>> 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.
> 
>>
>>> 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?
>>
>>> 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.
>>
>>>
>>>>
>>>> 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?

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