Re: [PATCH][v2] page_pool: refill page when alloc.count of pool is zero

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Tue, 11 Feb 2020 10:13:44 +0800
Li RongQing <lirongqing@baidu.com> wrote:

> "do {} while" in page_pool_refill_alloc_cache will always
> refill page once whether refill is true or false, and whether
> alloc.count of pool is less than PP_ALLOC_CACHE_REFILL or not
> this is wrong, and will cause overflow of pool->alloc.cache
> 
> the caller of __page_pool_get_cached should provide guarantee
> that pool->alloc.cache is safe to access, so in_serving_softirq
> should be removed as suggested by Jesper Dangaard Brouer in
> https://patchwork.ozlabs.org/patch/1233713/
> 
> so fix this issue by calling page_pool_refill_alloc_cache()
> only when pool->alloc.count is zero
> 
> Fixes: 44768decb7c0 ("page_pool: handle page recycle for NUMA_NO_NODE condition")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> Suggested: Jesper Dangaard Brouer <brouer@redhat.com>

You forgot the "-by" part of "suggested-by:", added it below so patchwork pick it up.

Suggested-by: Jesper Dangaard Brouer <brouer@redhat.com>


> ---
> v1-->v2: remove the in_serving_softirq test

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

I've tested the patch and gave it some exercise with my page_pool
benchmarks tools, everything looked good.

 
>  net/core/page_pool.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 9b7cbe35df37..10d2b255df5e 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -99,8 +99,7 @@ EXPORT_SYMBOL(page_pool_create);
>  static void __page_pool_return_page(struct page_pool *pool, struct page *page);
>  
>  noinline
> -static struct page *page_pool_refill_alloc_cache(struct page_pool *pool,
> -						 bool refill)
> +static struct page *page_pool_refill_alloc_cache(struct page_pool *pool)
>  {
>  	struct ptr_ring *r = &pool->ring;
>  	struct page *page;
> @@ -141,8 +140,7 @@ static struct page *page_pool_refill_alloc_cache(struct page_pool *pool,
>  			page = NULL;
>  			break;
>  		}
> -	} while (pool->alloc.count < PP_ALLOC_CACHE_REFILL &&
> -		 refill);
> +	} while (pool->alloc.count < PP_ALLOC_CACHE_REFILL);
>  
>  	/* Return last page */
>  	if (likely(pool->alloc.count > 0))
> @@ -155,20 +153,16 @@ static struct page *page_pool_refill_alloc_cache(struct page_pool *pool,
>  /* fast path */
>  static struct page *__page_pool_get_cached(struct page_pool *pool)
>  {
> -	bool refill = false;
>  	struct page *page;
>  
> -	/* Test for safe-context, caller should provide this guarantee */
> -	if (likely(in_serving_softirq())) {
> -		if (likely(pool->alloc.count)) {
> -			/* Fast-path */
> -			page = pool->alloc.cache[--pool->alloc.count];
> -			return page;
> -		}
> -		refill = true;
> +	/* Caller MUST guarantee safe non-concurrent access, e.g. softirq */
> +	if (likely(pool->alloc.count)) {
> +		/* Fast-path */
> +		page = pool->alloc.cache[--pool->alloc.count];
> +	} else {
> +		page = page_pool_refill_alloc_cache(pool);
>  	}
>  
> -	page = page_pool_refill_alloc_cache(pool, refill);
>  	return page;
>  }
>  


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer