* Re: [PATCH] page_pool: fill page only when refill condition is true
2020-02-05 8:22 [PATCH] page_pool: fill page only when refill condition is true Li RongQing
@ 2020-02-05 11:50 ` Jesper Dangaard Brouer
2020-02-06 2:23 ` 答复: " Li,Rongqing
0 siblings, 1 reply; 3+ messages in thread
From: Jesper Dangaard Brouer @ 2020-02-05 11:50 UTC (permalink / raw)
To: Li RongQing; +Cc: netdev, brouer, Ilias Apalodimas
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
^ permalink raw reply related [flat|nested] 3+ messages in thread