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

[Asaf Penso <asafp@nvidia.com>]

>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Kevin Traynor
>Sent: Thursday, October 8, 2020 12:10 PM
>To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org
>Cc: ferruh.yigit@intel.com; arybchenko@solarflare.com
>Subject: Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per
>queue
>
>On 07/10/2020 22:48, 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:
>>
>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Ffast.
>> dpdk.org%2Fevents%2Fslides%2FDPDK-2019-09-
>Ethernet_Statistics.pdf&amp;
>>
>data=02%7C01%7Casafp%40nvidia.com%7C4a551495aff7412fa65108d86b6a22
>2f%7
>>
>C43083d15727340c1b7db39efd9ccc17a%7C0%7C1%7C637377450869384533&a
>mp;sda
>>
>ta=1Nn2If%2BgHcSc4hZS8FtLTBD5eyEyZeDZZP4u0%2FON5lU%3D&amp;reserv
>ed=0
>>
>> Fixes: bd6aa172cf35 ("ethdev: fetch extended statistics with integer
>> ids")
>>
>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>> ---
>
>Acked-by: Kevin Traynor <ktraynor@redhat.com>
>
>
>>  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:
>> +* 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.
>*/
>>
Thanks, Thomas, for taking care of such changes 😊
Looks like hns3 pmd should be updated as well, since it uses the same format.