* [PATCH] page_pool: fill page only when refill condition is true
@ 2020-02-05 8:22 Li RongQing
2020-02-05 11:50 ` Jesper Dangaard Brouer
0 siblings, 1 reply; 3+ messages in thread
From: Li RongQing @ 2020-02-05 8:22 UTC (permalink / raw)
To: netdev, brouer
"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(-)
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;
}
--
2.16.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* 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
* 答复: [PATCH] page_pool: fill page only when refill condition is true
2020-02-05 11:50 ` Jesper Dangaard Brouer
@ 2020-02-06 2:23 ` Li,Rongqing
0 siblings, 0 replies; 3+ messages in thread
From: Li,Rongqing @ 2020-02-06 2:23 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: netdev, Ilias Apalodimas
> -----邮件原件-----
> 发件人: 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-02-06 2:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).