net: mvneta: add support for page_pool_get_stats
Subsystems (4)
v3 cover net: mvneta: add support for page_pool_get_stats
Introduce page_pool stats ethtool APIs in order to avoid driver duplicated code.
Changes since v2:
- remove enum list of page_pool stats in page_pool.h
- remove leftover change in mvneta.c for ethtool_stats array allocation
Changes since v1:
- move stats accounting to page_pool code
- move stats string management to page_pool code
Lorenzo Bianconi (2):
net: page_pool: introduce ethtool stats
net: mvneta: add support for page_pool_get_stats
drivers/net/ethernet/marvell/Kconfig | 1 + drivers/net/ethernet/marvell/mvneta.c | 20 ++++++++- include/net/page_pool.h | 4 ++ net/core/page_pool.c | 64 ++++++++++++++++++++++++++- 4 files changed, 87 insertions(+), 2 deletions(-)
Signature · 1 line
2.35.1
PATCH v3 1/2 net: page_pool: introduce ethtool stats
Introduce page_pool APIs to report stats through ethtool and reduce duplicated code in each driver.
--- include/net/page_pool.h | 4 +++ net/core/page_pool.c | 64 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 1 deletion(-)
include/net/page_pool.h +4 -0
@@ -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
net/core/page_pool.c +63 -1
88 lines not on this page · raw message
Signature · 1 line
2.35.1
Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats
Quoted 5 lines
include/net/page_pool.h +0
-0
include/net/page_pool.h struct page_pool_stats { · line 126
Quoted 10 lines
include/net/page_pool.h +4
-0@@ -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.
Quoted 9 lines
include/net/page_pool.h +9
-0+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;
include/net/page_pool.h
Quoted 10 lines
include/net/page_pool.h +10
-0+ *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
Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats
Quoted 33 lines
Quoted 5 lines
include/net/page_pool.h +0
-0include/net/page_pool.h struct page_pool_stats { · line 126
Quoted 10 lines
include/net/page_pool.h +4
-0@@ -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.
Quoted 13 lines
include/net/page_pool.h +13
-0+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;
Quoted 8 lines
include/net/page_pool.h
Quoted 6 lines
include/net/page_pool.h +6
-0+ *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
Quoted 2 lines
Andrew
Ilias Apalodimas
maintainerRe: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats
In reply to Lorenzo BianconiHi Lorenzo,
[...]
Quoted 8 lines
for_each_possible_cpu(cpu){
const structpage_pool_recycle_stats*pcpu =net/core/page_pool.c +1
-0@@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool,return true; }EXPORT_SYMBOL (page_pool_get_stats); +
net/core/page_pool.c bool page_pool_get_stats(struct page_pool *pool, · line 100
Quoted 10 lines
net/core/page_pool.c +10
-0+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?
Quoted 18 lines
net/core/page_pool.c +18
-0+} +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;
net/core/page_pool.c bool page_pool_get_stats(struct page_pool *pool, · line 128
Quoted 10 lines
net/core/page_pool.c +10
-0+ *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
Quoted 8 lines
net/core/page_pool.c +2
-1+} +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
Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats
Quoted 1 line
Hi Lorenzo,
Hi Ilias,
Quoted 15 lines
[...]
Quoted 12 lines
for_each_possible_cpu(cpu){
const structpage_pool_recycle_stats*pcpu =net/core/page_pool.c +5
-0@@ -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; +
Quoted 8 lines
net/core/page_pool.c bool page_pool_get_stats(struct page_pool *pool, · line 100
Quoted 6 lines
net/core/page_pool.c +6
-0+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.
Quoted 23 lines
Quoted 23 lines
net/core/page_pool.c +22
-0+} +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;
Quoted 8 lines
net/core/page_pool.c bool page_pool_get_stats(struct page_pool *pool, · line 128
Quoted 6 lines
net/core/page_pool.c +6
-0+ *data++ = stats->recycle_stats.ring; + *data++ = stats->recycle_stats.ring_full; + *data++ = stats->recycle_stats.released_refcnt; + } + +return data;Ditto
same here.
Regards,
Lorenzo
Quoted 12 lines
Quoted 9 lines
net/core/page_pool.c +2
-1+} +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
Ilias Apalodimas
maintainerRe: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats
Quoted 29 lines · Lorenzo Bianconi
Quoted 2 lines
Hi Lorenzo,
Hi Ilias,
Quoted 15 lines
[...]
Quoted 12 lines
for_each_possible_cpu(cpu){
const structpage_pool_recycle_stats*pcpu =net/core/page_pool.c +5
-0@@ -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; +Quoted 8 lines
net/core/page_pool.c bool page_pool_get_stats(struct page_pool *pool, · line 100
Quoted 6 lines
net/core/page_pool.c +6
-0+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?
Quoted 2 lines
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
Quoted 50 lines
Quoted 24 lines
Quoted 24 lines
net/core/page_pool.c +22
-0+} +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;Quoted 8 lines
net/core/page_pool.c bool page_pool_get_stats(struct page_pool *pool, · line 128
Quoted 6 lines
net/core/page_pool.c +6
-0+ *data++ = stats->recycle_stats.ring; + *data++ = stats->recycle_stats.ring_full; + *data++ = stats->recycle_stats.released_refcnt; + } + +return data;Ditto
same here.
Regards,
LorenzoQuoted 12 lines
Quoted 9 lines
net/core/page_pool.c +2
-1+} +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
Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats
Quoted 30 lines · Ilias Apalodimas
Quoted 29 lines · Lorenzo Bianconi
Quoted 2 lines
Hi Lorenzo,
Hi Ilias,
Quoted 23 lines
[...]
Quoted 8 lines
for_each_possible_cpu(cpu){
const structpage_pool_recycle_stats*pcpu =net/core/page_pool.c +1
-0@@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool,return true; }EXPORT_SYMBOL (page_pool_get_stats); +net/core/page_pool.c bool page_pool_get_stats(struct page_pool *pool, · line 100
Quoted 10 lines
net/core/page_pool.c +10
-0+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?
Quoted 7 lines
Quoted 2 lines
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
Quoted 53 lines
Thanks
/IliasQuoted 50 lines
Quoted 24 lines
Quoted 24 lines
net/core/page_pool.c +22
-0+} +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;Quoted 8 lines
net/core/page_pool.c bool page_pool_get_stats(struct page_pool *pool, · line 128
Quoted 6 lines
net/core/page_pool.c +6
-0+ *data++ = stats->recycle_stats.ring; + *data++ = stats->recycle_stats.ring_full; + *data++ = stats->recycle_stats.released_refcnt; + } + +return data;Ditto
same here.
Regards,
LorenzoQuoted 12 lines
Quoted 9 lines
net/core/page_pool.c +2
-1+} +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
Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats
In reply to Ilias ApalodimasQuoted 30 lines · Ilias Apalodimas
Quoted 29 lines · Lorenzo Bianconi
Quoted 2 lines
Hi Lorenzo,
Hi Ilias,
Quoted 23 lines
[...]
Quoted 8 lines
for_each_possible_cpu(cpu){
const structpage_pool_recycle_stats*pcpu =net/core/page_pool.c +1
-0@@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool,return true; }EXPORT_SYMBOL (page_pool_get_stats); +net/core/page_pool.c bool page_pool_get_stats(struct page_pool *pool, · line 100
Quoted 10 lines
net/core/page_pool.c +10
-0+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?
Quoted 4 lines
Quoted 2 lines
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..
Quoted 4 lines
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
Ilias Apalodimas
maintainerRe: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats
Hi Andrew,
Quoted 42 lines · Andrew Lunn
Quoted 30 lines · Ilias Apalodimas
Quoted 29 lines · Lorenzo Bianconi
Quoted 2 lines
Hi Lorenzo,
Hi Ilias,
Quoted 23 lines
[...]
Quoted 8 lines
for_each_possible_cpu(cpu){
const structpage_pool_recycle_stats*pcpu =net/core/page_pool.c +1
-0@@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool,return true; }EXPORT_SYMBOL (page_pool_get_stats); +net/core/page_pool.c bool page_pool_get_stats(struct page_pool *pool, · line 100
Quoted 10 lines
net/core/page_pool.c +10
-0+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?
Quoted 4 lines
Quoted 2 lines
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..
Quoted 8 lines
Quoted 4 lines
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
Quoted 2 lines
Andrew
PATCH v3 2/2 net: mvneta: add support for page_pool_get_stats
In reply to Lorenzo BianconiIntroduce support for the page_pool_get_stats API to mvneta driver Report page_pool stats through ethtool.
--- drivers/net/ethernet/marvell/Kconfig | 1 + drivers/net/ethernet/marvell/mvneta.c | 20 +++++++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-)
drivers/net/ethernet/marvell/Kconfig +1 -0
@@ -62,6 +62,7 @@ config MVNETA select MVMDIO select PHYLINK select PAGE_POOL + select PAGE_POOL_STATS help This driver supports the network interface units in the Marvell ARMADA XP, ARMADA 370, ARMADA 38x and
drivers/net/ethernet/marvell/mvneta.c +19 -1
@@ -4735,6 +4735,9 @@ static void mvneta_ethtool_get_strings(struct net_device *netdev, u32 sset,for (i =0 ; i <ARRAY_SIZE (mvneta_statistics); i++)memcpy (data + i *ETH_GSTRING_LEN , mvneta_statistics[i].name,ETH_GSTRING_LEN ); + + data +=ETH_GSTRING_LEN *ARRAY_SIZE (mvneta_statistics); +page_pool_ethtool_stats_get_strings (data); } }
@@ -4847,6 +4850,17 @@ static void mvneta_ethtool_update_stats(struct mvneta_port *pp) } } +static void mvneta_ethtool_pp_stats (struct mvneta_port *pp,u64 *data) +{ +struct page_pool_stats stats = {}; +int i; + +for (i =0 ; i < rxq_number; i++) +page_pool_get_stats (pp->rxqs[i].page_pool, &stats); + +page_pool_ethtool_stats_get (data, &stats); +} +static void mvneta_ethtool_get_stats (struct net_device *dev,struct ethtool_stats *stats,u64 *data) {
@@ -4857,12 +4871,16 @@ static void mvneta_ethtool_get_stats(struct net_device *dev,for (i =0 ; i <ARRAY_SIZE (mvneta_statistics); i++) *data++ = pp->ethtool_stats[i]; + +mvneta_ethtool_pp_stats (pp, data); }static int mvneta_ethtool_get_sset_count (struct net_device *dev,int sset) {if (sset ==ETH_SS_STATS ) -return ARRAY_SIZE (mvneta_statistics); +return ARRAY_SIZE (mvneta_statistics) + +page_pool_ethtool_stats_get_count (); +return -EOPNOTSUPP ; }
Signature · 1 line
2.35.1
Quoted 4 lines · Lorenzo Bianconi
Introduce support for the
page_pool_get_statsAPI to mvneta driver Reportpage_poolstats through ethtool.Signed-off-by Lorenzo Bianconi <[email protected]>
Andrew