[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3e78e255-d50a-40fb-a438-bfcebb11b049@kernel.org>
Date: Fri, 9 Aug 2024 11:12:31 +0200
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Mina Almasry <almasrymina@...gle.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Cc: Ilias Apalodimas <ilias.apalodimas@...aro.org>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v2] page_pool: unexport set dma_addr helper
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@...nel.org>
> Signed-off-by: Mina Almasry <almasrymina@...gle.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
Powered by blists - more mailing lists