Re: [PATCH net-next RFC 1/2] page_pool: add page recycling support based on elevated refcnt

[Jesper Dangaard Brouer]

On 30/06/2021 11.17, Yunsheng Lin wrote:
> Currently page pool only support page recycling only when
> refcnt of page is one, which means it can not support the
> split page recycling implemented in the most ethernet driver.

Cc. Alex Duyck as I consider him an expert in this area.


> So add elevated refcnt support in page pool, and support
> allocating page frag to enable multi-frames-per-page based
> on the elevated refcnt support.
>
> As the elevated refcnt is per page, and there is no space
> for that in "struct page" now, so add a dynamically allocated
> "struct page_pool_info" to record page pool ptr and refcnt
> corrsponding to a page for now. Later, we can recycle the
> "struct page_pool_info" too, or use part of page memory to
> record pp_info.

I'm not happy with allocating a memory (slab) object "struct 
page_pool_info" per page.

This also gives us an extra level of indirection.


You are also adding a page "frag" API inside page pool, which I'm not 
100% convinced belongs inside page_pool APIs.

Please notice the APIs that Alex Duyck added in mm/page_alloc.c:

 __page_frag_cache_refill() + __page_frag_cache_drain() + 
page_frag_alloc_align()


No more comments below, but kept it if Alex wants to review the details.

> Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
> ---
>   drivers/net/ethernet/marvell/mvneta.c           |   6 +-
>   drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c |   2 +-
>   include/linux/mm_types.h                        |   2 +-
>   include/linux/skbuff.h                          |   4 +-
>   include/net/page_pool.h                         |  30 +++-
>   net/core/page_pool.c                            | 215 ++++++++++++++++++++----
>   6 files changed, 207 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 88a7550..5a29af2 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -2327,7 +2327,7 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
>   	if (!skb)
>   		return ERR_PTR(-ENOMEM);
>   
> -	skb_mark_for_recycle(skb, virt_to_page(xdp->data), pool);
> +	skb_mark_for_recycle(skb);
>   
>   	skb_reserve(skb, xdp->data - xdp->data_hard_start);
>   	skb_put(skb, xdp->data_end - xdp->data);
> @@ -2339,10 +2339,6 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
>   		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
>   				skb_frag_page(frag), skb_frag_off(frag),
>   				skb_frag_size(frag), PAGE_SIZE);
> -		/* We don't need to reset pp_recycle here. It's already set, so
> -		 * just mark fragments for recycling.
> -		 */
> -		page_pool_store_mem_info(skb_frag_page(frag), pool);
>   	}
>   
>   	return skb;
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 3135220..540e387 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -3997,7 +3997,7 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
>   		}
>   
>   		if (pp)
> -			skb_mark_for_recycle(skb, page, pp);
> +			skb_mark_for_recycle(skb);
>   		else
>   			dma_unmap_single_attrs(dev->dev.parent, dma_addr,
>   					       bm_pool->buf_size, DMA_FROM_DEVICE,
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 862f88a..cf613df 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -101,7 +101,7 @@ struct page {
>   			 * page_pool allocated pages.
>   			 */
>   			unsigned long pp_magic;
> -			struct page_pool *pp;
> +			struct page_pool_info *pp_info;
>   			unsigned long _pp_mapping_pad;
>   			/**
>   			 * @dma_addr: might require a 64-bit value on
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index b2db9cd..7795979 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -4711,11 +4711,9 @@ static inline u64 skb_get_kcov_handle(struct sk_buff *skb)
>   }
>   
>   #ifdef CONFIG_PAGE_POOL
> -static inline void skb_mark_for_recycle(struct sk_buff *skb, struct page *page,
> -					struct page_pool *pp)
> +static inline void skb_mark_for_recycle(struct sk_buff *skb)
>   {
>   	skb->pp_recycle = 1;
> -	page_pool_store_mem_info(page, pp);
>   }
>   #endif
>   
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index 3dd62dd..44e7545 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -45,7 +45,9 @@
>   					* Please note DMA-sync-for-CPU is still
>   					* device driver responsibility
>   					*/
> -#define PP_FLAG_ALL		(PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV)
> +#define PP_FLAG_PAGECNT_BIAS	BIT(2)	/* Enable elevated refcnt */
> +#define PP_FLAG_ALL		(PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV |\
> +				 PP_FLAG_PAGECNT_BIAS)
>   
>   /*
>    * Fast allocation side cache array/stack
> @@ -77,6 +79,7 @@ struct page_pool_params {
>   	enum dma_data_direction dma_dir; /* DMA mapping direction */
>   	unsigned int	max_len; /* max DMA sync memory size */
>   	unsigned int	offset;  /* DMA addr offset */
> +	unsigned int	frag_size;
>   };
>   
>   struct page_pool {
> @@ -88,6 +91,8 @@ struct page_pool {
>   	unsigned long defer_warn;
>   
>   	u32 pages_state_hold_cnt;
> +	unsigned int frag_offset;
> +	struct page *frag_page;
>   
>   	/*
>   	 * Data structure for allocation side
> @@ -128,6 +133,11 @@ struct page_pool {
>   	u64 destroy_cnt;
>   };
>   
> +struct page_pool_info {
> +	struct page_pool *pp;
> +	int pagecnt_bias;
> +};
> +
>   struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
>   
>   static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool)
> @@ -137,6 +147,17 @@ static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool)
>   	return page_pool_alloc_pages(pool, gfp);
>   }
>   
> +struct page *page_pool_alloc_frag(struct page_pool *pool,
> +				  unsigned int *offset, gfp_t gfp);
> +
> +static inline struct page *page_pool_dev_alloc_frag(struct page_pool *pool,
> +						    unsigned int *offset)
> +{
> +	gfp_t gfp = (GFP_ATOMIC | __GFP_NOWARN);
> +
> +	return page_pool_alloc_frag(pool, offset, gfp);
> +}
> +
>   /* get the stored dma direction. A driver might decide to treat this locally and
>    * avoid the extra cache line from page_pool to determine the direction
>    */
> @@ -253,11 +274,4 @@ static inline void page_pool_ring_unlock(struct page_pool *pool)
>   		spin_unlock_bh(&pool->ring.producer_lock);
>   }
>   
> -/* Store mem_info on struct page and use it while recycling skb frags */
> -static inline
> -void page_pool_store_mem_info(struct page *page, struct page_pool *pp)
> -{
> -	page->pp = pp;
> -}
> -
>   #endif /* _NET_PAGE_POOL_H */
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 5e4eb45..95d94a7 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -206,6 +206,49 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
>   	return true;
>   }
>   
> +static int page_pool_set_pp_info(struct page_pool *pool,
> +				 struct page *page, gfp_t gfp)
> +{
> +	struct page_pool_info *pp_info;
> +
> +	pp_info = kzalloc_node(sizeof(*pp_info), gfp, pool->p.nid);
> +	if (!pp_info)
> +		return -ENOMEM;
> +
> +	if (pool->p.flags & PP_FLAG_PAGECNT_BIAS) {
> +		page_ref_add(page, USHRT_MAX);
> +		pp_info->pagecnt_bias = USHRT_MAX;
> +	} else {
> +		pp_info->pagecnt_bias = 0;
> +	}
> +
> +	page->pp_magic |= PP_SIGNATURE;
> +	pp_info->pp = pool;
> +	page->pp_info = pp_info;
> +	return 0;
> +}
> +
> +static int page_pool_clear_pp_info(struct page *page)
> +{
> +	struct page_pool_info *pp_info = page->pp_info;
> +	int bias;
> +
> +	bias = pp_info->pagecnt_bias;
> +
> +	kfree(pp_info);
> +	page->pp_info = NULL;
> +	page->pp_magic = 0;
> +
> +	return bias;
> +}
> +
> +static void page_pool_clear_and_drain_page(struct page *page)
> +{
> +	int bias = page_pool_clear_pp_info(page);
> +
> +	__page_frag_cache_drain(page, bias + 1);
> +}
> +
>   static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
>   						 gfp_t gfp)
>   {
> @@ -216,13 +259,16 @@ static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
>   	if (unlikely(!page))
>   		return NULL;
>   
> -	if ((pool->p.flags & PP_FLAG_DMA_MAP) &&
> -	    unlikely(!page_pool_dma_map(pool, page))) {
> +	if (unlikely(page_pool_set_pp_info(pool, page, gfp))) {
>   		put_page(page);
>   		return NULL;
>   	}
>   
> -	page->pp_magic |= PP_SIGNATURE;
> +	if ((pool->p.flags & PP_FLAG_DMA_MAP) &&
> +	    unlikely(!page_pool_dma_map(pool, page))) {
> +		page_pool_clear_and_drain_page(page);
> +		return NULL;
> +	}
>   
>   	/* Track how many pages are held 'in-flight' */
>   	pool->pages_state_hold_cnt++;
> @@ -261,12 +307,17 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
>   	 */
>   	for (i = 0; i < nr_pages; i++) {
>   		page = pool->alloc.cache[i];
> +		if (unlikely(page_pool_set_pp_info(pool, page, gfp))) {
> +			put_page(page);
> +			continue;
> +		}
> +
>   		if ((pp_flags & PP_FLAG_DMA_MAP) &&
>   		    unlikely(!page_pool_dma_map(pool, page))) {
> -			put_page(page);
> +			page_pool_clear_and_drain_page(page);
>   			continue;
>   		}
> -		page->pp_magic |= PP_SIGNATURE;
> +
>   		pool->alloc.cache[pool->alloc.count++] = page;
>   		/* Track how many pages are held 'in-flight' */
>   		pool->pages_state_hold_cnt++;
> @@ -284,6 +335,25 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
>   	return page;
>   }
>   
> +static void page_pool_sub_bias(struct page *page, int nr)
> +{
> +	struct page_pool_info *pp_info = page->pp_info;
> +
> +	/* "pp_info->pagecnt_bias == 0" indicates the PAGECNT_BIAS
> +	 * flags is not set.
> +	 */
> +	if (!pp_info->pagecnt_bias)
> +		return;
> +
> +	/* Make sure pagecnt_bias > 0 for elevated refcnt case */
> +	if (unlikely(pp_info->pagecnt_bias <= nr)) {
> +		page_ref_add(page, USHRT_MAX);
> +		pp_info->pagecnt_bias += USHRT_MAX;
> +	}
> +
> +	pp_info->pagecnt_bias -= nr;
> +}
> +
>   /* For using page_pool replace: alloc_pages() API calls, but provide
>    * synchronization guarantee for allocation side.
>    */
> @@ -293,15 +363,66 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp)
>   
>   	/* Fast-path: Get a page from cache */
>   	page = __page_pool_get_cached(pool);
> -	if (page)
> +	if (page) {
> +		page_pool_sub_bias(page, 1);
>   		return page;
> +	}
>   
>   	/* Slow-path: cache empty, do real allocation */
>   	page = __page_pool_alloc_pages_slow(pool, gfp);
> +	if (page)
> +		page_pool_sub_bias(page, 1);
> +
>   	return page;
>   }
>   EXPORT_SYMBOL(page_pool_alloc_pages);
>   
> +struct page *page_pool_alloc_frag(struct page_pool *pool,
> +				  unsigned int *offset, gfp_t gfp)
> +{
> +	unsigned int frag_offset = pool->frag_offset;
> +	unsigned int frag_size = pool->p.frag_size;
> +	struct page *frag_page = pool->frag_page;
> +	unsigned int max_len = pool->p.max_len;
> +
> +	if (!frag_page || frag_offset + frag_size > max_len) {
> +		frag_page = page_pool_alloc_pages(pool, gfp);
> +		if (unlikely(!frag_page)) {
> +			pool->frag_page = NULL;
> +			return NULL;
> +		}
> +
> +		pool->frag_page = frag_page;
> +		frag_offset = 0;
> +
> +		page_pool_sub_bias(frag_page, max_len / frag_size - 1);
> +	}
> +
> +	*offset = frag_offset;
> +	pool->frag_offset = frag_offset + frag_size;
> +
> +	return frag_page;
> +}
> +EXPORT_SYMBOL(page_pool_alloc_frag);
> +
> +static void page_pool_empty_frag(struct page_pool *pool)
> +{
> +	unsigned int frag_offset = pool->frag_offset;
> +	unsigned int frag_size = pool->p.frag_size;
> +	struct page *frag_page = pool->frag_page;
> +	unsigned int max_len = pool->p.max_len;
> +
> +	if (!frag_page)
> +		return;
> +
> +	while (frag_offset + frag_size <= max_len) {
> +		page_pool_put_full_page(pool, frag_page, false);
> +		frag_offset += frag_size;
> +	}
> +
> +	pool->frag_page = NULL;
> +}
> +
>   /* Calculate distance between two u32 values, valid if distance is below 2^(31)
>    *  https://en.wikipedia.org/wiki/Serial_number_arithmetic#General_Solution
>    */
> @@ -326,10 +447,11 @@ static s32 page_pool_inflight(struct page_pool *pool)
>    * a regular page (that will eventually be returned to the normal
>    * page-allocator via put_page).
>    */
> -void page_pool_release_page(struct page_pool *pool, struct page *page)
> +static int __page_pool_release_page(struct page_pool *pool,
> +				    struct page *page)
>   {
>   	dma_addr_t dma;
> -	int count;
> +	int bias, count;
>   
>   	if (!(pool->p.flags & PP_FLAG_DMA_MAP))
>   		/* Always account for inflight pages, even if we didn't
> @@ -345,22 +467,29 @@ void page_pool_release_page(struct page_pool *pool, struct page *page)
>   			     DMA_ATTR_SKIP_CPU_SYNC);
>   	page_pool_set_dma_addr(page, 0);
>   skip_dma_unmap:
> -	page->pp_magic = 0;
> +	bias = page_pool_clear_pp_info(page);
>   
>   	/* This may be the last page returned, releasing the pool, so
>   	 * it is not safe to reference pool afterwards.
>   	 */
>   	count = atomic_inc_return(&pool->pages_state_release_cnt);
>   	trace_page_pool_state_release(pool, page, count);
> +	return bias;
> +}
> +
> +void page_pool_release_page(struct page_pool *pool, struct page *page)
> +{
> +	int bias = __page_pool_release_page(pool, page);
> +
> +	WARN_ONCE(bias, "PAGECNT_BIAS is not supposed to be enabled\n");
>   }
>   EXPORT_SYMBOL(page_pool_release_page);
>   
>   /* Return a page to the page allocator, cleaning up our state */
>   static void page_pool_return_page(struct page_pool *pool, struct page *page)
>   {
> -	page_pool_release_page(pool, page);
> +	__page_frag_cache_drain(page, __page_pool_release_page(pool, page) + 1);
>   
> -	put_page(page);
>   	/* An optimization would be to call __free_pages(page, pool->p.order)
>   	 * knowing page is not part of page-cache (thus avoiding a
>   	 * __page_cache_release() call).
> @@ -395,7 +524,16 @@ static bool page_pool_recycle_in_cache(struct page *page,
>   	return true;
>   }
>   
> -/* If the page refcnt == 1, this will try to recycle the page.
> +static bool page_pool_bias_page_recyclable(struct page *page, int bias)
> +{
> +	int ref = page_ref_dec_return(page);
> +
> +	WARN_ON(ref < bias);
> +	return ref == bias + 1;
> +}
> +
> +/* If pagecnt_bias == 0 and the page refcnt == 1, this will try to
> + * recycle the page.
>    * if PP_FLAG_DMA_SYNC_DEV is set, we'll try to sync the DMA area for
>    * the configured size min(dma_sync_size, pool->max_len).
>    * If the page refcnt != 1, then the page will be returned to memory
> @@ -405,16 +543,35 @@ static __always_inline struct page *
>   __page_pool_put_page(struct page_pool *pool, struct page *page,
>   		     unsigned int dma_sync_size, bool allow_direct)
>   {
> -	/* This allocator is optimized for the XDP mode that uses
> +	int bias = page->pp_info->pagecnt_bias;
> +
> +	/* Handle the elevated refcnt case first:
> +	 * multi-frames-per-page, it is likely from the skb, which
> +	 * is likely called in non-sofrirq context, so do not recycle
> +	 * it in pool->alloc.
> +	 *
> +	 * Then handle non-elevated refcnt case:
>   	 * one-frame-per-page, but have fallbacks that act like the
>   	 * regular page allocator APIs.
> -	 *
>   	 * refcnt == 1 means page_pool owns page, and can recycle it.
>   	 *
>   	 * page is NOT reusable when allocated when system is under
>   	 * some pressure. (page_is_pfmemalloc)
>   	 */
> -	if (likely(page_ref_count(page) == 1 && !page_is_pfmemalloc(page))) {
> +	if (bias) {
> +		/* We have gave some refcnt to the stack, so wait for
> +		 * all refcnt of the stack to be decremented before
> +		 * enabling recycling.
> +		 */
> +		if (!page_pool_bias_page_recyclable(page, bias))
> +			return NULL;
> +
> +		/* only enable recycling when it is not pfmemalloced */
> +		if (!page_is_pfmemalloc(page))
> +			return page;
> +
> +	} else if (likely(page_ref_count(page) == 1 &&
> +			  !page_is_pfmemalloc(page))) {
>   		/* Read barrier done in page_ref_count / READ_ONCE */
>   
>   		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
> @@ -428,22 +585,8 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
>   		/* Page found as candidate for recycling */
>   		return page;
>   	}
> -	/* Fallback/non-XDP mode: API user have elevated refcnt.
> -	 *
> -	 * Many drivers split up the page into fragments, and some
> -	 * want to keep doing this to save memory and do refcnt based
> -	 * recycling. Support this use case too, to ease drivers
> -	 * switching between XDP/non-XDP.
> -	 *
> -	 * In-case page_pool maintains the DMA mapping, API user must
> -	 * call page_pool_put_page once.  In this elevated refcnt
> -	 * case, the DMA is unmapped/released, as driver is likely
> -	 * doing refcnt based recycle tricks, meaning another process
> -	 * will be invoking put_page.
> -	 */
> -	/* Do not replace this with page_pool_return_page() */
> +
>   	page_pool_release_page(pool, page);
> -	put_page(page);
>   
>   	return NULL;
>   }
> @@ -452,6 +595,7 @@ void page_pool_put_page(struct page_pool *pool, struct page *page,
>   			unsigned int dma_sync_size, bool allow_direct)
>   {
>   	page = __page_pool_put_page(pool, page, dma_sync_size, allow_direct);
> +
>   	if (page && !page_pool_recycle_in_ring(pool, page)) {
>   		/* Cache full, fallback to free pages */
>   		page_pool_return_page(pool, page);
> @@ -503,8 +647,11 @@ static void page_pool_empty_ring(struct page_pool *pool)
>   
>   	/* Empty recycle ring */
>   	while ((page = ptr_ring_consume_bh(&pool->ring))) {
> -		/* Verify the refcnt invariant of cached pages */
> -		if (!(page_ref_count(page) == 1))
> +		/* Verify the refcnt invariant of cached pages for
> +		 * non elevated refcnt case.
> +		 */
> +		if (!(pool->p.flags & PP_FLAG_PAGECNT_BIAS) &&
> +		    !(page_ref_count(page) == 1))
>   			pr_crit("%s() page_pool refcnt %d violation\n",
>   				__func__, page_ref_count(page));
>   
> @@ -544,6 +691,7 @@ static void page_pool_empty_alloc_cache_once(struct page_pool *pool)
>   
>   static void page_pool_scrub(struct page_pool *pool)
>   {
> +	page_pool_empty_frag(pool);
>   	page_pool_empty_alloc_cache_once(pool);
>   	pool->destroy_cnt++;
>   
> @@ -637,14 +785,13 @@ bool page_pool_return_skb_page(struct page *page)
>   	if (unlikely(page->pp_magic != PP_SIGNATURE))
>   		return false;
>   
> -	pp = page->pp;
> +	pp = page->pp_info->pp;
>   
>   	/* Driver set this to memory recycling info. Reset it on recycle.
>   	 * This will *not* work for NIC using a split-page memory model.
>   	 * The page will be returned to the pool here regardless of the
>   	 * 'flipped' fragment being in use or not.
>   	 */
> -	page->pp = NULL;
>   	page_pool_put_full_page(pp, page, false);
>   
>   	return true;