* [PATCH net-next] net: page_pool: fix recycle stats for system page_pool allocator
@ 2024-02-16 9:25 Lorenzo Bianconi
2024-02-16 10:32 ` Alexander Lobakin
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Lorenzo Bianconi @ 2024-02-16 9:25 UTC (permalink / raw)
To: netdev
Cc: lorenzo.bianconi, kuba, davem, edumazet, pabeni, hawk,
ilias.apalodimas, linyunsheng, toke, aleksander.lobakin
Use global percpu page_pool_recycle_stats counter for system page_pool
allocator instead of allocating a separate percpu variable for each
(also percpu) page pool instance.
Reviewed-by: Toke Hoiland-Jorgensen <toke@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
include/net/page_pool/types.h | 5 +++--
net/core/dev.c | 1 +
net/core/page_pool.c | 22 +++++++++++++++++-----
3 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
index 3828396ae60c..2515cca6518b 100644
--- a/include/net/page_pool/types.h
+++ b/include/net/page_pool/types.h
@@ -18,8 +18,9 @@
* Please note DMA-sync-for-CPU is still
* device driver responsibility
*/
-#define PP_FLAG_ALL (PP_FLAG_DMA_MAP |\
- PP_FLAG_DMA_SYNC_DEV)
+#define PP_FLAG_SYSTEM_POOL BIT(2) /* Global system page_pool */
+#define PP_FLAG_ALL (PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV | \
+ PP_FLAG_SYSTEM_POOL)
/*
* Fast allocation side cache array/stack
diff --git a/net/core/dev.c b/net/core/dev.c
index cc9c2eda65ac..c588808be77f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11738,6 +11738,7 @@ static int net_page_pool_create(int cpuid)
#if IS_ENABLED(CONFIG_PAGE_POOL)
struct page_pool_params page_pool_params = {
.pool_size = SYSTEM_PERCPU_PAGE_POOL_SIZE,
+ .flags = PP_FLAG_SYSTEM_POOL,
.nid = NUMA_NO_NODE,
};
struct page_pool *pp_ptr;
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 89c835fcf094..8f0c4e76181b 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -31,6 +31,8 @@
#define BIAS_MAX (LONG_MAX >> 1)
#ifdef CONFIG_PAGE_POOL_STATS
+static DEFINE_PER_CPU(struct page_pool_recycle_stats, pp_system_recycle_stats);
+
/* alloc_stat_inc is intended to be used in softirq context */
#define alloc_stat_inc(pool, __stat) (pool->alloc_stats.__stat++)
/* recycle_stat_inc is safe to use when preemption is possible. */
@@ -220,14 +222,23 @@ static int page_pool_init(struct page_pool *pool,
pool->has_init_callback = !!pool->slow.init_callback;
#ifdef CONFIG_PAGE_POOL_STATS
- pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
- if (!pool->recycle_stats)
- return -ENOMEM;
+ if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL)) {
+ pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
+ if (!pool->recycle_stats)
+ return -ENOMEM;
+ } else {
+ /* For system page pool instance we use a singular stats object
+ * instead of allocating a separate percpu variable for each
+ * (also percpu) page pool instance.
+ */
+ pool->recycle_stats = &pp_system_recycle_stats;
+ }
#endif
if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0) {
#ifdef CONFIG_PAGE_POOL_STATS
- free_percpu(pool->recycle_stats);
+ if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL))
+ free_percpu(pool->recycle_stats);
#endif
return -ENOMEM;
}
@@ -251,7 +262,8 @@ static void page_pool_uninit(struct page_pool *pool)
put_device(pool->p.dev);
#ifdef CONFIG_PAGE_POOL_STATS
- free_percpu(pool->recycle_stats);
+ if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL))
+ free_percpu(pool->recycle_stats);
#endif
}
--
2.43.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: page_pool: fix recycle stats for system page_pool allocator
2024-02-16 9:25 [PATCH net-next] net: page_pool: fix recycle stats for system page_pool allocator Lorenzo Bianconi
@ 2024-02-16 10:32 ` Alexander Lobakin
2024-02-17 3:46 ` Yunsheng Lin
2024-02-19 20:50 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: Alexander Lobakin @ 2024-02-16 10:32 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: netdev, lorenzo.bianconi, kuba, davem, edumazet, pabeni, hawk,
ilias.apalodimas, linyunsheng, toke
From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: Fri, 16 Feb 2024 10:25:43 +0100
> Use global percpu page_pool_recycle_stats counter for system page_pool
> allocator instead of allocating a separate percpu variable for each
> (also percpu) page pool instance.
>
> Reviewed-by: Toke Hoiland-Jorgensen <toke@redhat.com>
Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
> include/net/page_pool/types.h | 5 +++--
> net/core/dev.c | 1 +
> net/core/page_pool.c | 22 +++++++++++++++++-----
> 3 files changed, 21 insertions(+), 7 deletions(-)
Thanks,
Olek
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: page_pool: fix recycle stats for system page_pool allocator
2024-02-16 9:25 [PATCH net-next] net: page_pool: fix recycle stats for system page_pool allocator Lorenzo Bianconi
2024-02-16 10:32 ` Alexander Lobakin
@ 2024-02-17 3:46 ` Yunsheng Lin
2024-02-17 11:00 ` Lorenzo Bianconi
2024-02-19 20:50 ` patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Yunsheng Lin @ 2024-02-17 3:46 UTC (permalink / raw)
To: Lorenzo Bianconi, netdev
Cc: lorenzo.bianconi, kuba, davem, edumazet, pabeni, hawk,
ilias.apalodimas, toke, aleksander.lobakin
On 2024/2/16 17:25, Lorenzo Bianconi wrote:
> Use global percpu page_pool_recycle_stats counter for system page_pool
> allocator instead of allocating a separate percpu variable for each
> (also percpu) page pool instance.
I may missed some obvious discussion in previous version due to spring
holiday.
>
> Reviewed-by: Toke Hoiland-Jorgensen <toke@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
> include/net/page_pool/types.h | 5 +++--
> net/core/dev.c | 1 +
> net/core/page_pool.c | 22 +++++++++++++++++-----
> 3 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
> index 3828396ae60c..2515cca6518b 100644
> --- a/include/net/page_pool/types.h
> +++ b/include/net/page_pool/types.h
> @@ -18,8 +18,9 @@
> * Please note DMA-sync-for-CPU is still
> * device driver responsibility
> */
> -#define PP_FLAG_ALL (PP_FLAG_DMA_MAP |\
> - PP_FLAG_DMA_SYNC_DEV)
> +#define PP_FLAG_SYSTEM_POOL BIT(2) /* Global system page_pool */
> +#define PP_FLAG_ALL (PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV | \
> + PP_FLAG_SYSTEM_POOL)
>
> /*
> * Fast allocation side cache array/stack
> diff --git a/net/core/dev.c b/net/core/dev.c
> index cc9c2eda65ac..c588808be77f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -11738,6 +11738,7 @@ static int net_page_pool_create(int cpuid)
> #if IS_ENABLED(CONFIG_PAGE_POOL)
> struct page_pool_params page_pool_params = {
> .pool_size = SYSTEM_PERCPU_PAGE_POOL_SIZE,
> + .flags = PP_FLAG_SYSTEM_POOL,
> .nid = NUMA_NO_NODE,
> };
> struct page_pool *pp_ptr;
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 89c835fcf094..8f0c4e76181b 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -31,6 +31,8 @@
> #define BIAS_MAX (LONG_MAX >> 1)
>
> #ifdef CONFIG_PAGE_POOL_STATS
> +static DEFINE_PER_CPU(struct page_pool_recycle_stats, pp_system_recycle_stats);
> +
> /* alloc_stat_inc is intended to be used in softirq context */
> #define alloc_stat_inc(pool, __stat) (pool->alloc_stats.__stat++)
> /* recycle_stat_inc is safe to use when preemption is possible. */
> @@ -220,14 +222,23 @@ static int page_pool_init(struct page_pool *pool,
> pool->has_init_callback = !!pool->slow.init_callback;
>
> #ifdef CONFIG_PAGE_POOL_STATS
> - pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
> - if (!pool->recycle_stats)
> - return -ENOMEM;
> + if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL)) {
> + pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
> + if (!pool->recycle_stats)
> + return -ENOMEM;
> + } else {
> + /* For system page pool instance we use a singular stats object
> + * instead of allocating a separate percpu variable for each
> + * (also percpu) page pool instance.
> + */
> + pool->recycle_stats = &pp_system_recycle_stats;
Do we need to return -EINVAL here if page_pool_init() is called with
pool->p.flags & PP_FLAG_SYSTEM_POOL being true and cpuid being a valid
cpu?
If yes, it seems we may be able to use the cpuid to decide if we need
to allocate a new pool->recycle_stats without adding a new flag.
If no, the API for page_pool_create_percpu() seems a litte weird as it
relies on the user calling it correctly.
Also, do we need to enforce that page_pool_create_percpu() is only called
once for the same cpu? if no, we may have two page_pool instance sharing
the same stats.
> + }
> #endif
>
> if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0) {
> #ifdef CONFIG_PAGE_POOL_STATS
> - free_percpu(pool->recycle_stats);
> + if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL))
> + free_percpu(pool->recycle_stats);
> #endif
> return -ENOMEM;
> }
> @@ -251,7 +262,8 @@ static void page_pool_uninit(struct page_pool *pool)
> put_device(pool->p.dev);
>
> #ifdef CONFIG_PAGE_POOL_STATS
> - free_percpu(pool->recycle_stats);
> + if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL))
> + free_percpu(pool->recycle_stats);
> #endif
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: page_pool: fix recycle stats for system page_pool allocator
2024-02-17 3:46 ` Yunsheng Lin
@ 2024-02-17 11:00 ` Lorenzo Bianconi
2024-02-18 3:49 ` Yunsheng Lin
0 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Bianconi @ 2024-02-17 11:00 UTC (permalink / raw)
To: Yunsheng Lin
Cc: Lorenzo Bianconi, netdev, kuba, davem, edumazet, pabeni, hawk,
ilias.apalodimas, toke, aleksander.lobakin
[-- Attachment #1: Type: text/plain, Size: 2544 bytes --]
> On 2024/2/16 17:25, Lorenzo Bianconi wrote:
> > Use global percpu page_pool_recycle_stats counter for system page_pool
> > allocator instead of allocating a separate percpu variable for each
> > (also percpu) page pool instance.
>
> I may missed some obvious discussion in previous version due to spring
> holiday.
>
> >
[...]
> > #ifdef CONFIG_PAGE_POOL_STATS
> > - pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
> > - if (!pool->recycle_stats)
> > - return -ENOMEM;
> > + if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL)) {
> > + pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
> > + if (!pool->recycle_stats)
> > + return -ENOMEM;
> > + } else {
> > + /* For system page pool instance we use a singular stats object
> > + * instead of allocating a separate percpu variable for each
> > + * (also percpu) page pool instance.
> > + */
> > + pool->recycle_stats = &pp_system_recycle_stats;
>
> Do we need to return -EINVAL here if page_pool_init() is called with
> pool->p.flags & PP_FLAG_SYSTEM_POOL being true and cpuid being a valid
> cpu?
> If yes, it seems we may be able to use the cpuid to decide if we need
> to allocate a new pool->recycle_stats without adding a new flag.
for the current use-cases cpuid is set to a valid core id just for system
page_pool but in the future probably there will not be a 1:1 relation (e.g.
we would want to pin a per-cpu page_pool instance to a particular CPU?)
>
> If no, the API for page_pool_create_percpu() seems a litte weird as it
> relies on the user calling it correctly.
>
> Also, do we need to enforce that page_pool_create_percpu() is only called
> once for the same cpu? if no, we may have two page_pool instance sharing
> the same stats.
do you mean for the same pp instance? I guess it is not allowed by the APIs.
Regards,
Lorenzo
>
> > + }
> > #endif
> >
> > if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0) {
> > #ifdef CONFIG_PAGE_POOL_STATS
> > - free_percpu(pool->recycle_stats);
> > + if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL))
> > + free_percpu(pool->recycle_stats);
> > #endif
> > return -ENOMEM;
> > }
> > @@ -251,7 +262,8 @@ static void page_pool_uninit(struct page_pool *pool)
> > put_device(pool->p.dev);
> >
> > #ifdef CONFIG_PAGE_POOL_STATS
> > - free_percpu(pool->recycle_stats);
> > + if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL))
> > + free_percpu(pool->recycle_stats);
> > #endif
> > }
> >
> >
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: page_pool: fix recycle stats for system page_pool allocator
2024-02-17 11:00 ` Lorenzo Bianconi
@ 2024-02-18 3:49 ` Yunsheng Lin
0 siblings, 0 replies; 6+ messages in thread
From: Yunsheng Lin @ 2024-02-18 3:49 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Lorenzo Bianconi, netdev, kuba, davem, edumazet, pabeni, hawk,
ilias.apalodimas, toke, aleksander.lobakin
On 2024/2/17 19:00, Lorenzo Bianconi wrote:
>>> #ifdef CONFIG_PAGE_POOL_STATS
>>> - pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
>>> - if (!pool->recycle_stats)
>>> - return -ENOMEM;
>>> + if (!(pool->p.flags & PP_FLAG_SYSTEM_POOL)) {
>>> + pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
>>> + if (!pool->recycle_stats)
>>> + return -ENOMEM;
>>> + } else {
>>> + /* For system page pool instance we use a singular stats object
>>> + * instead of allocating a separate percpu variable for each
>>> + * (also percpu) page pool instance.
>>> + */
>>> + pool->recycle_stats = &pp_system_recycle_stats;
>>
>> Do we need to return -EINVAL here if page_pool_init() is called with
>> pool->p.flags & PP_FLAG_SYSTEM_POOL being true and cpuid being a valid
>> cpu?
My fault, the above "cpuid being a valid cpu" should be "cpuid not being a
valid cpu".
In other word, do we need to protect user from calling page_pool_init()
with PP_FLAG_SYSTEM_POOL flag and cpuid being -1?
>> If yes, it seems we may be able to use the cpuid to decide if we need
>> to allocate a new pool->recycle_stats without adding a new flag.
>
> for the current use-cases cpuid is set to a valid core id just for system
> page_pool but in the future probably there will not be a 1:1 relation (e.g.
> we would want to pin a per-cpu page_pool instance to a particular CPU?)
if it is a per-cpu page_pool instance, doesn't it run into the similar
problem this patch is trying to fix?
>
>>
>> If no, the API for page_pool_create_percpu() seems a litte weird as it
>> relies on the user calling it correctly.
>>
>> Also, do we need to enforce that page_pool_create_percpu() is only called
>> once for the same cpu? if no, we may have two page_pool instance sharing
>> the same stats.
>
> do you mean for the same pp instance? I guess it is not allowed by the APIs.
As above comment, if the user is passing a valid cpuid, the PP_FLAG_SYSTEM_POOL
flag need to be set too? if yes, doesn't the new flag seems somewhat redundant
here?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: page_pool: fix recycle stats for system page_pool allocator
2024-02-16 9:25 [PATCH net-next] net: page_pool: fix recycle stats for system page_pool allocator Lorenzo Bianconi
2024-02-16 10:32 ` Alexander Lobakin
2024-02-17 3:46 ` Yunsheng Lin
@ 2024-02-19 20:50 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-02-19 20:50 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: netdev, lorenzo.bianconi, kuba, davem, edumazet, pabeni, hawk,
ilias.apalodimas, linyunsheng, toke, aleksander.lobakin
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Fri, 16 Feb 2024 10:25:43 +0100 you wrote:
> Use global percpu page_pool_recycle_stats counter for system page_pool
> allocator instead of allocating a separate percpu variable for each
> (also percpu) page pool instance.
>
> Reviewed-by: Toke Hoiland-Jorgensen <toke@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>
> [...]
Here is the summary with links:
- [net-next] net: page_pool: fix recycle stats for system page_pool allocator
https://git.kernel.org/netdev/net-next/c/f853fa5c54e7
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-02-19 20:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-16 9:25 [PATCH net-next] net: page_pool: fix recycle stats for system page_pool allocator Lorenzo Bianconi
2024-02-16 10:32 ` Alexander Lobakin
2024-02-17 3:46 ` Yunsheng Lin
2024-02-17 11:00 ` Lorenzo Bianconi
2024-02-18 3:49 ` Yunsheng Lin
2024-02-19 20:50 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.