[PATCH net-next v2] page_pool: unexport set dma_addr helper

[PATCH net-next v2] page_pool: unexport set dma_addr helper

[Mina Almasry]

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

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
-- 
2.46.0.76.ge559c4bf1a-goog

Re: [PATCH net-next v2] page_pool: unexport set dma_addr helper

[Jesper Dangaard Brouer]

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

Re: [PATCH net-next v2] page_pool: unexport set dma_addr helper

[Mina Almasry]

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.

-- 
Thanks,
Mina