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:   Mon, 9 Jan 2023 18:36:54 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     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 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:

    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))



View attachment "direct-netmem.diff" of type "text/plain" (11184 bytes)