Message-ID: <fade4bc7-c1c7-517e-a775-0a5bb2e66be6@huawei.com>
Date:   Mon, 17 May 2021 16:25:43 +0800
From:   Yunsheng Lin <linyunsheng@...wei.com>
To:     Ilias Apalodimas <ilias.apalodimas@...aro.org>
CC:     Matteo Croce <mcroce@...ux.microsoft.com>,
        <netdev@...r.kernel.org>, <linux-mm@...ck.org>,
        Ayush Sawal <ayush.sawal@...lsio.com>,
        "Vinay Kumar Yadav" <vinay.yadav@...lsio.com>,
        Rohit Maheshwari <rohitm@...lsio.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Marcin Wojtas <mw@...ihalf.com>,
        Russell King <linux@...linux.org.uk>,
        Mirko Lindner <mlindner@...vell.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        "Tariq Toukan" <tariqt@...dia.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        "Alexei Starovoitov" <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "John Fastabend" <john.fastabend@...il.com>,
        Boris Pismenny <borisp@...dia.com>,
        Arnd Bergmann <arnd@...db.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Vlastimil Babka <vbabka@...e.cz>, Yu Zhao <yuzhao@...gle.com>,
        Will Deacon <will@...nel.org>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Roman Gushchin <guro@...com>, Hugh Dickins <hughd@...gle.com>,
        Peter Xu <peterx@...hat.com>, Jason Gunthorpe <jgg@...pe.ca>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Alexander Lobakin <alobakin@...me>,
        Cong Wang <cong.wang@...edance.com>, wenxu <wenxu@...oud.cn>,
        Kevin Hao <haokexin@...il.com>,
        Jakub Sitnicki <jakub@...udflare.com>,
        Marco Elver <elver@...gle.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Miaohe Lin <linmiaohe@...wei.com>,
        Guillaume Nault <gnault@...hat.com>,
        <linux-kernel@...r.kernel.org>, <linux-rdma@...r.kernel.org>,
        <bpf@...r.kernel.org>, Matthew Wilcox <willy@...radead.org>,
        Eric Dumazet <edumazet@...gle.com>,
        David Ahern <dsahern@...il.com>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        Saeed Mahameed <saeedm@...dia.com>,
        Andrew Lunn <andrew@...n.ch>, Paolo Abeni <pabeni@...hat.com>,
        Sven Auhagen <sven.auhagen@...eatech.de>
Subject: Re: [PATCH net-next v5 3/5] page_pool: Allow drivers to hint on SKB
 recycling

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

Powered by blists - more mailing lists