| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <67d60543-2f3c-b0ff-b7fb-e44518cf325b@redhat.com>
Date: Tue, 10 Jan 2023 11:04:32 +0100
From: Jesper Dangaard Brouer <jbrouer@...hat.com>
To: Matthew Wilcox <willy@...radead.org>,
Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc: brouer@...hat.com, Jesper Dangaard Brouer <hawk@...nel.org>,
Ilias Apalodimas <ilias.apalodimas@...aro.org>,
netdev@...r.kernel.org, linux-mm@...ck.org,
Shakeel Butt <shakeelb@...gle.com>
Subject: Re: [PATCH v2 17/24] page_pool: Convert page_pool_return_skb_page()
to use netmem
On 09/01/2023 19.36, Matthew Wilcox wrote:
> On Fri, Jan 06, 2023 at 09:16:25PM +0100, Jesper Dangaard Brouer wrote:
>>
>>
>> On 06/01/2023 17.53, Matthew Wilcox wrote:
>>> On Fri, Jan 06, 2023 at 04:49:12PM +0100, Jesper Dangaard Brouer wrote:
>>>> On 05/01/2023 22.46, Matthew Wilcox (Oracle) wrote:
>>>>> This function accesses the pagepool members of struct page directly,
>>>>> so it needs to become netmem. Add page_pool_put_full_netmem() and
>>>>> page_pool_recycle_netmem().
>>>>>
>>>>> Signed-off-by: Matthew Wilcox (Oracle) <willy@...radead.org>
>>>>> ---
>>>>> include/net/page_pool.h | 14 +++++++++++++-
>>>>> net/core/page_pool.c | 13 ++++++-------
>>>>> 2 files changed, 19 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
>>>>> index fbb653c9f1da..126c04315929 100644
>>>>> --- a/include/net/page_pool.h
>>>>> +++ b/include/net/page_pool.h
>>>>> @@ -464,10 +464,16 @@ static inline void page_pool_put_page(struct page_pool *pool,
>>>>> }
>>>>> /* Same as above but will try to sync the entire area pool->max_len */
>>>>> +static inline void page_pool_put_full_netmem(struct page_pool *pool,
>>>>> + struct netmem *nmem, bool allow_direct)
>>>>> +{
>>>>> + page_pool_put_netmem(pool, nmem, -1, allow_direct);
>>>>> +}
>>>>> +
>>>>> static inline void page_pool_put_full_page(struct page_pool *pool,
>>>>> struct page *page, bool allow_direct)
>>>>> {
>>>>> - page_pool_put_page(pool, page, -1, allow_direct);
>>>>> + page_pool_put_full_netmem(pool, page_netmem(page), allow_direct);
>>>>> }
>>>>> /* Same as above but the caller must guarantee safe context. e.g NAPI */
>>>>> @@ -477,6 +483,12 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
>>>>> page_pool_put_full_page(pool, page, true);
>>>>> }
>>>>> +static inline void page_pool_recycle_netmem(struct page_pool *pool,
>>>>> + struct netmem *nmem)
>>>>> +{
>>>>> + page_pool_put_full_netmem(pool, nmem, true);
>>>> ^^^^
>>>>
>>>> It is not clear in what context page_pool_recycle_netmem() will be used,
>>>> but I think the 'true' (allow_direct=true) might be wrong here.
>>>>
>>>> It is only in limited special cases (RX-NAPI context) we can allow
>>>> direct return to the RX-alloc-cache.
>>>
>>> Mmm. It's a c'n'p of the previous function:
>>>
>>> static inline void page_pool_recycle_direct(struct page_pool *pool,
>>> struct page *page)
>>> {
>>> page_pool_put_full_page(pool, page, true);
>>> }
>>>
>>> so perhaps it's just badly named?
>>
>> Yes, I think so.
>>
>> Can we name it:
>> page_pool_recycle_netmem_direct
>>
>> And perhaps add a comment with a warning like:
>> /* Caller must guarantee safe context. e.g NAPI */
>>
>> Like the page_pool_recycle_direct() function has a comment.
>
> I don't really like the new name you're proposing here. Really,
> page_pool_recycle_direct() is the perfect name, it just has the wrong
> type.
>
> I considered the attached megapatch, but I don't think that's a great
> idea.
>
> So here's what I'm planning instead:
I do like below patch.
I must admit I had to lookup _Generic() when I started reviewing this
patchset. I think it makes a lot of sense to use here as it allow us to
easier convert drivers over.
We have 22 call spots in drivers:
$ git grep page_pool_recycle_direct drivers/net/ethernet/ | wc -l
22
But approx 9 drivers doing this (as each driver calls it in multiple
places).
>
> page_pool: Allow page_pool_recycle_direct() to take a netmem or a page
>
> With no better name for a variant of page_pool_recycle_direct() which
> takes a netmem instead of a page, use _Generic() to allow it to take
> either a page or a netmem argument. It's a bit ugly, but maybe not
> the worst alternative?
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@...radead.org>
>
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index abe3822a1125..1eed8ed2dcc1 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -477,12 +477,22 @@ static inline void page_pool_put_full_page(struct page_pool *pool,
> }
>
> /* Same as above but the caller must guarantee safe context. e.g NAPI */
> -static inline void page_pool_recycle_direct(struct page_pool *pool,
> +static inline void __page_pool_recycle_direct(struct page_pool *pool,
> + struct netmem *nmem)
> +{
> + page_pool_put_full_netmem(pool, nmem, true);
> +}
> +
> +static inline void __page_pool_recycle_page_direct(struct page_pool *pool,
> struct page *page)
> {
> - page_pool_put_full_page(pool, page, true);
> + page_pool_put_full_netmem(pool, page_netmem(page), true);
> }
>
> +#define page_pool_recycle_direct(pool, mem) _Generic((mem), \
> + struct netmem *: __page_pool_recycle_direct(pool, (struct netmem *)mem), \
> + struct page *: __page_pool_recycle_page_direct(pool, (struct page *)mem))
> +
> #define PAGE_POOL_DMA_USE_PP_FRAG_COUNT \
> (sizeof(dma_addr_t) > sizeof(unsigned long))
>
>
Powered by blists - more mailing lists