[RFC net-next] net: page_pool: fix recycle stats for percpu page_pool allocator

[RFC net-next] net: page_pool: fix recycle stats for percpu page_pool allocator

[Lorenzo Bianconi]

Use global page_pool_recycle_stats percpu counter for percpu page_pool
allocator.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 net/core/page_pool.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 6e0753e6a95b..1bb83b6e7a61 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_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,19 @@ 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 (cpuid < 0) {
+		pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
+		if (!pool->recycle_stats)
+			return -ENOMEM;
+	} else {
+		pool->recycle_stats = &pp_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 (cpuid < 0)
+			free_percpu(pool->recycle_stats);
 #endif
 		return -ENOMEM;
 	}
@@ -251,7 +258,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->cpuid < 0)
+		free_percpu(pool->recycle_stats);
 #endif
 }
 
-- 
2.43.0

Re: [RFC net-next] net: page_pool: fix recycle stats for percpu page_pool allocator

[Toke Høiland-Jørgensen]

Lorenzo Bianconi <lorenzo@kernel.org> writes:

> Use global page_pool_recycle_stats percpu counter for percpu page_pool
> allocator.
>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Neat trick with just referencing the pointer to the global object inside
the page_pool. With just a few nits below:

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>

> ---
>  net/core/page_pool.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 6e0753e6a95b..1bb83b6e7a61 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_recycle_stats);

Should we call this pp_system_recycle_stats to be consistent with the
naming of the other global variable?

>  /* 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,19 @@ 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 (cpuid < 0) {
> +		pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
> +		if (!pool->recycle_stats)
> +			return -ENOMEM;
> +	} else {

Maybe add a short comment here to explain what's going on? Something
like:

/* When a cpuid is supplied, we're initialising the percpu system page pool
 * instance, so use a singular stats object instead of allocating a
 * separate percpu variable for each (also percpu) page pool instance.
 */

-Toke

Re: [RFC net-next] net: page_pool: fix recycle stats for percpu page_pool allocator

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2098 bytes --]

> Lorenzo Bianconi <lorenzo@kernel.org> writes:
> 
> > Use global page_pool_recycle_stats percpu counter for percpu page_pool
> > allocator.
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> Neat trick with just referencing the pointer to the global object inside
> the page_pool. With just a few nits below:
> 
> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> > ---
> >  net/core/page_pool.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index 6e0753e6a95b..1bb83b6e7a61 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_recycle_stats);
> 
> Should we call this pp_system_recycle_stats to be consistent with the
> naming of the other global variable?

ack, I agree.

> 
> >  /* 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,19 @@ 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 (cpuid < 0) {
> > +		pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
> > +		if (!pool->recycle_stats)
> > +			return -ENOMEM;
> > +	} else {
> 
> Maybe add a short comment here to explain what's going on? Something
> like:
> 
> /* When a cpuid is supplied, we're initialising the percpu system page pool
>  * instance, so use a singular stats object instead of allocating a
>  * separate percpu variable for each (also percpu) page pool instance.
>  */
> 
> -Toke
> 

sure, I will add it.

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC net-next] net: page_pool: fix recycle stats for percpu page_pool allocator

[Alexander Lobakin]

From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: Wed, 14 Feb 2024 19:08:40 +0100

> Use global page_pool_recycle_stats percpu counter for percpu page_pool
> allocator.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  net/core/page_pool.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 6e0753e6a95b..1bb83b6e7a61 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_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,19 @@ 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 (cpuid < 0) {

TBH I don't like the idea of assuming that only system page_pools might
be created with cpuid >= 0.
For example, if I have an Rx queue always pinned to one CPU, I might
want to create a PP for this queue with the cpuid set already to save
some cycles when recycling. We might also reuse cpuid later for some
more optimizations or features.

Maybe add a new PP_FLAG indicating that system percpu PP stats should be
used?

> +		pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
> +		if (!pool->recycle_stats)
> +			return -ENOMEM;
> +	} else {
> +		pool->recycle_stats = &pp_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 (cpuid < 0)
> +			free_percpu(pool->recycle_stats);
>  #endif
>  		return -ENOMEM;
>  	}
> @@ -251,7 +258,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->cpuid < 0)
> +		free_percpu(pool->recycle_stats);
>  #endif
>  }
>  

Thanks,
Olek

Re: [RFC net-next] net: page_pool: fix recycle stats for percpu page_pool allocator

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2749 bytes --]

> From: Lorenzo Bianconi <lorenzo@kernel.org>
> Date: Wed, 14 Feb 2024 19:08:40 +0100
> 
> > Use global page_pool_recycle_stats percpu counter for percpu page_pool
> > allocator.
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  net/core/page_pool.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index 6e0753e6a95b..1bb83b6e7a61 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_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,19 @@ 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 (cpuid < 0) {
> 
> TBH I don't like the idea of assuming that only system page_pools might
> be created with cpuid >= 0.
> For example, if I have an Rx queue always pinned to one CPU, I might
> want to create a PP for this queue with the cpuid set already to save
> some cycles when recycling. We might also reuse cpuid later for some
> more optimizations or features.
> 
> Maybe add a new PP_FLAG indicating that system percpu PP stats should be
> used?

Ack, I like the idea. What about creating a flag to indicate this is a percpu
page_pool instead of relying on cpuid value? Maybe it can be useful in the
future, what do you think?

Regards,
Lorenzo

> 
> > +		pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
> > +		if (!pool->recycle_stats)
> > +			return -ENOMEM;
> > +	} else {
> > +		pool->recycle_stats = &pp_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 (cpuid < 0)
> > +			free_percpu(pool->recycle_stats);
> >  #endif
> >  		return -ENOMEM;
> >  	}
> > @@ -251,7 +258,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->cpuid < 0)
> > +		free_percpu(pool->recycle_stats);
> >  #endif
> >  }
> >  
> 
> Thanks,
> Olek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC net-next] net: page_pool: fix recycle stats for percpu page_pool allocator

[Jakub Kicinski]

On Thu, 15 Feb 2024 14:41:52 +0100 Alexander Lobakin wrote:
> For example, if I have an Rx queue always pinned to one CPU, I might
> want to create a PP for this queue with the cpuid set already to save
> some cycles when recycling. We might also reuse cpuid later for some
> more optimizations or features.

You say "pin Rx queue to one CPU" like that's actually possible to do
reliably :)

> Maybe add a new PP_FLAG indicating that system percpu PP stats should be
> used?

Part of me feels like checking the dev pointer would be good enough.
It may make sense to create more per CPU pools for particular devices
further down, but creating more pools without no dev / DMA mapping
makes no sense, right?

Dunno if looking at dev is not too hacky, tho, flags are cheap.

Re: [RFC net-next] net: page_pool: fix recycle stats for percpu page_pool allocator

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 885 bytes --]

> On Thu, 15 Feb 2024 14:41:52 +0100 Alexander Lobakin wrote:
> > For example, if I have an Rx queue always pinned to one CPU, I might
> > want to create a PP for this queue with the cpuid set already to save
> > some cycles when recycling. We might also reuse cpuid later for some
> > more optimizations or features.
> 
> You say "pin Rx queue to one CPU" like that's actually possible to do
> reliably :)
> 
> > Maybe add a new PP_FLAG indicating that system percpu PP stats should be
> > used?
> 
> Part of me feels like checking the dev pointer would be good enough.
> It may make sense to create more per CPU pools for particular devices
> further down, but creating more pools without no dev / DMA mapping
> makes no sense, right?
> 
> Dunno if looking at dev is not too hacky, tho, flags are cheap.

I would vote for a dedicated flag ;)

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]