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

[Ferruh Yigit]

On 10/8/2020 11:29 AM, Asaf Penso wrote:
>> -----Original Message-----
>> From: dev <dev-bounces at dpdk.org> On Behalf Of Kevin Traynor
>> Sent: Thursday, October 8, 2020 12:10 PM
>> To: NBU-Contact-Thomas Monjalon <thomas at monjalon.net>; dev at dpdk.org
>> Cc: ferruh.yigit at intel.com; arybchenko at 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&
>>>
>> data=02%7C01%7Casafp%40nvidia.com%7C4a551495aff7412fa65108d86b6a22
>> 2f%7
>>>
>> C43083d15727340c1b7db39efd9ccc17a%7C0%7C1%7C637377450869384533&a
>> mp;sda
>>>
>> ta=1Nn2If%2BgHcSc4hZS8FtLTBD5eyEyZeDZZP4u0%2FON5lU%3D&reserv
>> ed=0
>>>
>>> Fixes: bd6aa172cf35 ("ethdev: fetch extended statistics with integer
>>> ids")
>>>
>>> Signed-off-by: Thomas Monjalon <thomas at monjalon.net>
>>> ---
>>
>> Acked-by: Kevin Traynor <ktraynor at 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.
> 

Added hns3 maintainers.

Xavier, Connor, Yisen, can you please check the above naming scheme for the 
xstats and apply it to the hns3 pmd specific ones?

Thanks,
ferruh