Add page_pool_stats ethtool APIs
Subsystems (4)
cover Add page_pool_stats ethtool APIs
Introduce page_pool_stats support to mvneta driver.
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 | 59 ++++++++++++++-- include/net/page_pool.h | 24 +++++++ net/core/page_pool.c | 96 +++++++++++++++++++++++++++ 4 files changed, 175 insertions(+), 5 deletions(-)
Signature · 1 line
2.35.1
RFC 1/2 net: page_pool: introduce ethtool stats
Introduce APIs to report page_pool stats through ethtool and reduce duplicated code in each driver.
--- include/net/page_pool.h | 24 +++++++++++ net/core/page_pool.c | 96 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+)
include/net/page_pool.h +24 -0
@@ -117,6 +117,30 @@ struct page_pool_stats {struct page_pool_recycle_stats recycle_stats; }; +/* List of page_pool stats exported through ethtool. */ +enum { +PP_ETHTOOL_ALLOC_FAST , +PP_ETHTOOL_ALLOC_SLOW , +PP_ETHTOOL_ALLOC_SLOW_HIGH_ORDER , +PP_ETHTOOL_ALLOC_EMPTY , +PP_ETHTOOL_ALLOC_REFILL , +PP_ETHTOOL_ALLOC_WAIVE , +PP_ETHTOOL_RECYCLE_CACHED , +PP_ETHTOOL_RECYCLE_CACHE_FULL , +PP_ETHTOOL_RECYCLE_RING , +PP_ETHTOOL_RECYCLE_RING_FULL , +PP_ETHTOOL_RECYCLE_RELEASED_REF , +PP_ETHTOOL_STATS_MAX , +}; + +static inline int page_pool_ethtool_stats_get_count (void ) +{ +return PP_ETHTOOL_STATS_MAX ; +} + +void page_pool_ethtool_stats_get_strings (u8 *data); +void 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 +96 -0
108 lines not on this page · raw message
Signature · 1 line
2.35.1
Jakub Kicinski
maintainerRe: [RFC net-next 1/2] net: page_pool: introduce ethtool stats
Quoted 1 line · On Thu, 7 Apr 2022 18:55:04 +0200 Lorenzo Bianconi wrote:
+void
page_pool_ethtool_stats_get_strings(u8 *data)
This needs to return the pointer to after the used area, so drivers can append more stats. Or make data double pointer and update it before returning.
net/core/page_pool.c bool page_pool_get_stats(struct page_pool *pool, · line 78
Quoted 7 lines
net/core/page_pool.c +7
-0+{ +static const struct { +u8 type; +char name[ETH_GSTRING_LEN ]; + } stats[PP_ETHTOOL_STATS_MAX ] = { + { + .type =PP_ETHTOOL_ALLOC_FAST ,
Why enumerate the type? It's not used.
Quoted 39 lines
net/core/page_pool.c +39
-0+ .name = "rx_pp_alloc_fast" + }, { + .type = PP_ETHTOOL_ALLOC_SLOW, + .name ="rx_pp_alloc_slow" + }, { + .type = PP_ETHTOOL_ALLOC_SLOW_HIGH_ORDER, + .name ="rx_pp_alloc_slow_ho" + }, { + .type = PP_ETHTOOL_ALLOC_EMPTY, + .name ="rx_pp_alloc_empty" + }, { + .type = PP_ETHTOOL_ALLOC_REFILL, + .name ="rx_pp_alloc_refill" + }, { + .type = PP_ETHTOOL_ALLOC_WAIVE, + .name ="rx_pp_alloc_waive" + }, { + .type = PP_ETHTOOL_RECYCLE_CACHED, + .name ="rx_pp_recycle_cached" + }, { + .type = PP_ETHTOOL_RECYCLE_CACHE_FULL, + .name ="rx_pp_recycle_cache_full" + }, { + .type = PP_ETHTOOL_RECYCLE_RING, + .name ="rx_pp_recycle_ring" + }, { + .type = PP_ETHTOOL_RECYCLE_RING_FULL, + .name ="rx_pp_recycle_ring_full" + }, { + .type = PP_ETHTOOL_RECYCLE_RELEASED_REF, + .name ="rx_pp_recycle_released_ref" + }, + }; +int i; + +for (i =0 ; i <PP_ETHTOOL_STATS_MAX ; i++) { +memcpy (data, stats[i].name,ETH_GSTRING_LEN ); + data +=ETH_GSTRING_LEN ; + }
Re: [RFC net-next 1/2] net: page_pool: introduce ethtool stats
Quoted 6 lines · Jakub Kicinski
Quoted 1 line · On Thu, 7 Apr 2022 18:55:04 +0200 Lorenzo Bianconi wrote:
+void
page_pool_ethtool_stats_get_strings(u8 *data)This needs to return the pointer to after the used area, so drivers can append more stats. Or make data double pointer and update it before returning.
ack, I will fix it in v2.
Quoted 10 lines
net/core/page_pool.c bool page_pool_get_stats(struct page_pool *pool, · line 78
Quoted 8 lines
net/core/page_pool.c +7
-0+{ +static const struct { +u8 type; +char name[ETH_GSTRING_LEN ]; + } stats[PP_ETHTOOL_STATS_MAX ] = { + { + .type =PP_ETHTOOL_ALLOC_FAST ,Why enumerate the type? It's not used.
ack, I will fix it in v2.
Regards,
Lorenzo
Quoted 40 lines
Quoted 40 lines
net/core/page_pool.c +39
-0+ .name = "rx_pp_alloc_fast" + }, { + .type = PP_ETHTOOL_ALLOC_SLOW, + .name ="rx_pp_alloc_slow" + }, { + .type = PP_ETHTOOL_ALLOC_SLOW_HIGH_ORDER, + .name ="rx_pp_alloc_slow_ho" + }, { + .type = PP_ETHTOOL_ALLOC_EMPTY, + .name ="rx_pp_alloc_empty" + }, { + .type = PP_ETHTOOL_ALLOC_REFILL, + .name ="rx_pp_alloc_refill" + }, { + .type = PP_ETHTOOL_ALLOC_WAIVE, + .name ="rx_pp_alloc_waive" + }, { + .type = PP_ETHTOOL_RECYCLE_CACHED, + .name ="rx_pp_recycle_cached" + }, { + .type = PP_ETHTOOL_RECYCLE_CACHE_FULL, + .name ="rx_pp_recycle_cache_full" + }, { + .type = PP_ETHTOOL_RECYCLE_RING, + .name ="rx_pp_recycle_ring" + }, { + .type = PP_ETHTOOL_RECYCLE_RING_FULL, + .name ="rx_pp_recycle_ring_full" + }, { + .type = PP_ETHTOOL_RECYCLE_RELEASED_REF, + .name ="rx_pp_recycle_released_ref" + }, + }; +int i; + +for (i =0 ; i <PP_ETHTOOL_STATS_MAX ; i++) { +memcpy (data, stats[i].name,ETH_GSTRING_LEN ); + data +=ETH_GSTRING_LEN ; + }
RFC 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 | 59 ++++++++++++++++++++++++--- 2 files changed, 55 insertions(+), 5 deletions(-)
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 +54 -5
100 lines not on this page · raw message
Signature · 1 line
2.35.1
Re: [RFC net-next 2/2] net: mvneta: add support for page_pool_get_stats
Quoted 17 lines
drivers/net/ethernet/marvell/mvneta.c +17
-0+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++) { +struct page_pool *page_pool = pp->rxqs[i].page_pool; +struct page_pool_stats pp_stats = {}; + +if (!page_pool_get_stats (page_pool, &pp_stats)) +continue ; + + stats.alloc_stats.fast += pp_stats.alloc_stats.fast; + stats.alloc_stats.slow += pp_stats.alloc_stats.slow; + stats.alloc_stats.slow_high_order += + pp_stats.alloc_stats.slow_high_order; + stats.alloc_stats.empty += pp_stats.alloc_stats.empty;
drivers/net/ethernet/marvell/mvneta.c static void mvneta_ethtool_update_stats(struct mvneta_port *pp) · line 4880
Quoted 10 lines
drivers/net/ethernet/marvell/mvneta.c +10
-0+ stats.alloc_stats.refill += pp_stats.alloc_stats.refill; + stats.alloc_stats.waive += pp_stats.alloc_stats.waive; + stats.recycle_stats.cached += pp_stats.recycle_stats.cached; + stats.recycle_stats.cache_full += + pp_stats.recycle_stats.cache_full; + stats.recycle_stats.ring += pp_stats.recycle_stats.ring; + stats.recycle_stats.ring_full += + pp_stats.recycle_stats.ring_full; + stats.recycle_stats.released_refcnt += + pp_stats.recycle_stats.released_refcnt;
We should be trying to remove this sort of code from the driver, and put it all in the core. It wants to be something more like:
struct page_pool_stats stats = {};
int i;
for (i = 0; i < rxq_number; i++) {
struct page_pool *page_pool = pp->rxqs[i].page_pool;
if (!page_pool_get_stats(page_pool, &stats))
continue;
page_pool_ethtool_stats_get(data, &stats);Let page_pool_get_stats() do the accumulate as it puts values in stats.
You probably should also rework the mellanox driver to use the same code structure.
Andrew
Ilias Apalodimas
maintainerRe: [RFC net-next 2/2] net: mvneta: add support for page_pool_get_stats
Hi Andrew,
Quoted 43 lines · Andrew Lunn
Quoted 18 lines
drivers/net/ethernet/marvell/mvneta.c +17
-0+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++) { +struct page_pool *page_pool = pp->rxqs[i].page_pool; +struct page_pool_stats pp_stats = {}; + +if (!page_pool_get_stats (page_pool, &pp_stats)) +continue ; + + stats.alloc_stats.fast += pp_stats.alloc_stats.fast; + stats.alloc_stats.slow += pp_stats.alloc_stats.slow; + stats.alloc_stats.slow_high_order += + pp_stats.alloc_stats.slow_high_order; + stats.alloc_stats.empty += pp_stats.alloc_stats.empty;drivers/net/ethernet/marvell/mvneta.c static void mvneta_ethtool_update_stats(struct mvneta_port *pp) · line 4880
Quoted 10 lines
drivers/net/ethernet/marvell/mvneta.c +10
-0+ stats.alloc_stats.refill += pp_stats.alloc_stats.refill; + stats.alloc_stats.waive += pp_stats.alloc_stats.waive; + stats.recycle_stats.cached += pp_stats.recycle_stats.cached; + stats.recycle_stats.cache_full += + pp_stats.recycle_stats.cache_full; + stats.recycle_stats.ring += pp_stats.recycle_stats.ring; + stats.recycle_stats.ring_full += + pp_stats.recycle_stats.ring_full; + stats.recycle_stats.released_refcnt += + pp_stats.recycle_stats.released_refcnt;We should be trying to remove this sort of code from the driver, and put it all in the core. It wants to be something more like:
struct page_pool_stats stats = {}; int i; for (i = 0; i < rxq_number; i++) { struct page_pool *page_pool = pp->rxqs[i].page_pool; if (!page_pool_get_stats(page_pool, &stats)) continue; page_pool_ethtool_stats_get(data, &stats);
Quoted 1 line
Let
page_pool_get_stats()do the accumulate as it puts values in stats.
Unless I misunderstand this, I don't think that's doable in page pool. That means page pool is aware of what stats to accumulate per driver and I certainly don't want anything driver specific to creep in there. The driver knows the number of pools he is using and he can gather them all together.
Regards
/Ilias
Quoted 7 lines
You probably should also rework the mellanox driver to use the same code structure.
Andrew
Re: [RFC net-next 2/2] net: mvneta: add support for page_pool_get_stats
Quoted 46 lines · Ilias Apalodimas
Hi Andrew,
Quoted 43 lines · Andrew Lunn
Quoted 18 lines
drivers/net/ethernet/marvell/mvneta.c +17
-0+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++) { +struct page_pool *page_pool = pp->rxqs[i].page_pool; +struct page_pool_stats pp_stats = {}; + +if (!page_pool_get_stats (page_pool, &pp_stats)) +continue ; + + stats.alloc_stats.fast += pp_stats.alloc_stats.fast; + stats.alloc_stats.slow += pp_stats.alloc_stats.slow; + stats.alloc_stats.slow_high_order += + pp_stats.alloc_stats.slow_high_order; + stats.alloc_stats.empty += pp_stats.alloc_stats.empty;drivers/net/ethernet/marvell/mvneta.c static void mvneta_ethtool_update_stats(struct mvneta_port *pp) · line 4880
Quoted 10 lines
drivers/net/ethernet/marvell/mvneta.c +10
-0+ stats.alloc_stats.refill += pp_stats.alloc_stats.refill; + stats.alloc_stats.waive += pp_stats.alloc_stats.waive; + stats.recycle_stats.cached += pp_stats.recycle_stats.cached; + stats.recycle_stats.cache_full += + pp_stats.recycle_stats.cache_full; + stats.recycle_stats.ring += pp_stats.recycle_stats.ring; + stats.recycle_stats.ring_full += + pp_stats.recycle_stats.ring_full; + stats.recycle_stats.released_refcnt += + pp_stats.recycle_stats.released_refcnt;We should be trying to remove this sort of code from the driver, and put it all in the core. It wants to be something more like:
struct page_pool_stats stats = {}; int i; for (i = 0; i < rxq_number; i++) { struct page_pool *page_pool = pp->rxqs[i].page_pool; if (!page_pool_get_stats(page_pool, &stats)) continue; page_pool_ethtool_stats_get(data, &stats);
Quoted 7 lines
Quoted 1 line
Let
page_pool_get_stats()do the accumulate as it puts values in stats.Unless I misunderstand this, I don't think that's doable in page pool. That means page pool is aware of what stats to accumulate per driver and I certainly don't want anything driver specific to creep in there. The driver knows the number of pools he is using and he can gather them all together.
I agree that the driver knows about the number of pools. For mvneta, there is one per RX queue. Which is this part of my suggestion
Quoted 3 lines
Quoted 3 lines
for (i = 0; i < rxq_number; i++) { struct page_pool *page_pool = pp->rxqs[i].page_pool;
However, it has no idea about the stats themselves. They are purely a construct of the page pool. Hence the next part of my suggest, ask the page pool for the stats, place them into stats, doing the accumulate at the same time.:
Quoted 2 lines
Quoted 2 lines
if (!page_pool_get_stats(page_pool, &stats))
continue;
and now we have the accumulated stats, turn them into ethtool format:
Quoted 1 line
Quoted 1 line
page_pool_ethtool_stats_get(data, &stats);
Where do you see any driver knowledge required in either of page_pool_get_stats() or page_pool_ethtool_stats_get().
Andrew
Ilias Apalodimas
maintainerRe: [RFC net-next 2/2] net: mvneta: add support for page_pool_get_stats
Hi Andrew,
[...]
Quoted 54 lines
Quoted 31 lines
Quoted 31 lines
Quoted 6 lines
drivers/net/ethernet/marvell/mvneta.c +6
-0+ + stats.alloc_stats.fast += pp_stats.alloc_stats.fast; + stats.alloc_stats.slow += pp_stats.alloc_stats.slow; + stats.alloc_stats.slow_high_order += + pp_stats.alloc_stats.slow_high_order; + stats.alloc_stats.empty += pp_stats.alloc_stats.empty;drivers/net/ethernet/marvell/mvneta.c static void mvneta_ethtool_update_stats(struct mvneta_port *pp) · line 4880
Quoted 10 lines
drivers/net/ethernet/marvell/mvneta.c +10
-0+ stats.alloc_stats.refill += pp_stats.alloc_stats.refill; + stats.alloc_stats.waive += pp_stats.alloc_stats.waive; + stats.recycle_stats.cached += pp_stats.recycle_stats.cached; + stats.recycle_stats.cache_full += + pp_stats.recycle_stats.cache_full; + stats.recycle_stats.ring += pp_stats.recycle_stats.ring; + stats.recycle_stats.ring_full += + pp_stats.recycle_stats.ring_full; + stats.recycle_stats.released_refcnt += + pp_stats.recycle_stats.released_refcnt;We should be trying to remove this sort of code from the driver, and put it all in the core. It wants to be something more like:
struct page_pool_stats stats = {}; int i; for (i = 0; i < rxq_number; i++) { struct page_pool *page_pool = pp->rxqs[i].page_pool; if (!page_pool_get_stats(page_pool, &stats)) continue; page_pool_ethtool_stats_get(data, &stats);Quoted 7 lines
Quoted 1 line
Let
page_pool_get_stats()do the accumulate as it puts values in stats.Unless I misunderstand this, I don't think that's doable in page pool. That means page pool is aware of what stats to accumulate per driver and I certainly don't want anything driver specific to creep in there. The driver knows the number of pools he is using and he can gather them all together.
I agree that the driver knows about the number of pools. For mvneta, there is one per RX queue. Which is this part of my suggestion
Quoted 3 lines
Quoted 3 lines
for (i = 0; i < rxq_number; i++) { struct page_pool *page_pool = pp->rxqs[i].page_pool;However, it has no idea about the stats themselves. They are purely a construct of the page pool. Hence the next part of my suggest, ask the page pool for the stats, place them into stats, doing the accumulate at the same time.:
Quoted 2 lines
Quoted 2 lines
if (!page_pool_get_stats(page_pool, &stats))
continue;
Quoted 6 lines
and now we have the accumulated stats, turn them into ethtool format:
Quoted 1 line
Quoted 1 line
page_pool_ethtool_stats_get(data, &stats);Where do you see any driver knowledge required in either of
page_pool_get_stats()orpage_pool_ethtool_stats_get().
Indeed I read the first mail wrong. I thought you wanted page_pool it self to account for the driver stats without passing a 'struct page_pool *pool' to page_pool_get_stats(). In a system with XDP (which also uses page_pool) or multiple drivers using that, it would require some metadata fed into the page pool subsystem to reason about which pools to accumulate.
The code snip you included seems fine.
Thanks
/Ilias
Quoted 2 lines
Andrew