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

From: Yunsheng Lin
Date: Fri May 14 2021 - 04:31:58 EST


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