| Message ID | 9f0a571c1f322ff6c4e6facfd7d6d508e73a8f2f.1706451150.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add multi-buff support for xdp running in generic mode | expand |
On 2024/1/28 22:20, Lorenzo Bianconi wrote: > Move page_pool stats allocation in page_pool_create routine and get rid > of it for percpu page_pools. Is there any reason why we do not need those kind stats for per cpu page_pool?
> On 2024/1/28 22:20, Lorenzo Bianconi wrote: > > Move page_pool stats allocation in page_pool_create routine and get rid > > of it for percpu page_pools. > > Is there any reason why we do not need those kind stats for per cpu > page_pool? > IIRC discussing with Jakub, we decided to not support them since the pool is not associated to any net_device in this case. Regards, Lorenzo
On 2024/1/29 21:07, Lorenzo Bianconi wrote: >> On 2024/1/28 22:20, Lorenzo Bianconi wrote: >>> Move page_pool stats allocation in page_pool_create routine and get rid >>> of it for percpu page_pools. >> >> Is there any reason why we do not need those kind stats for per cpu >> page_pool? >> > > IIRC discussing with Jakub, we decided to not support them since the pool is not > associated to any net_device in this case. It seems what jakub suggested is to 'extend netlink to dump unbound page pools'? > > Regards, > Lorenzo >
> On 2024/1/29 21:07, Lorenzo Bianconi wrote: > >> On 2024/1/28 22:20, Lorenzo Bianconi wrote: > >>> Move page_pool stats allocation in page_pool_create routine and get rid > >>> of it for percpu page_pools. > >> > >> Is there any reason why we do not need those kind stats for per cpu > >> page_pool? > >> > > > > IIRC discussing with Jakub, we decided to not support them since the pool is not > > associated to any net_device in this case. > > It seems what jakub suggested is to 'extend netlink to dump unbound page pools'? I do not have a strong opinion about it (since we do not have any use-case for it at the moment). In the case we want to support stats for per-cpu page_pools, I think we should not create a per-cpu recycle_stats pointer and add a page_pool_recycle_stats field in page_pool struct since otherwise we will endup with ncpu^2 copies, right? Do we want to support it now? @Jakub, Jesper: what do you guys think? Regards, Lorenzo > > > > > Regards, > > Lorenzo > > >
On 30/01/2024 14.52, Lorenzo Bianconi wrote: >> On 2024/1/29 21:07, Lorenzo Bianconi wrote: >>>> On 2024/1/28 22:20, Lorenzo Bianconi wrote: >>>>> Move page_pool stats allocation in page_pool_create routine and get rid >>>>> of it for percpu page_pools. >>>> >>>> Is there any reason why we do not need those kind stats for per cpu >>>> page_pool? >>>> >>> >>> IIRC discussing with Jakub, we decided to not support them since the pool is not >>> associated to any net_device in this case. >> >> It seems what jakub suggested is to 'extend netlink to dump unbound page pools'? > > I do not have a strong opinion about it (since we do not have any use-case for > it at the moment). > In the case we want to support stats for per-cpu page_pools, I think we should > not create a per-cpu recycle_stats pointer and add a page_pool_recycle_stats field > in page_pool struct since otherwise we will endup with ncpu^2 copies, right? > Do we want to support it now? > > @Jakub, Jesper: what do you guys think? > I do see an need for being able to access page_pool stats for all page_pool's in the system. And I do like Jakub's netlink based stats. --Jesper (p.s. I'm debugging some production issues with page_pool and broadcom bnxt_en driver).
> > > On 30/01/2024 14.52, Lorenzo Bianconi wrote: > > > On 2024/1/29 21:07, Lorenzo Bianconi wrote: > > > > > On 2024/1/28 22:20, Lorenzo Bianconi wrote: > > > > > > Move page_pool stats allocation in page_pool_create routine and get rid > > > > > > of it for percpu page_pools. > > > > > > > > > > Is there any reason why we do not need those kind stats for per cpu > > > > > page_pool? > > > > > > > > > > > > > IIRC discussing with Jakub, we decided to not support them since the pool is not > > > > associated to any net_device in this case. > > > > > > It seems what jakub suggested is to 'extend netlink to dump unbound page pools'? > > > > I do not have a strong opinion about it (since we do not have any use-case for > > it at the moment). > > In the case we want to support stats for per-cpu page_pools, I think we should > > not create a per-cpu recycle_stats pointer and add a page_pool_recycle_stats field > > in page_pool struct since otherwise we will endup with ncpu^2 copies, right? > > Do we want to support it now? > > > > @Jakub, Jesper: what do you guys think? > > > > > I do see an need for being able to access page_pool stats for all > page_pool's in the system. > And I do like Jakub's netlink based stats. ack from my side if you have some use-cases in mind. Some questions below: - can we assume ethtool will be used to report stats just for 'global' page_pool (not per-cpu page_pool)? - can we assume netlink/yaml will be used to report per-cpu page_pool stats? I think in the current series we can fix the accounting part (in particular avoiding memory wasting) and then we will figure out how to report percpu page_pool stats through netlink/yaml. Agree? Regards, Lorenzo > > --Jesper > (p.s. I'm debugging some production issues with page_pool and broadcom > bnxt_en driver). >
Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes: >> >> >> On 30/01/2024 14.52, Lorenzo Bianconi wrote: >> > > On 2024/1/29 21:07, Lorenzo Bianconi wrote: >> > > > > On 2024/1/28 22:20, Lorenzo Bianconi wrote: >> > > > > > Move page_pool stats allocation in page_pool_create routine and get rid >> > > > > > of it for percpu page_pools. >> > > > > >> > > > > Is there any reason why we do not need those kind stats for per cpu >> > > > > page_pool? >> > > > > >> > > > >> > > > IIRC discussing with Jakub, we decided to not support them since the pool is not >> > > > associated to any net_device in this case. >> > > >> > > It seems what jakub suggested is to 'extend netlink to dump unbound page pools'? >> > >> > I do not have a strong opinion about it (since we do not have any use-case for >> > it at the moment). >> > In the case we want to support stats for per-cpu page_pools, I think we should >> > not create a per-cpu recycle_stats pointer and add a page_pool_recycle_stats field >> > in page_pool struct since otherwise we will endup with ncpu^2 copies, right? >> > Do we want to support it now? >> > >> > @Jakub, Jesper: what do you guys think? >> > >> >> >> I do see an need for being able to access page_pool stats for all >> page_pool's in the system. >> And I do like Jakub's netlink based stats. > > ack from my side if you have some use-cases in mind. > Some questions below: > - can we assume ethtool will be used to report stats just for 'global' > page_pool (not per-cpu page_pool)? > - can we assume netlink/yaml will be used to report per-cpu page_pool stats? > > I think in the current series we can fix the accounting part (in particular > avoiding memory wasting) and then we will figure out how to report percpu > page_pool stats through netlink/yaml. Agree? Deferring the export API to a separate series after this is merged is fine with me. In which case the *gathering* of statistics could also be deferred (it's not really useful if it can't be exported). -Toke
On Wed, 31 Jan 2024 16:32:00 +0100 Toke Høiland-Jørgensen wrote: > > ack from my side if you have some use-cases in mind. > > Some questions below: > > - can we assume ethtool will be used to report stats just for 'global' > > page_pool (not per-cpu page_pool)? > > - can we assume netlink/yaml will be used to report per-cpu page_pool stats? > > > > I think in the current series we can fix the accounting part (in particular > > avoiding memory wasting) and then we will figure out how to report percpu > > page_pool stats through netlink/yaml. Agree? > > Deferring the export API to a separate series after this is merged is > fine with me. +1 > In which case the *gathering* of statistics could also be > deferred (it's not really useful if it can't be exported). What do you mean by "gather" here? If we plan to expose them later on I reckon there's no point having this patch which actively optimizes them away, no? IOW we should just drop this patch from v7?
> On Wed, 31 Jan 2024 16:32:00 +0100 Toke Høiland-Jørgensen wrote: > > > ack from my side if you have some use-cases in mind. > > > Some questions below: > > > - can we assume ethtool will be used to report stats just for 'global' > > > page_pool (not per-cpu page_pool)? > > > - can we assume netlink/yaml will be used to report per-cpu page_pool stats? > > > > > > I think in the current series we can fix the accounting part (in particular > > > avoiding memory wasting) and then we will figure out how to report percpu > > > page_pool stats through netlink/yaml. Agree? > > > > Deferring the export API to a separate series after this is merged is > > fine with me. > > +1 > > > In which case the *gathering* of statistics could also be > > deferred (it's not really useful if it can't be exported). > > What do you mean by "gather" here? If we plan to expose them later on > I reckon there's no point having this patch which actively optimizes > them away, no? IOW we should just drop this patch from v7? ack, I will get rid of it in v7. Regards, Lorenzo
diff --git a/net/core/page_pool.c b/net/core/page_pool.c index 89c835fcf094..5278ffef6442 100644 --- a/net/core/page_pool.c +++ b/net/core/page_pool.c @@ -37,13 +37,15 @@ #define recycle_stat_inc(pool, __stat) \ do { \ struct page_pool_recycle_stats __percpu *s = pool->recycle_stats; \ - this_cpu_inc(s->__stat); \ + if (s) \ + this_cpu_inc(s->__stat); \ } while (0) #define recycle_stat_add(pool, __stat, val) \ do { \ struct page_pool_recycle_stats __percpu *s = pool->recycle_stats; \ - this_cpu_add(s->__stat, val); \ + if (s) \ + this_cpu_add(s->__stat, val); \ } while (0) static const char pp_stats[][ETH_GSTRING_LEN] = { @@ -79,6 +81,9 @@ bool page_pool_get_stats(const struct page_pool *pool, if (!stats) return false; + if (!pool->recycle_stats) + return false; + /* The caller is responsible to initialize stats. */ stats->alloc_stats.fast += pool->alloc_stats.fast; stats->alloc_stats.slow += pool->alloc_stats.slow; @@ -218,19 +223,8 @@ 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) + if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0) return -ENOMEM; -#endif - - if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0) { -#ifdef CONFIG_PAGE_POOL_STATS - free_percpu(pool->recycle_stats); -#endif - return -ENOMEM; - } atomic_set(&pool->pages_state_release_cnt, 0); @@ -295,7 +289,21 @@ EXPORT_SYMBOL(page_pool_create_percpu); */ struct page_pool *page_pool_create(const struct page_pool_params *params) { - return page_pool_create_percpu(params, -1); + struct page_pool *pool; + + pool = page_pool_create_percpu(params, -1); + if (IS_ERR(pool)) + return pool; + +#ifdef CONFIG_PAGE_POOL_STATS + pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats); + if (!pool->recycle_stats) { + page_pool_uninit(pool); + kfree(pool); + pool = ERR_PTR(-ENOMEM); + } +#endif + return pool; } EXPORT_SYMBOL(page_pool_create);
Move page_pool stats allocation in page_pool_create routine and get rid of it for percpu page_pools. Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- net/core/page_pool.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)