| Message ID | 20240808214520.2648194-1-almasrymina@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] page_pool: unexport set dma_addr helper | expand |
On 08/08/2024 23.45, Mina Almasry wrote: > This helper doesn't need to be exported. Move it to page_pool_priv.h > > Moving the implementation to the .c file allows us to hide netmem > implementation details in internal header files rather than the public > file. > Hmm, I worry this is a performance paper cut. AFAICT this cause the page_pool_set_dma_addr() to be a function call, while it before was inlined and on 64bit archs it is a simple assignment "page->dma_addr = addr". See below, maybe a simple 'static' function define will resolve this. > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Mina Almasry <almasrymina@google.com> > > --- > > v2: https://patchwork.kernel.org/project/netdevbpf/patch/20240805212536.2172174-6-almasrymina@google.com/ > - Move get back to the public header. (Jakub) > - Move set to the internal header page_pool_priv.h (Jakub) > > --- > include/net/page_pool/helpers.h | 23 ----------------------- > net/core/page_pool.c | 17 +++++++++++++++++ > net/core/page_pool_priv.h | 6 ++++++ > 3 files changed, 23 insertions(+), 23 deletions(-) > > diff --git a/include/net/page_pool/helpers.h b/include/net/page_pool/helpers.h > index 2b43a893c619d..375656baa2d45 100644 > --- a/include/net/page_pool/helpers.h > +++ b/include/net/page_pool/helpers.h > @@ -423,24 +423,6 @@ static inline dma_addr_t page_pool_get_dma_addr(const struct page *page) > return page_pool_get_dma_addr_netmem(page_to_netmem((struct page *)page)); > } > > -static inline bool page_pool_set_dma_addr_netmem(netmem_ref netmem, > - dma_addr_t addr) > -{ > - struct page *page = netmem_to_page(netmem); > - > - if (PAGE_POOL_32BIT_ARCH_WITH_64BIT_DMA) { > - page->dma_addr = addr >> PAGE_SHIFT; > - > - /* We assume page alignment to shave off bottom bits, > - * if this "compression" doesn't work we need to drop. > - */ > - return addr != (dma_addr_t)page->dma_addr << PAGE_SHIFT; > - } > - > - page->dma_addr = addr; > - return false; > -} > - > /** > * page_pool_dma_sync_for_cpu - sync Rx page for CPU after it's written by HW > * @pool: &page_pool the @page belongs to > @@ -463,11 +445,6 @@ static inline void page_pool_dma_sync_for_cpu(const struct page_pool *pool, > page_pool_get_dma_dir(pool)); > } > > -static inline bool page_pool_set_dma_addr(struct page *page, dma_addr_t addr) > -{ > - return page_pool_set_dma_addr_netmem(page_to_netmem(page), addr); > -} > - > static inline bool page_pool_put(struct page_pool *pool) > { > return refcount_dec_and_test(&pool->user_cnt); > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > index 2abe6e919224d..d689a20780f40 100644 > --- a/net/core/page_pool.c > +++ b/net/core/page_pool.c > @@ -1099,3 +1099,20 @@ void page_pool_update_nid(struct page_pool *pool, int new_nid) > } > } > EXPORT_SYMBOL(page_pool_update_nid); > + > +bool page_pool_set_dma_addr_netmem(netmem_ref netmem, dma_addr_t addr) Maybe defining function as 'static bool' will make compiler inline it(?) > +{ > + struct page *page = netmem_to_page(netmem); > + > + if (PAGE_POOL_32BIT_ARCH_WITH_64BIT_DMA) { > + page->dma_addr = addr >> PAGE_SHIFT; > + > + /* We assume page alignment to shave off bottom bits, > + * if this "compression" doesn't work we need to drop. > + */ > + return addr != (dma_addr_t)page->dma_addr << PAGE_SHIFT; > + } > + > + page->dma_addr = addr; > + return false; > +} > diff --git a/net/core/page_pool_priv.h b/net/core/page_pool_priv.h > index 90665d40f1eb7..4fbc69ace7d21 100644 > --- a/net/core/page_pool_priv.h > +++ b/net/core/page_pool_priv.h > @@ -8,5 +8,11 @@ s32 page_pool_inflight(const struct page_pool *pool, bool strict); > int page_pool_list(struct page_pool *pool); > void page_pool_detached(struct page_pool *pool); > void page_pool_unlist(struct page_pool *pool); > +bool page_pool_set_dma_addr_netmem(netmem_ref netmem, dma_addr_t addr); > + > +static inline bool page_pool_set_dma_addr(struct page *page, dma_addr_t addr) > +{ > + return page_pool_set_dma_addr_netmem(page_to_netmem(page), addr); > +} > > #endif
On Fri, Aug 9, 2024 at 5:12 AM Jesper Dangaard Brouer <hawk@kernel.org> wrote: > > > > On 08/08/2024 23.45, Mina Almasry wrote: > > This helper doesn't need to be exported. Move it to page_pool_priv.h > > > > Moving the implementation to the .c file allows us to hide netmem > > implementation details in internal header files rather than the public > > file. > > > > Hmm, I worry this is a performance paper cut. > AFAICT this cause the page_pool_set_dma_addr() to be a function call, > while it before was inlined and on 64bit archs it is a simple assignment > "page->dma_addr = addr". > > See below, maybe a simple 'static' function define will resolve this. > Unfortunately I can't static this function; I end up having to call it from net/core/devmem.c file I'm adding, to set up netmem dma_addr. So this is a genuine performance papercut. To be honest, I didn't care much about preserving the inline. I think the only call site of this function is the slowpath allocation which does an alloc_pages_node(), and a dma mapping, so uninlining this function should be a drop in the ocean AFAICT. However, what I may be able to do is to put it as static inline in page_pool_priv.h, if there are no objections. That accomplishes the same thing for my purposes.
diff --git a/include/net/page_pool/helpers.h b/include/net/page_pool/helpers.h index 2b43a893c619d..375656baa2d45 100644 --- a/include/net/page_pool/helpers.h +++ b/include/net/page_pool/helpers.h @@ -423,24 +423,6 @@ static inline dma_addr_t page_pool_get_dma_addr(const struct page *page) return page_pool_get_dma_addr_netmem(page_to_netmem((struct page *)page)); } -static inline bool page_pool_set_dma_addr_netmem(netmem_ref netmem, - dma_addr_t addr) -{ - struct page *page = netmem_to_page(netmem); - - if (PAGE_POOL_32BIT_ARCH_WITH_64BIT_DMA) { - page->dma_addr = addr >> PAGE_SHIFT; - - /* We assume page alignment to shave off bottom bits, - * if this "compression" doesn't work we need to drop. - */ - return addr != (dma_addr_t)page->dma_addr << PAGE_SHIFT; - } - - page->dma_addr = addr; - return false; -} - /** * page_pool_dma_sync_for_cpu - sync Rx page for CPU after it's written by HW * @pool: &page_pool the @page belongs to @@ -463,11 +445,6 @@ static inline void page_pool_dma_sync_for_cpu(const struct page_pool *pool, page_pool_get_dma_dir(pool)); } -static inline bool page_pool_set_dma_addr(struct page *page, dma_addr_t addr) -{ - return page_pool_set_dma_addr_netmem(page_to_netmem(page), addr); -} - static inline bool page_pool_put(struct page_pool *pool) { return refcount_dec_and_test(&pool->user_cnt); diff --git a/net/core/page_pool.c b/net/core/page_pool.c index 2abe6e919224d..d689a20780f40 100644 --- a/net/core/page_pool.c +++ b/net/core/page_pool.c @@ -1099,3 +1099,20 @@ void page_pool_update_nid(struct page_pool *pool, int new_nid) } } EXPORT_SYMBOL(page_pool_update_nid); + +bool page_pool_set_dma_addr_netmem(netmem_ref netmem, dma_addr_t addr) +{ + struct page *page = netmem_to_page(netmem); + + if (PAGE_POOL_32BIT_ARCH_WITH_64BIT_DMA) { + page->dma_addr = addr >> PAGE_SHIFT; + + /* We assume page alignment to shave off bottom bits, + * if this "compression" doesn't work we need to drop. + */ + return addr != (dma_addr_t)page->dma_addr << PAGE_SHIFT; + } + + page->dma_addr = addr; + return false; +} diff --git a/net/core/page_pool_priv.h b/net/core/page_pool_priv.h index 90665d40f1eb7..4fbc69ace7d21 100644 --- a/net/core/page_pool_priv.h +++ b/net/core/page_pool_priv.h @@ -8,5 +8,11 @@ s32 page_pool_inflight(const struct page_pool *pool, bool strict); int page_pool_list(struct page_pool *pool); void page_pool_detached(struct page_pool *pool); void page_pool_unlist(struct page_pool *pool); +bool page_pool_set_dma_addr_netmem(netmem_ref netmem, dma_addr_t addr); + +static inline bool page_pool_set_dma_addr(struct page *page, dma_addr_t addr) +{ + return page_pool_set_dma_addr_netmem(page_to_netmem(page), addr); +} #endif
This helper doesn't need to be exported. Move it to page_pool_priv.h Moving the implementation to the .c file allows us to hide netmem implementation details in internal header files rather than the public file. Suggested-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Mina Almasry <almasrymina@google.com> --- v2: https://patchwork.kernel.org/project/netdevbpf/patch/20240805212536.2172174-6-almasrymina@google.com/ - Move get back to the public header. (Jakub) - Move set to the internal header page_pool_priv.h (Jakub) --- include/net/page_pool/helpers.h | 23 ----------------------- net/core/page_pool.c | 17 +++++++++++++++++ net/core/page_pool_priv.h | 6 ++++++ 3 files changed, 23 insertions(+), 23 deletions(-)