| Message ID | 628c0a6d9bdbc547c93fcd4ae2e84d08af7bc8e1.1649528984.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mvneta: add support for page_pool_get_stats | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h > index ea5fb70e5101..94b2d666db03 100644 > --- a/include/net/page_pool.h > +++ b/include/net/page_pool.h > @@ -117,6 +117,10 @@ struct page_pool_stats { > struct page_pool_recycle_stats recycle_stats; > }; > > +int page_pool_ethtool_stats_get_count(void); > +u8 *page_pool_ethtool_stats_get_strings(u8 *data); > +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats); > + > /* > * Drivers that wish to harvest page pool stats and report them to users > * (perhaps via ethtool, debugfs, or another mechanism) can allocate a You could also add stub function here for when the page pool statistics are disabled. We can then avoid all the messy #ifdef in the drivers. > +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { > + *data++ = stats->alloc_stats.fast; > + *data++ = stats->alloc_stats.slow; > + *data++ = stats->alloc_stats.slow_high_order; > + *data++ = stats->alloc_stats.empty; > + *data++ = stats->alloc_stats.refill; > + *data++ = stats->alloc_stats.waive; > + *data++ = stats->recycle_stats.cached; > + *data++ = stats->recycle_stats.cache_full; > + *data++ = stats->recycle_stats.ring; > + *data++ = stats->recycle_stats.ring_full; > + *data++ = stats->recycle_stats.released_refcnt; > + } > + > + return data; What is the purpose of the loop? Andrew
Hi Lorenzo, [...] > > for_each_possible_cpu(cpu) { > const struct page_pool_recycle_stats *pcpu = > @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool, > return true; > } > EXPORT_SYMBOL(page_pool_get_stats); > + > +u8 *page_pool_ethtool_stats_get_strings(u8 *data) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { > + memcpy(data, pp_stats[i], ETH_GSTRING_LEN); > + data += ETH_GSTRING_LEN; > + } > + > + return data; Is there a point returning data here or can we make this a void? > +} > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_strings); > + > +int page_pool_ethtool_stats_get_count(void) > +{ > + return ARRAY_SIZE(pp_stats); > +} > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_count); > + > +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { > + *data++ = stats->alloc_stats.fast; > + *data++ = stats->alloc_stats.slow; > + *data++ = stats->alloc_stats.slow_high_order; > + *data++ = stats->alloc_stats.empty; > + *data++ = stats->alloc_stats.refill; > + *data++ = stats->alloc_stats.waive; > + *data++ = stats->recycle_stats.cached; > + *data++ = stats->recycle_stats.cache_full; > + *data++ = stats->recycle_stats.ring; > + *data++ = stats->recycle_stats.ring_full; > + *data++ = stats->recycle_stats.released_refcnt; > + } > + > + return data; Ditto > +} > +EXPORT_SYMBOL(page_pool_ethtool_stats_get); > #else > #define alloc_stat_inc(pool, __stat) > #define recycle_stat_inc(pool, __stat) > -- > 2.35.1 > Thanks /Ilias
> > diff --git a/include/net/page_pool.h b/include/net/page_pool.h > > index ea5fb70e5101..94b2d666db03 100644 > > --- a/include/net/page_pool.h > > +++ b/include/net/page_pool.h > > @@ -117,6 +117,10 @@ struct page_pool_stats { > > struct page_pool_recycle_stats recycle_stats; > > }; > > > > +int page_pool_ethtool_stats_get_count(void); > > +u8 *page_pool_ethtool_stats_get_strings(u8 *data); > > +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats); > > + > > /* > > * Drivers that wish to harvest page pool stats and report them to users > > * (perhaps via ethtool, debugfs, or another mechanism) can allocate a > > You could also add stub function here for when the page pool > statistics are disabled. We can then avoid all the messy #ifdef in the > drivers. > > > +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { > > + *data++ = stats->alloc_stats.fast; > > + *data++ = stats->alloc_stats.slow; > > + *data++ = stats->alloc_stats.slow_high_order; > > + *data++ = stats->alloc_stats.empty; > > + *data++ = stats->alloc_stats.refill; > > + *data++ = stats->alloc_stats.waive; > > + *data++ = stats->recycle_stats.cached; > > + *data++ = stats->recycle_stats.cache_full; > > + *data++ = stats->recycle_stats.ring; > > + *data++ = stats->recycle_stats.ring_full; > > + *data++ = stats->recycle_stats.released_refcnt; > > + } > > + > > + return data; > > What is the purpose of the loop? ops sorry, you are right. I will fix it. Regards, Lorenzo > > Andrew
> Hi Lorenzo, Hi Ilias, > > [...] > > > > > for_each_possible_cpu(cpu) { > > const struct page_pool_recycle_stats *pcpu = > > @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool, > > return true; > > } > > EXPORT_SYMBOL(page_pool_get_stats); > > + > > +u8 *page_pool_ethtool_stats_get_strings(u8 *data) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { > > + memcpy(data, pp_stats[i], ETH_GSTRING_LEN); > > + data += ETH_GSTRING_LEN; > > + } > > + > > + return data; > > Is there a point returning data here or can we make this a void? it is to add the capability to add more strings in the driver code after running page_pool_ethtool_stats_get_strings. > > > +} > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_strings); > > + > > +int page_pool_ethtool_stats_get_count(void) > > +{ > > + return ARRAY_SIZE(pp_stats); > > +} > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_count); > > + > > +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { > > + *data++ = stats->alloc_stats.fast; > > + *data++ = stats->alloc_stats.slow; > > + *data++ = stats->alloc_stats.slow_high_order; > > + *data++ = stats->alloc_stats.empty; > > + *data++ = stats->alloc_stats.refill; > > + *data++ = stats->alloc_stats.waive; > > + *data++ = stats->recycle_stats.cached; > > + *data++ = stats->recycle_stats.cache_full; > > + *data++ = stats->recycle_stats.ring; > > + *data++ = stats->recycle_stats.ring_full; > > + *data++ = stats->recycle_stats.released_refcnt; > > + } > > + > > + return data; > > Ditto same here. Regards, Lorenzo > > > +} > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get); > > #else > > #define alloc_stat_inc(pool, __stat) > > #define recycle_stat_inc(pool, __stat) > > -- > > 2.35.1 > > > > Thanks > /Ilias
On Mon, 11 Apr 2022 at 15:28, Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > Hi Lorenzo, > > Hi Ilias, > > > > > [...] > > > > > > > > for_each_possible_cpu(cpu) { > > > const struct page_pool_recycle_stats *pcpu = > > > @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool, > > > return true; > > > } > > > EXPORT_SYMBOL(page_pool_get_stats); > > > + > > > +u8 *page_pool_ethtool_stats_get_strings(u8 *data) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { > > > + memcpy(data, pp_stats[i], ETH_GSTRING_LEN); > > > + data += ETH_GSTRING_LEN; > > > + } > > > + > > > + return data; > > > > Is there a point returning data here or can we make this a void? > > it is to add the capability to add more strings in the driver code after > running page_pool_ethtool_stats_get_strings. But the current driver isn't using it. I don't have too much experience with how drivers consume ethtool stats, but would it make more sense to return a length instead of a pointer? Maybe Andrew has an idea. Thanks /Ilias > > > > > > +} > > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_strings); > > > + > > > +int page_pool_ethtool_stats_get_count(void) > > > +{ > > > + return ARRAY_SIZE(pp_stats); > > > +} > > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_count); > > > + > > > +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { > > > + *data++ = stats->alloc_stats.fast; > > > + *data++ = stats->alloc_stats.slow; > > > + *data++ = stats->alloc_stats.slow_high_order; > > > + *data++ = stats->alloc_stats.empty; > > > + *data++ = stats->alloc_stats.refill; > > > + *data++ = stats->alloc_stats.waive; > > > + *data++ = stats->recycle_stats.cached; > > > + *data++ = stats->recycle_stats.cache_full; > > > + *data++ = stats->recycle_stats.ring; > > > + *data++ = stats->recycle_stats.ring_full; > > > + *data++ = stats->recycle_stats.released_refcnt; > > > + } > > > + > > > + return data; > > > > Ditto > > same here. > > Regards, > Lorenzo > > > > > > +} > > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get); > > > #else > > > #define alloc_stat_inc(pool, __stat) > > > #define recycle_stat_inc(pool, __stat) > > > -- > > > 2.35.1 > > > > > > > Thanks > > /Ilias
On Apr 11, Ilias Apalodimas wrote: > On Mon, 11 Apr 2022 at 15:28, Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > > Hi Lorenzo, > > > > Hi Ilias, > > > > > > > > [...] > > > > > > > > > > > for_each_possible_cpu(cpu) { > > > > const struct page_pool_recycle_stats *pcpu = > > > > @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool, > > > > return true; > > > > } > > > > EXPORT_SYMBOL(page_pool_get_stats); > > > > + > > > > +u8 *page_pool_ethtool_stats_get_strings(u8 *data) > > > > +{ > > > > + int i; > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { > > > > + memcpy(data, pp_stats[i], ETH_GSTRING_LEN); > > > > + data += ETH_GSTRING_LEN; > > > > + } > > > > + > > > > + return data; > > > > > > Is there a point returning data here or can we make this a void? > > > > it is to add the capability to add more strings in the driver code after > > running page_pool_ethtool_stats_get_strings. > > But the current driver isn't using it. I don't have too much > experience with how drivers consume ethtool stats, but would it make > more sense to return a length instead of a pointer? Maybe Andrew has > an idea. yes, but it is for future usage. Returning a pointer modified by a local routine is a common approach in the kernel (not sure about ethtool) and it seems straightforward to me but len is fine as well. Opinions? Regards, Lorenzo > > Thanks > /Ilias > > > > > > > > > +} > > > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_strings); > > > > + > > > > +int page_pool_ethtool_stats_get_count(void) > > > > +{ > > > > + return ARRAY_SIZE(pp_stats); > > > > +} > > > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_count); > > > > + > > > > +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats) > > > > +{ > > > > + int i; > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { > > > > + *data++ = stats->alloc_stats.fast; > > > > + *data++ = stats->alloc_stats.slow; > > > > + *data++ = stats->alloc_stats.slow_high_order; > > > > + *data++ = stats->alloc_stats.empty; > > > > + *data++ = stats->alloc_stats.refill; > > > > + *data++ = stats->alloc_stats.waive; > > > > + *data++ = stats->recycle_stats.cached; > > > > + *data++ = stats->recycle_stats.cache_full; > > > > + *data++ = stats->recycle_stats.ring; > > > > + *data++ = stats->recycle_stats.ring_full; > > > > + *data++ = stats->recycle_stats.released_refcnt; > > > > + } > > > > + > > > > + return data; > > > > > > Ditto > > > > same here. > > > > Regards, > > Lorenzo > > > > > > > > > +} > > > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get); > > > > #else > > > > #define alloc_stat_inc(pool, __stat) > > > > #define recycle_stat_inc(pool, __stat) > > > > -- > > > > 2.35.1 > > > > > > > > > > Thanks > > > /Ilias
On Mon, Apr 11, 2022 at 03:34:21PM +0300, Ilias Apalodimas wrote: > On Mon, 11 Apr 2022 at 15:28, Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > > Hi Lorenzo, > > > > Hi Ilias, > > > > > > > > [...] > > > > > > > > > > > for_each_possible_cpu(cpu) { > > > > const struct page_pool_recycle_stats *pcpu = > > > > @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool, > > > > return true; > > > > } > > > > EXPORT_SYMBOL(page_pool_get_stats); > > > > + > > > > +u8 *page_pool_ethtool_stats_get_strings(u8 *data) > > > > +{ > > > > + int i; > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { > > > > + memcpy(data, pp_stats[i], ETH_GSTRING_LEN); > > > > + data += ETH_GSTRING_LEN; > > > > + } > > > > + > > > > + return data; > > > > > > Is there a point returning data here or can we make this a void? > > > > it is to add the capability to add more strings in the driver code after > > running page_pool_ethtool_stats_get_strings. > > But the current driver isn't using it. It could be you need it for the mlx5 driver, which puts the TLS counters after the page pool counters. Or you could just move them to the end. I don't think the order of statistics are ABI, just the strings themselves.. > I don't have too much > experience with how drivers consume ethtool stats, but would it make > more sense to return a length instead of a pointer? Maybe Andrew has > an idea. Either is acceptable. Even if you do make it a void, the driver can use the stats_get_count() and do the maths. But a length or a pointer is simpler. Andrew
Hi Andrew, On Mon, 11 Apr 2022 at 15:49, Andrew Lunn <andrew@lunn.ch> wrote: > > On Mon, Apr 11, 2022 at 03:34:21PM +0300, Ilias Apalodimas wrote: > > On Mon, 11 Apr 2022 at 15:28, Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > > > > Hi Lorenzo, > > > > > > Hi Ilias, > > > > > > > > > > > [...] > > > > > > > > > > > > > > for_each_possible_cpu(cpu) { > > > > > const struct page_pool_recycle_stats *pcpu = > > > > > @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool, > > > > > return true; > > > > > } > > > > > EXPORT_SYMBOL(page_pool_get_stats); > > > > > + > > > > > +u8 *page_pool_ethtool_stats_get_strings(u8 *data) > > > > > +{ > > > > > + int i; > > > > > + > > > > > + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { > > > > > + memcpy(data, pp_stats[i], ETH_GSTRING_LEN); > > > > > + data += ETH_GSTRING_LEN; > > > > > + } > > > > > + > > > > > + return data; > > > > > > > > Is there a point returning data here or can we make this a void? > > > > > > it is to add the capability to add more strings in the driver code after > > > running page_pool_ethtool_stats_get_strings. > > > > But the current driver isn't using it. > > It could be you need it for the mlx5 driver, which puts the TLS > counters after the page pool counters. Or you could just move them to > the end. I don't think the order of statistics are ABI, just the > strings themselves.. > > > I don't have too much > > experience with how drivers consume ethtool stats, but would it make > > more sense to return a length instead of a pointer? Maybe Andrew has > > an idea. > > Either is acceptable. Even if you do make it a void, the driver can > use the stats_get_count() and do the maths. But a length or a pointer > is simpler. Thanks, I am fine with either as well. I was just wondering why we need it since the mvneta driver wasn't using it. Let's leave it as is /Ilias > > Andrew
diff --git a/include/net/page_pool.h b/include/net/page_pool.h index ea5fb70e5101..94b2d666db03 100644 --- a/include/net/page_pool.h +++ b/include/net/page_pool.h @@ -117,6 +117,10 @@ struct page_pool_stats { struct page_pool_recycle_stats recycle_stats; }; +int page_pool_ethtool_stats_get_count(void); +u8 *page_pool_ethtool_stats_get_strings(u8 *data); +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats); + /* * Drivers that wish to harvest page pool stats and report them to users * (perhaps via ethtool, debugfs, or another mechanism) can allocate a diff --git a/net/core/page_pool.c b/net/core/page_pool.c index 4af55d28ffa3..e82b94040ed1 100644 --- a/net/core/page_pool.c +++ b/net/core/page_pool.c @@ -18,6 +18,7 @@ #include <linux/page-flags.h> #include <linux/mm.h> /* for __put_page() */ #include <linux/poison.h> +#include <linux/ethtool.h> #include <trace/events/page_pool.h> @@ -42,6 +43,20 @@ this_cpu_add(s->__stat, val); \ } while (0) +static const char pp_stats[][ETH_GSTRING_LEN] = { + "rx_pp_alloc_fast", + "rx_pp_alloc_slow", + "rx_pp_alloc_slow_ho", + "rx_pp_alloc_empty", + "rx_pp_alloc_refill", + "rx_pp_alloc_waive", + "rx_pp_recycle_cached", + "rx_pp_recycle_cache_full", + "rx_pp_recycle_ring", + "rx_pp_recycle_ring_full", + "rx_pp_recycle_released_ref", +}; + bool page_pool_get_stats(struct page_pool *pool, struct page_pool_stats *stats) { @@ -50,7 +65,13 @@ bool page_pool_get_stats(struct page_pool *pool, if (!stats) return false; - memcpy(&stats->alloc_stats, &pool->alloc_stats, sizeof(pool->alloc_stats)); + /* The caller is responsible to initialize stats. */ + stats->alloc_stats.fast += pool->alloc_stats.fast; + stats->alloc_stats.slow += pool->alloc_stats.slow; + stats->alloc_stats.slow_high_order += pool->alloc_stats.slow_high_order; + stats->alloc_stats.empty += pool->alloc_stats.empty; + stats->alloc_stats.refill += pool->alloc_stats.refill; + stats->alloc_stats.waive += pool->alloc_stats.waive; for_each_possible_cpu(cpu) { const struct page_pool_recycle_stats *pcpu = @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool, return true; } EXPORT_SYMBOL(page_pool_get_stats); + +u8 *page_pool_ethtool_stats_get_strings(u8 *data) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { + memcpy(data, pp_stats[i], ETH_GSTRING_LEN); + data += ETH_GSTRING_LEN; + } + + return data; +} +EXPORT_SYMBOL(page_pool_ethtool_stats_get_strings); + +int page_pool_ethtool_stats_get_count(void) +{ + return ARRAY_SIZE(pp_stats); +} +EXPORT_SYMBOL(page_pool_ethtool_stats_get_count); + +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(pp_stats); i++) { + *data++ = stats->alloc_stats.fast; + *data++ = stats->alloc_stats.slow; + *data++ = stats->alloc_stats.slow_high_order; + *data++ = stats->alloc_stats.empty; + *data++ = stats->alloc_stats.refill; + *data++ = stats->alloc_stats.waive; + *data++ = stats->recycle_stats.cached; + *data++ = stats->recycle_stats.cache_full; + *data++ = stats->recycle_stats.ring; + *data++ = stats->recycle_stats.ring_full; + *data++ = stats->recycle_stats.released_refcnt; + } + + return data; +} +EXPORT_SYMBOL(page_pool_ethtool_stats_get); #else #define alloc_stat_inc(pool, __stat) #define recycle_stat_inc(pool, __stat)
Introduce page_pool APIs to report stats through ethtool and reduce duplicated code in each driver. Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- include/net/page_pool.h | 4 +++ net/core/page_pool.c | 64 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 1 deletion(-)