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

[Yunsheng Lin]

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);
> 
> [...]