page_pool: fill page only when refill condition is true

"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>
---
 net/core/page_pool.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

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

> -----邮件原件-----
> 发件人: Jesper Dangaard Brouer [mailto:brouer@redhat.com]
> 发送时间: 2020年2月5日 19:51
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: netdev@vger.kernel.org; brouer@redhat.com; Ilias Apalodimas
> <ilias.apalodimas@linaro.org>
> 主题: Re: [PATCH] page_pool: fill page only when refill condition is true
> 
> 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.
> 
> 

I think it is better and semantic clarity to keep in_serving_softirq, and we can see opinions of others

Thanks

-Li