* Re: [PATCH][v2] page_pool: refill page when alloc.count of pool is zero
2020-02-11 2:13 [PATCH][v2] page_pool: refill page when alloc.count of pool is zero Li RongQing
@ 2020-02-12 8:01 ` Ilias Apalodimas
2020-02-12 8:36 ` Jesper Dangaard Brouer
2020-02-12 8:54 ` Ilias Apalodimas
2 siblings, 0 replies; 4+ messages in thread
From: Ilias Apalodimas @ 2020-02-12 8:01 UTC (permalink / raw)
To: Li RongQing; +Cc: netdev, brouer
Hi Li,
On Tue, Feb 11, 2020 at 10:13:44AM +0800, Li RongQing 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
Please include me on any future mails regarding pate pool. I almost missed this
one
Thanks
/Ilias
>
> 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>
> ---
> v1-->v2: remove the in_serving_softirq test
>
> 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;
> }
>
> --
> 2.16.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH][v2] page_pool: refill page when alloc.count of pool is zero
2020-02-11 2:13 [PATCH][v2] page_pool: refill page when alloc.count of pool is zero Li RongQing
2020-02-12 8:01 ` Ilias Apalodimas
@ 2020-02-12 8:36 ` Jesper Dangaard Brouer
2020-02-12 8:54 ` Ilias Apalodimas
2 siblings, 0 replies; 4+ messages in thread
From: Jesper Dangaard Brouer @ 2020-02-12 8:36 UTC (permalink / raw)
To: Li RongQing; +Cc: netdev, brouer
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH][v2] page_pool: refill page when alloc.count of pool is zero
2020-02-11 2:13 [PATCH][v2] page_pool: refill page when alloc.count of pool is zero Li RongQing
2020-02-12 8:01 ` Ilias Apalodimas
2020-02-12 8:36 ` Jesper Dangaard Brouer
@ 2020-02-12 8:54 ` Ilias Apalodimas
2 siblings, 0 replies; 4+ messages in thread
From: Ilias Apalodimas @ 2020-02-12 8:54 UTC (permalink / raw)
To: Li RongQing; +Cc: netdev, brouer
On Tue, Feb 11, 2020 at 10:13:44AM +0800, Li RongQing 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>
> ---
> v1-->v2: remove the in_serving_softirq test
>
> 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;
> }
>
> --
> 2.16.2
>
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
^ permalink raw reply [flat|nested] 4+ messages in thread