| Message ID | e56d630a7a6e8f738989745a2fa081225735a93c.1707933960.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] net: page_pool: fix recycle stats for percpu page_pool allocator | expand |
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
> 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
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
> 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
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.
> 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
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 }
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(-)