| Message ID | 20250305121420.kFO617zQ@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] net/mlnx5: Use generic code for page_pool statistics. | expand |
> @@ -276,6 +263,9 @@ static MLX5E_DECLARE_STATS_GRP_OP_FILL_STATS(sw) > mlx5e_ethtool_put_stat(data, > MLX5E_READ_CTR64_CPU(&priv->stats.sw, > sw_stats_desc, i)); > +#ifdef CONFIG_PAGE_POOL_STATS > + *data = page_pool_ethtool_stats_get(*data, &priv->stats.sw.page_pool_stats); > +#endif > } Are these #ifdef required? include/net/page_pool/helpers.h: static inline u64 *page_pool_ethtool_stats_get(u64 *data, const void *stats) { return data; } Seems silly to have a stub if it cannot be used. Andrew
On 2025-03-05 17:21:56 [+0100], Andrew Lunn wrote: > > @@ -276,6 +263,9 @@ static MLX5E_DECLARE_STATS_GRP_OP_FILL_STATS(sw) > > mlx5e_ethtool_put_stat(data, > > MLX5E_READ_CTR64_CPU(&priv->stats.sw, > > sw_stats_desc, i)); > > +#ifdef CONFIG_PAGE_POOL_STATS > > + *data = page_pool_ethtool_stats_get(*data, &priv->stats.sw.page_pool_stats); > > +#endif > > } > > Are these #ifdef required? include/net/page_pool/helpers.h: > > static inline u64 *page_pool_ethtool_stats_get(u64 *data, const void *stats) > { > return data; > } > > Seems silly to have a stub if it cannot be used. As I mentioned in the diffstat section, if we add the snippet below then it would work. Because the struct itself is not there. diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h index f45d55e6e8643..78984b9286c6b 100644 --- a/include/net/page_pool/types.h +++ b/include/net/page_pool/types.h @@ -143,10 +143,14 @@ struct page_pool_recycle_stats { */ struct page_pool_stats { struct page_pool_alloc_stats alloc_stats; struct page_pool_recycle_stats recycle_stats; }; + +#else /* !CONFIG_PAGE_POOL_STATS */ + +struct page_pool_stats { }; #endif /* The whole frag API block must stay within one cacheline. On 32-bit systems, * sizeof(long) == sizeof(int), so that the block size is ``3 * sizeof(long)``. * On 64-bit systems, the actual size is ``2 * sizeof(long) + sizeof(int)``. > Andrew Sebastian
On Wed, Mar 05, 2025 at 05:26:36PM +0100, Sebastian Andrzej Siewior wrote: > On 2025-03-05 17:21:56 [+0100], Andrew Lunn wrote: > > > @@ -276,6 +263,9 @@ static MLX5E_DECLARE_STATS_GRP_OP_FILL_STATS(sw) > > > mlx5e_ethtool_put_stat(data, > > > MLX5E_READ_CTR64_CPU(&priv->stats.sw, > > > sw_stats_desc, i)); > > > +#ifdef CONFIG_PAGE_POOL_STATS > > > + *data = page_pool_ethtool_stats_get(*data, &priv->stats.sw.page_pool_stats); > > > +#endif > > > } > > > > Are these #ifdef required? include/net/page_pool/helpers.h: > > > > static inline u64 *page_pool_ethtool_stats_get(u64 *data, const void *stats) > > { > > return data; > > } > > > > Seems silly to have a stub if it cannot be used. > > As I mentioned in the diffstat section, if we add the snippet below then > it would work. Because the struct itself is not there. > > diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h > index f45d55e6e8643..78984b9286c6b 100644 > --- a/include/net/page_pool/types.h > +++ b/include/net/page_pool/types.h > @@ -143,10 +143,14 @@ struct page_pool_recycle_stats { > */ > struct page_pool_stats { > struct page_pool_alloc_stats alloc_stats; > struct page_pool_recycle_stats recycle_stats; > }; > + > +#else /* !CONFIG_PAGE_POOL_STATS */ > + > +struct page_pool_stats { }; > #endif Please do add this, in a separate patch. But i wounder how other drivers handle this. Do they also have #ifdef? Andrew --- pw-bot: cr
On 2025-03-05 18:43:39 [+0100], Andrew Lunn wrote: > > diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h > > index f45d55e6e8643..78984b9286c6b 100644 > > --- a/include/net/page_pool/types.h > > +++ b/include/net/page_pool/types.h > > @@ -143,10 +143,14 @@ struct page_pool_recycle_stats { > > */ > > struct page_pool_stats { > > struct page_pool_alloc_stats alloc_stats; > > struct page_pool_recycle_stats recycle_stats; > > }; > > + > > +#else /* !CONFIG_PAGE_POOL_STATS */ > > + > > +struct page_pool_stats { }; > > #endif > > Please do add this, in a separate patch. But i wounder how other > drivers handle this. Do they also have #ifdef? veth.c, fec_main.c, yes. mvneta.c, mtk_eth_soc.c, no. But then they select PAGE_POOL_STATS so it is not exactly a fair comparison. > Andrew Sebastian
On 05/03/2025 14:14, Sebastian Andrzej Siewior wrote: > The statistics gathering code for page_pool statistics has multiple > steps: > - gather statistics from a channel via page_pool_get_stats() to an > on-stack structure. > - copy this data to dedicated rq_stats. > - copy the data from rq_stats global mlx5e_sw_stats structure, and merge > per-queue statistics into one counter. > - Finally copy the data the specific order for the ethtool query. > > The downside here is that the individual counter types are expected to > be u64 and if something changes, the code breaks. Also if additional > counter are added to struct page_pool_stats then they are not > automtically picked up by the driver but need to be manually added in > all four spots. > > Remove the page_pool_stats fields from rq_stats_desc and use instead > page_pool_ethtool_stats_get_count() for the number of files and > page_pool_ethtool_stats_get_strings() for the strings which are added at > the end. > Remove page_pool_stats members from all structs and add the struct to > mlx5e_sw_stats where the data is gathered directly for all channels. > At the end, use page_pool_ethtool_stats_get() to copy the data to the > output buffer. > > Suggested-by: Joe Damato <jdamato@fastly.com> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > Hi, Thanks for your patch. IIUC you remove here the per-ring page_pool stats, and keep only the summed stats. I guess the reason for this is that the page_pool strings have no per-ring variants. 59 static const char pp_stats[][ETH_GSTRING_LEN] = { 60 "rx_pp_alloc_fast", 61 "rx_pp_alloc_slow", 62 "rx_pp_alloc_slow_ho", 63 "rx_pp_alloc_empty", 64 "rx_pp_alloc_refill", 65 "rx_pp_alloc_waive", 66 "rx_pp_recycle_cached", 67 "rx_pp_recycle_cache_full", 68 "rx_pp_recycle_ring", 69 "rx_pp_recycle_ring_full", 70 "rx_pp_recycle_released_ref", 71 }; Is this the only reason? I like the direction of this patch, but we won't give up the per-ring counters. Please keep them. I can think of a new "customized page_pool counters strings" API, where the strings prefix is provided by the driver, and used to generate the per-pool strings. Example: Driver provides "rx5", and gets the strings: "rx5_pp_alloc_fast", "rx5_pp_alloc_slow", "rx5_pp_alloc_slow_ho", "rx5_pp_alloc_empty", "rx5_pp_alloc_refill", "rx5_pp_alloc_waive", "rx5_pp_recycle_cached", "rx5_pp_recycle_cache_full", "rx5_pp_recycle_ring", "rx5_pp_recycle_ring_full", "rx5_pp_recycle_released_ref", Alternatively, page_pool component provides the counters number and the "stripped" strings, and the driver takes it from there... "stripped" strings would be: "pp_alloc_fast", "pp_alloc_slow", "pp_alloc_slow_ho", "pp_alloc_empty", "pp_alloc_refill", "pp_alloc_waive", "pp_recycle_cached", "pp_recycle_cache_full", "pp_recycle_ring", "pp_recycle_ring_full", "pp_recycle_released_ref", or maybe even shorter: "alloc_fast", "alloc_slow", "alloc_slow_ho", "alloc_empty", "alloc_refill", "alloc_waive", "recycle_cached", "recycle_cache_full", "recycle_ring", "recycle_ring_full", "recycle_released_ref", Thanks, Tariq
On 2025-03-05 21:44:23 [+0200], Tariq Toukan wrote: > Hi, Hi, > Thanks for your patch. > > IIUC you remove here the per-ring page_pool stats, and keep only the summed > stats. > > I guess the reason for this is that the page_pool strings have no per-ring > variants. > > 59 static const char pp_stats[][ETH_GSTRING_LEN] = { > 60 "rx_pp_alloc_fast", > 61 "rx_pp_alloc_slow", > 62 "rx_pp_alloc_slow_ho", > 63 "rx_pp_alloc_empty", > 64 "rx_pp_alloc_refill", > 65 "rx_pp_alloc_waive", > 66 "rx_pp_recycle_cached", > 67 "rx_pp_recycle_cache_full", > 68 "rx_pp_recycle_ring", > 69 "rx_pp_recycle_ring_full", > 70 "rx_pp_recycle_released_ref", > 71 }; > > Is this the only reason? Yes. I haven't seen any reason to keep it. It is only copied around. > I like the direction of this patch, but we won't give up the per-ring > counters. Please keep them. Hmm. Okay. I guess I could stuff a struct there. But it really looks like waste since it is not used. > I can think of a new "customized page_pool counters strings" API, where the > strings prefix is provided by the driver, and used to generate the per-pool > strings. Okay. So I make room for it and you wire it up ;) Sebastian
On 05/03/2025 22:20, Sebastian Andrzej Siewior wrote: > On 2025-03-05 21:44:23 [+0200], Tariq Toukan wrote: >> Hi, > > Hi, > >> Thanks for your patch. >> >> IIUC you remove here the per-ring page_pool stats, and keep only the summed >> stats. >> >> I guess the reason for this is that the page_pool strings have no per-ring >> variants. >> >> 59 static const char pp_stats[][ETH_GSTRING_LEN] = { >> 60 "rx_pp_alloc_fast", >> 61 "rx_pp_alloc_slow", >> 62 "rx_pp_alloc_slow_ho", >> 63 "rx_pp_alloc_empty", >> 64 "rx_pp_alloc_refill", >> 65 "rx_pp_alloc_waive", >> 66 "rx_pp_recycle_cached", >> 67 "rx_pp_recycle_cache_full", >> 68 "rx_pp_recycle_ring", >> 69 "rx_pp_recycle_ring_full", >> 70 "rx_pp_recycle_released_ref", >> 71 }; >> >> Is this the only reason? > > Yes. I haven't seen any reason to keep it. It is only copied around. > >> I like the direction of this patch, but we won't give up the per-ring >> counters. Please keep them. > > Hmm. Okay. I guess I could stuff a struct there. But it really looks > like waste since it is not used. > Of course they are used. Per-ring (per-pool) counters are exposed via ethtool -S.
On 2025-03-06 08:49:55 [+0200], Tariq Toukan wrote: > > > I like the direction of this patch, but we won't give up the per-ring > > > counters. Please keep them. > > > > Hmm. Okay. I guess I could stuff a struct there. But it really looks > > like waste since it is not used. > > > > Of course they are used. > Per-ring (per-pool) counters are exposed via ethtool -S. That is true for some stats and they are prefixed with a number for the individual ring. In case of page_pool_stats they are not exposed individually. The individual page_pool_stats counters from each ring are merged into one counter and as exposed as one (not per ring). Sebastian
On 2025-03-05 21:20:57 [+0100], To Tariq Toukan wrote: > > I like the direction of this patch, but we won't give up the per-ring > > counters. Please keep them. > > Hmm. Okay. I guess I could stuff a struct there. But it really looks > like waste since it is not used. > > > I can think of a new "customized page_pool counters strings" API, where the > > strings prefix is provided by the driver, and used to generate the per-pool > > strings. > > Okay. So I make room for it and you wire it up ;) Could I keep it as-is for now with the removal of the counter from the RQ since we don't have the per-queue/ ring API for it now? It is not too hard it back later on. The thing is that page_pool_get_stats() sums up the individual stats (from each ring) and it works as intended. To do what you ask for, you I would have to add a struct page_pool_stats to each struct mlx5e_rq_stats for the per-ring stats (so far so good) but then I would have manually merge the stats into one struct page_pool_stats to expose it via page_pool_ethtool_stats_get(). Then I am touching it again while changing the type of the counters and this is how this patch started. Sebastian
On 06/03/2025 10:32, Sebastian Andrzej Siewior wrote: > On 2025-03-05 21:20:57 [+0100], To Tariq Toukan wrote: >>> I like the direction of this patch, but we won't give up the per-ring >>> counters. Please keep them. >> >> Hmm. Okay. I guess I could stuff a struct there. But it really looks >> like waste since it is not used. >> >>> I can think of a new "customized page_pool counters strings" API, where the >>> strings prefix is provided by the driver, and used to generate the per-pool >>> strings. >> >> Okay. So I make room for it and you wire it up ;) > > Could I keep it as-is for now with the removal of the counter from the > RQ since we don't have the per-queue/ ring API for it now? I'm fine with transition to generic APIs, as long as we get no regression. We must keep the per-ring counters exposed. >It is not too > hard it back later on. > The thing is that page_pool_get_stats() sums up the individual stats > (from each ring) and it works as intended. To do what you ask for, you I > would have to add a struct page_pool_stats to each struct mlx5e_rq_stats > for the per-ring stats (so far so good) but then I would have manually > merge the stats into one struct page_pool_stats to expose it via > page_pool_ethtool_stats_get(). Then I am touching it again while > changing the type of the counters and this is how this patch started. > > Sebastian
On 2025-03-06 11:50:27 [+0200], Tariq Toukan wrote: > On 06/03/2025 10:32, Sebastian Andrzej Siewior wrote: > > Could I keep it as-is for now with the removal of the counter from the > > RQ since we don't have the per-queue/ ring API for it now? > > I'm fine with transition to generic APIs, as long as we get no regression. > We must keep the per-ring counters exposed. I don't see a regression. Could you please show me how per-ring counters for page_pool_stats are exposed at the moment? Maybe I am missing something important. Sebastian
On 06/03/2025 11:56, Sebastian Andrzej Siewior wrote: > On 2025-03-06 11:50:27 [+0200], Tariq Toukan wrote: >> On 06/03/2025 10:32, Sebastian Andrzej Siewior wrote: >>> Could I keep it as-is for now with the removal of the counter from the >>> RQ since we don't have the per-queue/ ring API for it now? >> >> I'm fine with transition to generic APIs, as long as we get no regression. >> We must keep the per-ring counters exposed. > > I don't see a regression. > Could you please show me how per-ring counters for page_pool_stats are > exposed at the moment? Maybe I am missing something important. > After a ring is created for the first time, we start exposing its stats forever. So after your interface is up for the first time, you should be able to see the following per-ring stats with respective indices in ethtool -S. pp_alloc_fast pp_alloc_slow pp_alloc_slow_high_order pp_alloc_empty pp_alloc_refill pp_alloc_waive pp_recycle_cached pp_recycle_cache_full pp_recycle_ring pp_recycle_ring_full pp_recycle_released_ref Obviously, depending on CONFIG_PAGE_POOL_STATS.
On 2025-03-06 12:22:42 [+0200], Tariq Toukan wrote: > > > On 06/03/2025 11:56, Sebastian Andrzej Siewior wrote: > > On 2025-03-06 11:50:27 [+0200], Tariq Toukan wrote: > > > On 06/03/2025 10:32, Sebastian Andrzej Siewior wrote: > > > > Could I keep it as-is for now with the removal of the counter from the > > > > RQ since we don't have the per-queue/ ring API for it now? > > > > > > I'm fine with transition to generic APIs, as long as we get no regression. > > > We must keep the per-ring counters exposed. > > > > I don't see a regression. > > Could you please show me how per-ring counters for page_pool_stats are > > exposed at the moment? Maybe I am missing something important. > > > > After a ring is created for the first time, we start exposing its stats > forever. > > So after your interface is up for the first time, you should be able to see > the following per-ring stats with respective indices in ethtool -S. > > pp_alloc_fast > pp_alloc_slow > pp_alloc_slow_high_order > pp_alloc_empty > pp_alloc_refill > pp_alloc_waive > pp_recycle_cached > pp_recycle_cache_full > pp_recycle_ring > pp_recycle_ring_full > pp_recycle_released_ref > > Obviously, depending on CONFIG_PAGE_POOL_STATS. That is correct. But if you have 4 rings, you only see one pp_alloc_fast which combines the stats of all 4 rings. Or do I miss something? Sebastian
On 06/03/2025 11:56, Sebastian Andrzej Siewior wrote: > On 2025-03-06 11:50:27 [+0200], Tariq Toukan wrote: >> On 06/03/2025 10:32, Sebastian Andrzej Siewior wrote: >>> Could I keep it as-is for now with the removal of the counter from the >>> RQ since we don't have the per-queue/ ring API for it now? >> >> I'm fine with transition to generic APIs, as long as we get no regression. >> We must keep the per-ring counters exposed. > > I don't see a regression. > Could you please show me how per-ring counters for page_pool_stats are > exposed at the moment? Maybe I am missing something important. > What do you see in your ethtool -S? In code, you can check this function: drivers/net/ethernet/mellanox/mlx5/core/en_stats.c :: static MLX5E_DECLARE_STATS_GRP_OP_FILL_STRS(channels)
On 2025-03-06 13:10:12 [+0200], Tariq Toukan wrote: > > > On 06/03/2025 11:56, Sebastian Andrzej Siewior wrote: > > On 2025-03-06 11:50:27 [+0200], Tariq Toukan wrote: > > > On 06/03/2025 10:32, Sebastian Andrzej Siewior wrote: > > > > Could I keep it as-is for now with the removal of the counter from the > > > > RQ since we don't have the per-queue/ ring API for it now? > > > > > > I'm fine with transition to generic APIs, as long as we get no regression. > > > We must keep the per-ring counters exposed. > > > > I don't see a regression. > > Could you please show me how per-ring counters for page_pool_stats are > > exposed at the moment? Maybe I am missing something important. > > > > What do you see in your ethtool -S? Now, after comparing it again I noticed that there is | rx_pp_alloc_fast: 27783 | rx0_pp_alloc_fast: 441 | rx1_pp_alloc_fast: 441 which I didn't noticed earlier. I didn't rx0,1,… for pp_alloc_fast. Thanks. Sebastian
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c index 611ec4b6f3709..ae4d51f11fc5d 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c @@ -37,9 +37,7 @@ #include "en/ptp.h" #include "en/port.h" -#ifdef CONFIG_PAGE_POOL_STATS #include <net/page_pool/helpers.h> -#endif void mlx5e_ethtool_put_stat(u64 **data, u64 val) { @@ -196,19 +194,6 @@ static const struct counter_desc sw_stats_desc[] = { { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_arfs_err) }, #endif { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_recover) }, -#ifdef CONFIG_PAGE_POOL_STATS - { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_fast) }, - { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_slow) }, - { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_slow_high_order) }, - { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_empty) }, - { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_refill) }, - { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_waive) }, - { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_cached) }, - { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_cache_full) }, - { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_ring) }, - { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_ring_full) }, - { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_released_ref) }, -#endif #ifdef CONFIG_MLX5_EN_TLS { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_tls_decrypted_packets) }, { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_tls_decrypted_bytes) }, @@ -257,7 +242,7 @@ static const struct counter_desc sw_stats_desc[] = { static MLX5E_DECLARE_STATS_GRP_OP_NUM_STATS(sw) { - return NUM_SW_COUNTERS; + return NUM_SW_COUNTERS + page_pool_ethtool_stats_get_count(); } static MLX5E_DECLARE_STATS_GRP_OP_FILL_STRS(sw) @@ -266,6 +251,8 @@ static MLX5E_DECLARE_STATS_GRP_OP_FILL_STRS(sw) for (i = 0; i < NUM_SW_COUNTERS; i++) ethtool_puts(data, sw_stats_desc[i].format); + + *data = page_pool_ethtool_stats_get_strings(*data); } static MLX5E_DECLARE_STATS_GRP_OP_FILL_STATS(sw) @@ -276,6 +263,9 @@ static MLX5E_DECLARE_STATS_GRP_OP_FILL_STATS(sw) mlx5e_ethtool_put_stat(data, MLX5E_READ_CTR64_CPU(&priv->stats.sw, sw_stats_desc, i)); +#ifdef CONFIG_PAGE_POOL_STATS + *data = page_pool_ethtool_stats_get(*data, &priv->stats.sw.page_pool_stats); +#endif } static void mlx5e_stats_grp_sw_update_stats_xdp_red(struct mlx5e_sw_stats *s, @@ -377,19 +367,6 @@ static void mlx5e_stats_grp_sw_update_stats_rq_stats(struct mlx5e_sw_stats *s, s->rx_arfs_err += rq_stats->arfs_err; #endif s->rx_recover += rq_stats->recover; -#ifdef CONFIG_PAGE_POOL_STATS - s->rx_pp_alloc_fast += rq_stats->pp_alloc_fast; - s->rx_pp_alloc_slow += rq_stats->pp_alloc_slow; - s->rx_pp_alloc_empty += rq_stats->pp_alloc_empty; - s->rx_pp_alloc_refill += rq_stats->pp_alloc_refill; - s->rx_pp_alloc_waive += rq_stats->pp_alloc_waive; - s->rx_pp_alloc_slow_high_order += rq_stats->pp_alloc_slow_high_order; - s->rx_pp_recycle_cached += rq_stats->pp_recycle_cached; - s->rx_pp_recycle_cache_full += rq_stats->pp_recycle_cache_full; - s->rx_pp_recycle_ring += rq_stats->pp_recycle_ring; - s->rx_pp_recycle_ring_full += rq_stats->pp_recycle_ring_full; - s->rx_pp_recycle_released_ref += rq_stats->pp_recycle_released_ref; -#endif #ifdef CONFIG_MLX5_EN_TLS s->rx_tls_decrypted_packets += rq_stats->tls_decrypted_packets; s->rx_tls_decrypted_bytes += rq_stats->tls_decrypted_bytes; @@ -497,30 +474,14 @@ static void mlx5e_stats_grp_sw_update_stats_qos(struct mlx5e_priv *priv, } #ifdef CONFIG_PAGE_POOL_STATS -static void mlx5e_stats_update_stats_rq_page_pool(struct mlx5e_channel *c) +static void mlx5e_stats_update_stats_rq_page_pool(struct mlx5e_sw_stats *s, + struct mlx5e_channel *c) { - struct mlx5e_rq_stats *rq_stats = c->rq.stats; - struct page_pool *pool = c->rq.page_pool; - struct page_pool_stats stats = { 0 }; - - if (!page_pool_get_stats(pool, &stats)) - return; - - rq_stats->pp_alloc_fast = stats.alloc_stats.fast; - rq_stats->pp_alloc_slow = stats.alloc_stats.slow; - rq_stats->pp_alloc_slow_high_order = stats.alloc_stats.slow_high_order; - rq_stats->pp_alloc_empty = stats.alloc_stats.empty; - rq_stats->pp_alloc_waive = stats.alloc_stats.waive; - rq_stats->pp_alloc_refill = stats.alloc_stats.refill; - - rq_stats->pp_recycle_cached = stats.recycle_stats.cached; - rq_stats->pp_recycle_cache_full = stats.recycle_stats.cache_full; - rq_stats->pp_recycle_ring = stats.recycle_stats.ring; - rq_stats->pp_recycle_ring_full = stats.recycle_stats.ring_full; - rq_stats->pp_recycle_released_ref = stats.recycle_stats.released_refcnt; + page_pool_get_stats(c->rq.page_pool, &s->page_pool_stats); } #else -static void mlx5e_stats_update_stats_rq_page_pool(struct mlx5e_channel *c) +static void mlx5e_stats_update_stats_rq_page_pool(struct mlx5e_sw_stats *s, + struct mlx5e_channel *c) { } #endif @@ -532,15 +493,13 @@ static MLX5E_DECLARE_STATS_GRP_OP_UPDATE_STATS(sw) memset(s, 0, sizeof(*s)); - for (i = 0; i < priv->channels.num; i++) /* for active channels only */ - mlx5e_stats_update_stats_rq_page_pool(priv->channels.c[i]); - for (i = 0; i < priv->stats_nch; i++) { struct mlx5e_channel_stats *channel_stats = priv->channel_stats[i]; int j; + mlx5e_stats_update_stats_rq_page_pool(s, priv->channels.c[i]); mlx5e_stats_grp_sw_update_stats_rq_stats(s, &channel_stats->rq); mlx5e_stats_grp_sw_update_stats_xdpsq(s, &channel_stats->rq_xdpsq); mlx5e_stats_grp_sw_update_stats_ch_stats(s, &channel_stats->ch); @@ -2086,19 +2045,6 @@ static const struct counter_desc rq_stats_desc[] = { { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, arfs_err) }, #endif { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, recover) }, -#ifdef CONFIG_PAGE_POOL_STATS - { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_alloc_fast) }, - { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_alloc_slow) }, - { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_alloc_slow_high_order) }, - { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_alloc_empty) }, - { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_alloc_refill) }, - { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_alloc_waive) }, - { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_recycle_cached) }, - { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_recycle_cache_full) }, - { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_recycle_ring) }, - { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_recycle_ring_full) }, - { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_recycle_released_ref) }, -#endif #ifdef CONFIG_MLX5_EN_TLS { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, tls_decrypted_packets) }, { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, tls_decrypted_bytes) }, diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h index 5961c569cfe01..f6a186e293c4c 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h @@ -33,6 +33,8 @@ #ifndef __MLX5_EN_STATS_H__ #define __MLX5_EN_STATS_H__ +#include <net/page_pool/types.h> + #define MLX5E_READ_CTR64_CPU(ptr, dsc, i) \ (*(u64 *)((char *)ptr + dsc[i].offset)) #define MLX5E_READ_CTR64_BE(ptr, dsc, i) \ @@ -216,17 +218,7 @@ struct mlx5e_sw_stats { u64 ch_force_irq; u64 ch_eq_rearm; #ifdef CONFIG_PAGE_POOL_STATS - u64 rx_pp_alloc_fast; - u64 rx_pp_alloc_slow; - u64 rx_pp_alloc_slow_high_order; - u64 rx_pp_alloc_empty; - u64 rx_pp_alloc_refill; - u64 rx_pp_alloc_waive; - u64 rx_pp_recycle_cached; - u64 rx_pp_recycle_cache_full; - u64 rx_pp_recycle_ring; - u64 rx_pp_recycle_ring_full; - u64 rx_pp_recycle_released_ref; + struct page_pool_stats page_pool_stats; #endif #ifdef CONFIG_MLX5_EN_TLS u64 tx_tls_encrypted_packets; @@ -381,19 +373,6 @@ struct mlx5e_rq_stats { u64 arfs_err; #endif u64 recover; -#ifdef CONFIG_PAGE_POOL_STATS - u64 pp_alloc_fast; - u64 pp_alloc_slow; - u64 pp_alloc_slow_high_order; - u64 pp_alloc_empty; - u64 pp_alloc_refill; - u64 pp_alloc_waive; - u64 pp_recycle_cached; - u64 pp_recycle_cache_full; - u64 pp_recycle_ring; - u64 pp_recycle_ring_full; - u64 pp_recycle_released_ref; -#endif #ifdef CONFIG_MLX5_EN_TLS u64 tls_decrypted_packets; u64 tls_decrypted_bytes;
The statistics gathering code for page_pool statistics has multiple steps: - gather statistics from a channel via page_pool_get_stats() to an on-stack structure. - copy this data to dedicated rq_stats. - copy the data from rq_stats global mlx5e_sw_stats structure, and merge per-queue statistics into one counter. - Finally copy the data the specific order for the ethtool query. The downside here is that the individual counter types are expected to be u64 and if something changes, the code breaks. Also if additional counter are added to struct page_pool_stats then they are not automtically picked up by the driver but need to be manually added in all four spots. Remove the page_pool_stats fields from rq_stats_desc and use instead page_pool_ethtool_stats_get_count() for the number of files and page_pool_ethtool_stats_get_strings() for the strings which are added at the end. Remove page_pool_stats members from all structs and add the struct to mlx5e_sw_stats where the data is gathered directly for all channels. At the end, use page_pool_ethtool_stats_get() to copy the data to the output buffer. Suggested-by: Joe Damato <jdamato@fastly.com> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- diffstat wise, this looks nice. I hope I didn't break anything. Providing an empty struct page_pool_stats in the !CONFIG_PAGE_POOL_STATS case should eliminate a few additional ifdefs. .../ethernet/mellanox/mlx5/core/en_stats.c | 78 +++---------------- .../ethernet/mellanox/mlx5/core/en_stats.h | 27 +------ 2 files changed, 15 insertions(+), 90 deletions(-)