ethdev: fix xstat name of basic stats per queue
Checks
Commit Message
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(-)
Comments
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>
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. */
>
>-----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&
>>
>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@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.
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?
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>
> -----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.
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?
> -----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
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
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?
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&
>>>
>> 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@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
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.
@@ -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()``
@@ -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++;
}
@@ -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. */