| Message ID | 87f572425e98faea3da45f76c3c68815c01a20ee.1708075412.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f853fa5c54e7a0364a52125074dedeaf2c7ddace |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: page_pool: fix recycle stats for system page_pool allocator | expand |
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
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 > } > >
> 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 > > } > > > > >
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?
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!
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 }