DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
@ 2020-10-07 21:48 Thomas Monjalon
  2020-10-08  9:09 ` Bruce Richardson
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Thomas Monjalon @ 2020-10-07 21:48 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, arybchenko

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:
+* 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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
  2020-10-07 21:48 [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue Thomas Monjalon
@ 2020-10-08  9:09 ` Bruce Richardson
  2020-10-08  9:10 ` Kevin Traynor
  2020-10-08 15:41 ` Ferruh Yigit
  2 siblings, 0 replies; 13+ messages in thread
From: Bruce Richardson @ 2020-10-08  9:09 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, ferruh.yigit, arybchenko

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>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
  2020-10-07 21:48 [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue Thomas Monjalon
  2020-10-08  9:09 ` Bruce Richardson
@ 2020-10-08  9:10 ` Kevin Traynor
  2020-10-08 10:29   ` Asaf Penso
  2020-10-09 21:01   ` Ferruh Yigit
  2020-10-08 15:41 ` Ferruh Yigit
  2 siblings, 2 replies; 13+ messages in thread
From: Kevin Traynor @ 2020-10-08  9:10 UTC (permalink / raw)
  To: Thomas Monjalon, dev; +Cc: ferruh.yigit, arybchenko

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:
> 	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>
> ---

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. */
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
  2020-10-08  9:10 ` Kevin Traynor
@ 2020-10-08 10:29   ` Asaf Penso
  2020-10-09 21:00     ` Ferruh Yigit
  2020-10-09 21:01   ` Ferruh Yigit
  1 sibling, 1 reply; 13+ messages in thread
From: Asaf Penso @ 2020-10-08 10:29 UTC (permalink / raw)
  To: Kevin Traynor, NBU-Contact-Thomas Monjalon, dev; +Cc: ferruh.yigit, arybchenko

>-----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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
  2020-10-07 21:48 [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue Thomas Monjalon
  2020-10-08  9:09 ` Bruce Richardson
  2020-10-08  9:10 ` Kevin Traynor
@ 2020-10-08 15:41 ` Ferruh Yigit
  2020-10-09  8:32   ` Power, Ciara
  2020-10-09 16:53   ` Kevin Laatz
  2 siblings, 2 replies; 13+ messages in thread
From: Ferruh Yigit @ 2020-10-08 15:41 UTC (permalink / raw)
  To: Thomas Monjalon, dev, Harry van Haaren, Ciara Power, Kevin Laatz
  Cc: arybchenko

On 10/7/2020 10:48 PM, 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>

no objection,
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.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_*``
> +

As far as I remember collect plugin was using xstat output, does this rename 
affects it? Or any other telemetry application relying on xstats.

Harry, Ciara, Kevin, do you know anything that will be affected from rename?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
  2020-10-08 15:41 ` Ferruh Yigit
@ 2020-10-09  8:32   ` Power, Ciara
  2020-10-09  8:36     ` Pattan, Reshma
  2020-10-09 16:53   ` Kevin Laatz
  1 sibling, 1 reply; 13+ messages in thread
From: Power, Ciara @ 2020-10-09  8:32 UTC (permalink / raw)
  To: Yigit, Ferruh, Thomas Monjalon, dev, Van Haaren, Harry, Laatz, Kevin
  Cc: arybchenko, Pattan, Reshma

Hi Ferruh, Thomas,


>-----Original Message-----
>From: Ferruh Yigit <ferruh.yigit@intel.com>
>Sent: Thursday 8 October 2020 16:42
>To: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; Van Haaren,
>Harry <harry.van.haaren@intel.com>; Power, Ciara <ciara.power@intel.com>;
>Laatz, Kevin <kevin.laatz@intel.com>
>Cc: arybchenko@solarflare.com
>Subject: Re: [PATCH] ethdev: fix xstat name of basic stats per queue
>
>On 10/7/2020 10:48 PM, 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.pd
>> f
>>
>> Fixes: bd6aa172cf35 ("ethdev: fetch extended statistics with integer
>> ids")
>>
>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>
>no objection,
>Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.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_*``
>> +
>
>As far as I remember collect plugin was using xstat output, does this rename
>affects it? Or any other telemetry application relying on xstats.
>
>Harry, Ciara, Kevin, do you know anything that will be affected from rename?

I don't think this change will affect anything with telemetry itself, but I am not so familiar with the CollectD plugin, Reshma may be able to verify that. 

When using the new dpdk-telemetry.py, the name is just changed in the output which seems ok to me:
	--> /ethdev/xstats,0
	{"/ethdev/xstats": {... "rx_q0packets": 0,

Changes to:
	--> /ethdev/xstats,0
	{"/ethdev/xstats": {... "rx_q0_packets": 0,


Acked-by: Ciara Power <ciara.power@intel.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
  2020-10-09  8:32   ` Power, Ciara
@ 2020-10-09  8:36     ` Pattan, Reshma
  2020-10-09  9:02       ` David Marchand
  0 siblings, 1 reply; 13+ messages in thread
From: Pattan, Reshma @ 2020-10-09  8:36 UTC (permalink / raw)
  To: Power, Ciara, Yigit, Ferruh, Thomas Monjalon, dev, Van Haaren,
	Harry, Laatz, Kevin
  Cc: arybchenko



> -----Original Message-----
> From: Power, Ciara <ciara.power@intel.com>
> Sent: Friday, October 9, 2020 9:33 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; dev@dpdk.org; Van Haaren, Harry
> <harry.van.haaren@intel.com>; Laatz, Kevin <kevin.laatz@intel.com>
> Cc: arybchenko@solarflare.com; Pattan, Reshma <reshma.pattan@intel.com>
> Subject: RE: [PATCH] ethdev: fix xstat name of basic stats per queue
> 
> Hi Ferruh, Thomas,
> 
> 
> >-----Original Message-----
> >From: Ferruh Yigit <ferruh.yigit@intel.com>
> >Sent: Thursday 8 October 2020 16:42
> >To: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; Van Haaren,
> >Harry <harry.van.haaren@intel.com>; Power, Ciara
> ><ciara.power@intel.com>; Laatz, Kevin <kevin.laatz@intel.com>
> >Cc: arybchenko@solarflare.com
> >Subject: Re: [PATCH] ethdev: fix xstat name of basic stats per queue
> >
> >On 10/7/2020 10:48 PM, 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.p
> >> d
> >> f
> >>
> >> Fixes: bd6aa172cf35 ("ethdev: fetch extended statistics with integer
> >> ids")
> >>
> >> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >
> >no objection,
> >Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.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_*``
> >> +
> >
> >As far as I remember collect plugin was using xstat output, does this
> >rename affects it? Or any other telemetry application relying on xstats.
> >
> >Harry, Ciara, Kevin, do you know anything that will be affected from rename?
> 
> I don't think this change will affect anything with telemetry itself, but I am not
> so familiar with the CollectD plugin, Reshma may be able to verify that.

Neither collectd has any dependency on naming. 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
  2020-10-09  8:36     ` Pattan, Reshma
@ 2020-10-09  9:02       ` David Marchand
  2020-10-09  9:08         ` Pattan, Reshma
  0 siblings, 1 reply; 13+ messages in thread
From: David Marchand @ 2020-10-09  9:02 UTC (permalink / raw)
  To: Pattan, Reshma
  Cc: Power, Ciara, Yigit, Ferruh, Thomas Monjalon, dev, Van Haaren,
	Harry, Laatz, Kevin, arybchenko

On Fri, Oct 9, 2020 at 10:36 AM Pattan, Reshma <reshma.pattan@intel.com> wrote:
> > >> +* 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_*``
> > >> +
> > >
> > >As far as I remember collect plugin was using xstat output, does this
> > >rename affects it? Or any other telemetry application relying on xstats.
> > >
> > >Harry, Ciara, Kevin, do you know anything that will be affected from rename?
> >
> > I don't think this change will affect anything with telemetry itself, but I am not
> > so familiar with the CollectD plugin, Reshma may be able to verify that.
>
> Neither collectd has any dependency on naming.

I am not sure about this statement.
collectd dpdkstats plugin does seem to inspect the names.

And I can see what looks like a workaround on the problem Thomas is fixing here.
https://github.com/collectd/collectd/commit/126b35ee8786a3aca1bcc1346a60bc528d06c414


Can you test Thomas patch?


-- 
David Marchand


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
  2020-10-09  9:02       ` David Marchand
@ 2020-10-09  9:08         ` Pattan, Reshma
  0 siblings, 0 replies; 13+ messages in thread
From: Pattan, Reshma @ 2020-10-09  9:08 UTC (permalink / raw)
  To: David Marchand
  Cc: Power, Ciara, Yigit, Ferruh, Thomas Monjalon, dev, Van Haaren,
	Harry, Laatz, Kevin, arybchenko



> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Subject: Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
> 
> On Fri, Oct 9, 2020 at 10:36 AM Pattan, Reshma <reshma.pattan@intel.com>
> wrote:
> > > >> +* 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_*``
> > > >> +
> > > >
> > > >As far as I remember collect plugin was using xstat output, does
> > > >this rename affects it? Or any other telemetry application relying on xstats.
> > > >
> > > >Harry, Ciara, Kevin, do you know anything that will be affected from
> rename?
> > >
> > > I don't think this change will affect anything with telemetry
> > > itself, but I am not so familiar with the CollectD plugin, Reshma may be able
> to verify that.
> >
> > Neither collectd has any dependency on naming.
> 
> I am not sure about this statement.
> collectd dpdkstats plugin does seem to inspect the names.
> 

Hi, 

I should have been more clear on this, It is dpdk_telemetry plugin of collectd I am talking about, as I worked on that.
dpdk_telemetry plugin  of collectd do not have dependency on the naming.

dpdkstats plugin I did not work on, so I cannot comment on that, might be Harry able to help with that .

Thanks,
Reshma 




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
  2020-10-08 15:41 ` Ferruh Yigit
  2020-10-09  8:32   ` Power, Ciara
@ 2020-10-09 16:53   ` Kevin Laatz
  2020-10-09 19:15     ` Ferruh Yigit
  1 sibling, 1 reply; 13+ messages in thread
From: Kevin Laatz @ 2020-10-09 16:53 UTC (permalink / raw)
  To: Ferruh Yigit, Thomas Monjalon, dev, Harry van Haaren, Ciara Power
  Cc: arybchenko


On 08/10/2020 16:41, Ferruh Yigit wrote:
> On 10/7/2020 10:48 PM, 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>
>
> no objection,
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.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_*``
>> +
>
> As far as I remember collect plugin was using xstat output, does this 
> rename affects it? Or any other telemetry application relying on xstats.
>
> Harry, Ciara, Kevin, do you know anything that will be affected from 
> rename?

Looks to me like everything will be ok in the old collectd plugins too - 
from doing a code review, it seems it just gets the names from ethdev.

/Kevin


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
  2020-10-09 16:53   ` Kevin Laatz
@ 2020-10-09 19:15     ` Ferruh Yigit
  0 siblings, 0 replies; 13+ messages in thread
From: Ferruh Yigit @ 2020-10-09 19:15 UTC (permalink / raw)
  To: Kevin Laatz, Thomas Monjalon, dev, Harry van Haaren, Ciara Power
  Cc: arybchenko, David Marchand, Maryam Tahhan

On 10/9/2020 5:53 PM, Kevin Laatz wrote:
> 
> On 08/10/2020 16:41, Ferruh Yigit wrote:
>> On 10/7/2020 10:48 PM, 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>
>>
>> no objection,
>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.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_*``
>>> +
>>
>> As far as I remember collect plugin was using xstat output, does this rename 
>> affects it? Or any other telemetry application relying on xstats.
>>
>> Harry, Ciara, Kevin, do you know anything that will be affected from rename?
> 
> Looks to me like everything will be ok in the old collectd plugins too - from 
> doing a code review, it seems it just gets the names from ethdev.
> 

Thanks for checking, the workaround David highlighted in the collectd seems 
preventing this change to cause an error, so we can proceed.

But it would be nice to test the 'dpdkstat' plugin with this change. Do you know 
who is the maintainer of that plugin in collectd?


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
  2020-10-08 10:29   ` Asaf Penso
@ 2020-10-09 21:00     ` Ferruh Yigit
  0 siblings, 0 replies; 13+ messages in thread
From: Ferruh Yigit @ 2020-10-09 21:00 UTC (permalink / raw)
  To: Asaf Penso, Kevin Traynor, NBU-Contact-Thomas Monjalon, dev,
	Min Hu (Connor), Yisen Zhuang, Wei Hu (Xavier)
  Cc: arybchenko

On 10/8/2020 11:29 AM, Asaf Penso wrote:
>> -----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.
> 

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue
  2020-10-08  9:10 ` Kevin Traynor
  2020-10-08 10:29   ` Asaf Penso
@ 2020-10-09 21:01   ` Ferruh Yigit
  1 sibling, 0 replies; 13+ messages in thread
From: Ferruh Yigit @ 2020-10-09 21:01 UTC (permalink / raw)
  To: Kevin Traynor, Thomas Monjalon, dev; +Cc: arybchenko

On 10/8/2020 10:10 AM, Kevin Traynor wrote:
> 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:
>> 	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>
>> ---
> 
> Acked-by: Kevin Traynor <ktraynor@redhat.com>
> 

Applied to dpdk-next-net/main, thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-10-09 21:01 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 21:48 [dpdk-dev] [PATCH] ethdev: fix xstat name of basic stats per queue Thomas Monjalon
2020-10-08  9:09 ` Bruce Richardson
2020-10-08  9:10 ` Kevin Traynor
2020-10-08 10:29   ` Asaf Penso
2020-10-09 21:00     ` Ferruh Yigit
2020-10-09 21:01   ` Ferruh Yigit
2020-10-08 15:41 ` Ferruh Yigit
2020-10-09  8:32   ` Power, Ciara
2020-10-09  8:36     ` Pattan, Reshma
2020-10-09  9:02       ` David Marchand
2020-10-09  9:08         ` Pattan, Reshma
2020-10-09 16:53   ` Kevin Laatz
2020-10-09 19:15     ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).