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