netdev - Re: [PATCH net-next v2] page_pool: unexport set dma

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]

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