netdev - Re: [PATCH v2 17/24] page_pool: Convert page_pool_return_skb

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 6 Jan 2023 21:16:25 +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 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.

--Jesper