Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Oct 07, 2020 at 11:48:48PM +0200, Thomas Monjalon wrote:
> As described in doc/guides/prog_guide/poll_mode_drv.rst,
> the naming scheme for the xstats is parts separated with underscore:
> 	* direction
> 	* detail 1
> 	* detail 2
> 	* detail n
> 	* unit
> where detail 1 can be "q" followed with a queue number.
> It means the name of the stats per queue should be rx_qN_* or tx_qN_*.
> 
> The second underscore was missing so far.
> Fixing the basic xstat names may be considered an API change,
> that's why it should not be backported.
> 
> While fixing this mistake, some examples of the naming scheme
> are given as part of the API documentation of rte_eth_xstat_name.
> More proposals about standardizing statistics:
> 	http://fast.dpdk.org/events/slides/DPDK-2019-09-Ethernet_Statistics.pdf
> 
> Fixes: bd6aa172cf35 ("ethdev: fetch extended statistics with integer ids")
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  doc/guides/rel_notes/release_20_11.rst | 8 +++++++-
>  lib/librte_ethdev/rte_ethdev.c         | 4 ++--
>  lib/librte_ethdev/rte_ethdev.h         | 7 +++++++
>  3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
> index cdf20404c9..d0d77c5d3d 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -200,7 +200,13 @@ API Changes
>  
>  * ethdev: ``rte_eth_rx_descriptor_done()`` API has been deprecated.
>  
> -* Renamed internal ethdev APIs:

Technically not related to this patch, but I think it's ok to slip it in
here. :-)

> +* ethdev: Renamed basic statistics per queue. An underscore is inserted
> +  between the queue number and the rest of the xstat name:
> +
> +  * ``rx_qN*`` -> ``rx_qN_*``
> +  * ``tx_qN*`` -> ``tx_qN_*``
> +
> +* ethdev: Renamed internal APIs:
>  
>    * ``_rte_eth_dev_callback_process()`` -> ``rte_eth_dev_callback_process()``
>    * ``_rte_eth_dev_reset`` -> ``rte_eth_dev_internal_reset()``
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 48d1333b17..286c1b5966 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -2549,7 +2549,7 @@ rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
>  		for (idx = 0; idx < RTE_NB_RXQ_STATS; idx++) {
>  			snprintf(xstats_names[cnt_used_entries].name,
>  				sizeof(xstats_names[0].name),
> -				"rx_q%u%s",
> +				"rx_q%u_%s",
>  				id_queue, rte_rxq_stats_strings[idx].name);
>  			cnt_used_entries++;
>  		}
> @@ -2560,7 +2560,7 @@ rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
>  		for (idx = 0; idx < RTE_NB_TXQ_STATS; idx++) {
>  			snprintf(xstats_names[cnt_used_entries].name,
>  				sizeof(xstats_names[0].name),
> -				"tx_q%u%s",
> +				"tx_q%u_%s",
>  				id_queue, rte_txq_stats_strings[idx].name);
>  			cnt_used_entries++;
>  		}
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d2bf74f128..86434c9cae 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1507,6 +1507,13 @@ struct rte_eth_xstat {
>   * An array of this structure is returned by rte_eth_xstats_get_names().
>   * It lists the names of extended statistics for a PMD. The *rte_eth_xstat*
>   * structure references these names by their array index.
> + *
> + * The xstats should follow a common naming scheme.
> + * Some names are standardized in rte_stats_strings.
> + * Examples:
> + *     - rx_missed_errors
> + *     - tx_q3_bytes
> + *     - tx_size_128_to_255_packets
>   */
>  struct rte_eth_xstat_name {
>  	char name[RTE_ETH_XSTATS_NAME_SIZE]; /**< The statistic name. */
> -- 
> 2.28.0
> 

Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>