Message-ID: <8168a8ee-ad2f-46c5-b48e-488a23243b3d@gmail.com>
Date: Wed, 5 Mar 2025 21:44:23 +0200
From: Tariq Toukan <ttoukan.linux@...il.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
 linux-rdma@...r.kernel.org, netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>,
 Andrew Lunn <andrew+netdev@...n.ch>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Joe Damato <jdamato@...tly.com>,
 Leon Romanovsky <leon@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Saeed Mahameed <saeedm@...dia.com>, Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH net-next] net/mlnx5: Use generic code for page_pool
 statistics.



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@...tly.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.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

Powered by blists - more mailing lists