Re: [PATCH v2 05/24] page_pool: Start using netmem in allocation path.

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Thu, Jan 05, 2023 at 09:46:12PM +0000, Matthew Wilcox (Oracle) wrote:
> Convert __page_pool_alloc_page_order() and __page_pool_alloc_pages_slow()
> to use netmem internally.  This removes a couple of calls
> to compound_head() that are hidden inside put_page().
> Convert trace_page_pool_state_hold(), page_pool_dma_map() and
> page_pool_set_pp_info() to take a netmem argument.
>
> Saves 83 bytes of text in __page_pool_alloc_page_order() and 98 in
> __page_pool_alloc_pages_slow() for a total of 181 bytes.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  include/trace/events/page_pool.h | 14 +++++------
>  net/core/page_pool.c             | 42 +++++++++++++++++---------------
>  2 files changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/include/trace/events/page_pool.h b/include/trace/events/page_pool.h
> index 113aad0c9e5b..d1237a7ce481 100644
> --- a/include/trace/events/page_pool.h
> +++ b/include/trace/events/page_pool.h
> @@ -67,26 +67,26 @@ TRACE_EVENT(page_pool_state_release,
>  TRACE_EVENT(page_pool_state_hold,
>
>  	TP_PROTO(const struct page_pool *pool,
> -		 const struct page *page, u32 hold),
> +		 const struct netmem *nmem, u32 hold),
>
> -	TP_ARGS(pool, page, hold),
> +	TP_ARGS(pool, nmem, hold),
>
>  	TP_STRUCT__entry(
>  		__field(const struct page_pool *,	pool)
> -		__field(const struct page *,		page)
> +		__field(const struct netmem *,		nmem)
>  		__field(u32,				hold)
>  		__field(unsigned long,			pfn)
>  	),
>
>  	TP_fast_assign(
>  		__entry->pool	= pool;
> -		__entry->page	= page;
> +		__entry->nmem	= nmem;
>  		__entry->hold	= hold;
> -		__entry->pfn	= page_to_pfn(page);
> +		__entry->pfn	= netmem_pfn(nmem);
>  	),
>
> -	TP_printk("page_pool=%p page=%p pfn=0x%lx hold=%u",
> -		  __entry->pool, __entry->page, __entry->pfn, __entry->hold)
> +	TP_printk("page_pool=%p netmem=%p pfn=0x%lx hold=%u",
> +		  __entry->pool, __entry->nmem, __entry->pfn, __entry->hold)
>  );
>
>  TRACE_EVENT(page_pool_update_nid,
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 437241aba5a7..4e985502c569 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -304,8 +304,9 @@ static void page_pool_dma_sync_for_device(struct page_pool *pool,
>  					 pool->p.dma_dir);
>  }
>
> -static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
> +static bool page_pool_dma_map(struct page_pool *pool, struct netmem *nmem)
>  {
> +	struct page *page = netmem_page(nmem);
>  	dma_addr_t dma;
>
>  	/* Setup DMA mapping: use 'struct page' area for storing DMA-addr
> @@ -328,12 +329,12 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
>  }
>
>  static void page_pool_set_pp_info(struct page_pool *pool,
> -				  struct page *page)
> +				  struct netmem *nmem)
>  {
> -	page->pp = pool;
> -	page->pp_magic |= PP_SIGNATURE;
> +	nmem->pp = pool;
> +	nmem->pp_magic |= PP_SIGNATURE;
>  	if (pool->p.init_callback)
> -		pool->p.init_callback(page, pool->p.init_arg);
> +		pool->p.init_callback(netmem_page(nmem), pool->p.init_arg);
>  }
>
>  static void page_pool_clear_pp_info(struct netmem *nmem)
> @@ -345,26 +346,26 @@ static void page_pool_clear_pp_info(struct netmem *nmem)
>  static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
>  						 gfp_t gfp)
>  {
> -	struct page *page;
> +	struct netmem *nmem;
>
>  	gfp |= __GFP_COMP;
> -	page = alloc_pages_node(pool->p.nid, gfp, pool->p.order);
> -	if (unlikely(!page))
> +	nmem = page_netmem(alloc_pages_node(pool->p.nid, gfp, pool->p.order));
> +	if (unlikely(!nmem))
>  		return NULL;
>
>  	if ((pool->p.flags & PP_FLAG_DMA_MAP) &&
> -	    unlikely(!page_pool_dma_map(pool, page))) {
> -		put_page(page);
> +	    unlikely(!page_pool_dma_map(pool, nmem))) {
> +		netmem_put(nmem);
>  		return NULL;
>  	}
>
>  	alloc_stat_inc(pool, slow_high_order);
> -	page_pool_set_pp_info(pool, page);
> +	page_pool_set_pp_info(pool, nmem);
>
>  	/* Track how many pages are held 'in-flight' */
>  	pool->pages_state_hold_cnt++;
> -	trace_page_pool_state_hold(pool, page, pool->pages_state_hold_cnt);
> -	return page;
> +	trace_page_pool_state_hold(pool, nmem, pool->pages_state_hold_cnt);
> +	return netmem_page(nmem);
>  }
>
>  /* slow path */
> @@ -398,18 +399,18 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
>  	 * page element have not been (possibly) DMA mapped.
>  	 */
>  	for (i = 0; i < nr_pages; i++) {
> -		page = pool->alloc.cache[i];
> +		struct netmem *nmem = page_netmem(pool->alloc.cache[i]);
>  		if ((pp_flags & PP_FLAG_DMA_MAP) &&
> -		    unlikely(!page_pool_dma_map(pool, page))) {
> -			put_page(page);
> +		    unlikely(!page_pool_dma_map(pool, nmem))) {
> +			netmem_put(nmem);
>  			continue;
>  		}
>
> -		page_pool_set_pp_info(pool, page);
> -		pool->alloc.cache[pool->alloc.count++] = page;
> +		page_pool_set_pp_info(pool, nmem);
> +		pool->alloc.cache[pool->alloc.count++] = netmem_page(nmem);
>  		/* Track how many pages are held 'in-flight' */
>  		pool->pages_state_hold_cnt++;
> -		trace_page_pool_state_hold(pool, page,
> +		trace_page_pool_state_hold(pool, nmem,
>  					   pool->pages_state_hold_cnt);
>  	}
>
> @@ -421,7 +422,8 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
>  		page = NULL;
>  	}
>
> -	/* When page just alloc'ed is should/must have refcnt 1. */
> +	/* When page just allocated it should have refcnt 1 (but may have
> +	 * speculative references) */
>  	return page;
>  }
>
> --
> 2.35.1
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>