Re: [PATCH] page_pool: fill page only when refill condition is true

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

On Wed,  5 Feb 2020 16:22:34 +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.
> 
> so fix it by calling page_pool_refill_alloc_cache() only when
> refill is true
> 
> Fixes: 44768decb7c0 ("page_pool: handle page recycle for NUMA_NO_NODE condition")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> Cc: Jesper Dangaard Brouer <brouer@redhat.com>

Hmmm... I'm not 100% convinced this is the right approach.

I do realize that in commit 44768decb7c0, I also added touching
pool->alloc.cache[] which was protected by "called under" in_serving_softirq().
(before I used a locked ptr_ring_consume(r)).

BUT maybe it will be better to remove, the test in_serving_softirq(),
because the caller should provide guarantee that pool->alloc.cache[] is
safe to access.

I added this in_serving_softirq() check, because I noticed NIC drivers
will call this from normal process context, during (1) initial fill of
their RX-rings, and (2) during driver RX-ring shutdown.  BUT in both
cases the NIC drivers will first have made sure that their RX-ring have
been disconnected and no concurrent accesses will happen.  Thus, access
to pool->alloc.cache[] is safe, so page_pool API should trust the
caller knows this.


> ---
>  net/core/page_pool.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 9b7cbe35df37..35ce663cb9de 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))
> @@ -156,7 +154,7 @@ static struct page *page_pool_refill_alloc_cache(struct page_pool *pool,
>  static struct page *__page_pool_get_cached(struct page_pool *pool)
>  {
>  	bool refill = false;
> -	struct page *page;
> +	struct page *page = NULL;
>  
>  	/* Test for safe-context, caller should provide this guarantee */
>  	if (likely(in_serving_softirq())) {
> @@ -168,7 +166,8 @@ static struct page *__page_pool_get_cached(struct page_pool *pool)
>  		refill = true;
>  	}
>  
> -	page = page_pool_refill_alloc_cache(pool, refill);
> +	if (refill)
> +		page = page_pool_refill_alloc_cache(pool);
>  	return page;
>  }

I guess, I instead propose:

 git diff
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);
  



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