[RFC 0/5] make PMD xstat_get alike

[RFC 0/5] make PMD xstat_get alike

[Stephen Hemminger]

The recent bug report about the txgbe driver motivated looking
at inconsistencies in drivers handling of xstats_get callback.
All drivers should treat call the same.

This is RFC since I don't have access to any of this hardware to test.

Stephen Hemminger (5):
  net/txgbe: fix query handling in xstats_get
  net/ngbe: fix query handling in xstats_get
  net/atlantic: fix handling of xstats_get
  net/octeontx: fix handling of xstats_get
  net/sfc: fix handling of xstats_get queries

 drivers/net/atlantic/atl_ethdev.c      |  2 +-
 drivers/net/ngbe/ngbe_ethdev.c         | 11 ++++-------
 drivers/net/octeontx/octeontx_ethdev.c |  3 +++
 drivers/net/sfc/sfc_ethdev.c           |  7 ++++---
 drivers/net/txgbe/txgbe_ethdev.c       | 12 +++---------
 5 files changed, 15 insertions(+), 20 deletions(-)

-- 
2.45.2

[RFC 1/5] net/txgbe: fix query handling in xstats_get

[Stephen Hemminger]

The xstats_get function in this driver did not act the same
as other drivers when queried. The correct check is to look
at the requested number of stats and compare it to the available
stats and if the request is too small, return the correct size.

Bugzilla ID: 1560
Fixes: 91fe49c87d76 ("net/txgbe: support device xstats")
Cc: jiawenwu@trustnetic.com

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/txgbe/txgbe_ethdev.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
index 2834468764..ee829d9120 100644
--- a/drivers/net/txgbe/txgbe_ethdev.c
+++ b/drivers/net/txgbe/txgbe_ethdev.c
@@ -2547,19 +2547,13 @@ txgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 	struct txgbe_hw_stats *hw_stats = TXGBE_DEV_STATS(dev);
 	unsigned int i, count;
 
-	txgbe_read_stats_registers(hw, hw_stats);
-
-	/* If this is a reset xstats is NULL, and we have cleared the
-	 * registers by reading them.
-	 */
 	count = txgbe_xstats_calc_num(dev);
-	if (xstats == NULL)
+	if (limit < count)
 		return count;
 
-	limit = min(limit, txgbe_xstats_calc_num(dev));
-
+	txgbe_read_stats_registers(hw, hw_stats);
 	/* Extended stats from txgbe_hw_stats */
-	for (i = 0; i < limit; i++) {
+	for (i = 0; i < count; i++) {
 		uint32_t offset = 0;
 
 		if (txgbe_get_offset_by_id(i, &offset)) {
-- 
2.45.2

[RFC 2/5] net/ngbe: fix query handling in xstats_get

[Stephen Hemminger]

The xstats_get function in this driver did not act the same
as other drivers when queried. The correct check is to look
at the requested number of stats and compare it to the available
stats and if the request is too small, return the correct size.

Bugzilla ID: 1560
Fixes: 8b433d04adc9 ("net/ngbe: support device xstats")
Cc: jiawenwu@trustnetic.com

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/ngbe/ngbe_ethdev.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ngbe/ngbe_ethdev.c b/drivers/net/ngbe/ngbe_ethdev.c
index 6c45ffaad3..ee65829aa0 100644
--- a/drivers/net/ngbe/ngbe_ethdev.c
+++ b/drivers/net/ngbe/ngbe_ethdev.c
@@ -1693,19 +1693,16 @@ ngbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 	struct ngbe_hw_stats *hw_stats = NGBE_DEV_STATS(dev);
 	unsigned int i, count;
 
-	ngbe_read_stats_registers(hw, hw_stats);
-
-	/* If this is a reset xstats is NULL, and we have cleared the
-	 * registers by reading them.
-	 */
 	count = ngbe_xstats_calc_num(dev);
-	if (xstats == NULL)
+	if (limit < count)
 		return count;
 
+	ngbe_read_stats_registers(hw, hw_stats);
+
 	limit = min(limit, ngbe_xstats_calc_num(dev));
 
 	/* Extended stats from ngbe_hw_stats */
-	for (i = 0; i < limit; i++) {
+	for (i = 0; i < count; i++) {
 		uint32_t offset = 0;
 
 		if (ngbe_get_offset_by_id(i, &offset)) {
-- 
2.45.2

[RFC 3/5] net/atlantic: fix handling of xstats_get

[Stephen Hemminger]

The xstats_get function in this driver did not act the same
as other drivers when queried. The correct check is to look
at the requested number of stats and compare it to the available
stats and if the request is too small, return the correct size.

Fixes: fbe059e87209 ("net/atlantic: implement device statistics")
Cc: pavel.belous@aquantia.com

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/atlantic/atl_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/atlantic/atl_ethdev.c b/drivers/net/atlantic/atl_ethdev.c
index 9cde935834..eac6f5a7b9 100644
--- a/drivers/net/atlantic/atl_ethdev.c
+++ b/drivers/net/atlantic/atl_ethdev.c
@@ -1029,7 +1029,7 @@ atl_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *stats,
 	unsigned int i;
 	unsigned int count = atl_dev_xstats_get_count(dev);
 
-	if (!stats)
+	if (n < count)
 		return count;
 
 	if (hw->aq_fw_ops->send_macsec_req != NULL) {
-- 
2.45.2

[RFC 4/5] net/octeontx: fix handling of xstats_get

[Stephen Hemminger]

The xstats_get function in this driver did not act the same
as other drivers when queried. The correct check is to look
at the requested number of stats and compare it to the available
stats and if the request is too small, return the correct size.

Fixes: 5538990924f2 ("net/octeontx: add basic stats support")
Cc: jerin.jacob@caviumnetworks.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/octeontx/octeontx_ethdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/octeontx/octeontx_ethdev.c b/drivers/net/octeontx/octeontx_ethdev.c
index 3b8d717797..51093cc815 100644
--- a/drivers/net/octeontx/octeontx_ethdev.c
+++ b/drivers/net/octeontx/octeontx_ethdev.c
@@ -1016,6 +1016,9 @@ octeontx_dev_xstats_get(struct rte_eth_dev *dev,
 	struct octeontx_nic *nic = octeontx_pmd_priv(dev);
 
 	PMD_INIT_FUNC_TRACE();
+	if (n < NUM_BGX_XSTAT)
+		return NUM_BGX_XSTAT;
+
 	return octeontx_port_xstats(nic, xstats, n);
 }
 
-- 
2.45.2

[RFC 5/5] net/sfc: fix handling of xstats_get queries

[Stephen Hemminger]

The xstats_get function in this driver did not act the same
as other drivers when queried. The correct check is to look
at the requested number of stats and compare it to the available
stats and if the request is too small, return the correct size.

Fixes: fdd7719eb3c1 ("net/sfc: add xstats for Rx/Tx doorbells")
Cc: ivan.ilchenko@oktetlabs.ru
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/sfc/sfc_ethdev.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 3480a51642..f0710dab5c 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -833,11 +833,12 @@ sfc_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 {
 	struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
 	unsigned int nb_written = 0;
-	unsigned int nb_supported = 0;
+	unsigned int nb_supported;
 	int rc;
 
-	if (unlikely(xstats == NULL))
-		return sfc_xstats_get_nb_supported(sa);
+	nb_supported = sfc_xstats_get_nb_supported(sa);
+	if (xstats_count < nb_supported)
+		return nb_supported;
 
 	rc = sfc_port_get_mac_stats(sa, xstats, xstats_count, &nb_written);
 	if (rc < 0)
-- 
2.45.2

RE: [RFC 1/5] net/txgbe: fix query handling in xstats_get

[Jiawen Wu]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, October 9, 2024 12:00 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; jiawenwu@trustnetic.com
> Subject: [RFC 1/5] net/txgbe: fix query handling in xstats_get
> 
> The xstats_get function in this driver did not act the same
> as other drivers when queried. The correct check is to look
> at the requested number of stats and compare it to the available
> stats and if the request is too small, return the correct size.
> 
> Bugzilla ID: 1560
> Fixes: 91fe49c87d76 ("net/txgbe: support device xstats")
> Cc: jiawenwu@trustnetic.com
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/txgbe/txgbe_ethdev.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
> index 2834468764..ee829d9120 100644
> --- a/drivers/net/txgbe/txgbe_ethdev.c
> +++ b/drivers/net/txgbe/txgbe_ethdev.c
> @@ -2547,19 +2547,13 @@ txgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
>  	struct txgbe_hw_stats *hw_stats = TXGBE_DEV_STATS(dev);
>  	unsigned int i, count;
> 
> -	txgbe_read_stats_registers(hw, hw_stats);
> -
> -	/* If this is a reset xstats is NULL, and we have cleared the
> -	 * registers by reading them.
> -	 */
>  	count = txgbe_xstats_calc_num(dev);
> -	if (xstats == NULL)
> +	if (limit < count)
>  		return count;
> 
> -	limit = min(limit, txgbe_xstats_calc_num(dev));
> -
> +	txgbe_read_stats_registers(hw, hw_stats);
>  	/* Extended stats from txgbe_hw_stats */
> -	for (i = 0; i < limit; i++) {
> +	for (i = 0; i < count; i++) {
>  		uint32_t offset = 0;
> 
>  		if (txgbe_get_offset_by_id(i, &offset)) {
> --
> 2.45.2
> 
> 

Thanks!
Reviewed-by: Jiawen Wu <jiawenwu@trustnetic.com>

Re: [RFC 5/5] net/sfc: fix handling of xstats_get queries

[Stephen Hemminger]

On Tue,  8 Oct 2024 08:59:59 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> The xstats_get function in this driver did not act the same
> as other drivers when queried. The correct check is to look
> at the requested number of stats and compare it to the available
> stats and if the request is too small, return the correct size.
> 
> Fixes: fdd7719eb3c1 ("net/sfc: add xstats for Rx/Tx doorbells")
> Cc: ivan.ilchenko@oktetlabs.ru
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Could someone with Solarflare hardware test this please?

Re: [RFC 2/5] net/ngbe: fix query handling in xstats_get

[Stephen Hemminger]

On Tue,  8 Oct 2024 08:59:56 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> The xstats_get function in this driver did not act the same
> as other drivers when queried. The correct check is to look
> at the requested number of stats and compare it to the available
> stats and if the request is too small, return the correct size.
> 
> Bugzilla ID: 1560
> Fixes: 8b433d04adc9 ("net/ngbe: support device xstats")
> Cc: jiawenwu@trustnetic.com
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

This needs to be tested on this hardware before merging.

Re: [RFC 4/5] net/octeontx: fix handling of xstats_get

[Stephen Hemminger]

On Tue,  8 Oct 2024 08:59:58 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> The xstats_get function in this driver did not act the same
> as other drivers when queried. The correct check is to look
> at the requested number of stats and compare it to the available
> stats and if the request is too small, return the correct size.
> 
> Fixes: 5538990924f2 ("net/octeontx: add basic stats support")
> Cc: jerin.jacob@caviumnetworks.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/octeontx/octeontx_ethdev.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/octeontx/octeontx_ethdev.c b/drivers/net/octeontx/octeontx_ethdev.c
> index 3b8d717797..51093cc815 100644
> --- a/drivers/net/octeontx/octeontx_ethdev.c
> +++ b/drivers/net/octeontx/octeontx_ethdev.c
> @@ -1016,6 +1016,9 @@ octeontx_dev_xstats_get(struct rte_eth_dev *dev,
>  	struct octeontx_nic *nic = octeontx_pmd_priv(dev);
>  
>  	PMD_INIT_FUNC_TRACE();
> +	if (n < NUM_BGX_XSTAT)
> +		return NUM_BGX_XSTAT;
> +
>  	return octeontx_port_xstats(nic, xstats, n);
>  }
>  

Jerin or someone with access to this NIC, could you check
that it works?

RE: [RFC 2/5] net/ngbe: fix query handling in xstats_get

[Jiawen Wu]

Cc: Zaiyu Wang <zaiyuwang@net-swift.com>

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Thursday, December 5, 2024 6:19 AM
> To: dev@dpdk.org
> Cc: jiawenwu@trustnetic.com
> Subject: Re: [RFC 2/5] net/ngbe: fix query handling in xstats_get
> 
> On Tue,  8 Oct 2024 08:59:56 -0700
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> > The xstats_get function in this driver did not act the same
> > as other drivers when queried. The correct check is to look
> > at the requested number of stats and compare it to the available
> > stats and if the request is too small, return the correct size.
> >
> > Bugzilla ID: 1560
> > Fixes: 8b433d04adc9 ("net/ngbe: support device xstats")
> > Cc: jiawenwu@trustnetic.com
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> This needs to be tested on this hardware before merging.
> 

RE: [RFC 2/5] net/ngbe: fix query handling in xstats_get

[Zaiyu Wang]

> 
> Cc: Zaiyu Wang <zaiyuwang@net-swift.com>
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Thursday, December 5, 2024 6:19 AM
> > To: dev@dpdk.org
> > Cc: jiawenwu@trustnetic.com
> > Subject: Re: [RFC 2/5] net/ngbe: fix query handling in xstats_get
> >
> > On Tue,  8 Oct 2024 08:59:56 -0700
> > Stephen Hemminger <stephen@networkplumber.org> wrote:
> >
> > > The xstats_get function in this driver did not act the same as other
> > > drivers when queried. The correct check is to look at the requested
> > > number of stats and compare it to the available stats and if the
> > > request is too small, return the correct size.
> > >
> > > Bugzilla ID: 1560
> > > Fixes: 8b433d04adc9 ("net/ngbe: support device xstats")
> > > Cc: jiawenwu@trustnetic.com
> > >
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >
> > This needs to be tested on this hardware before merging.
> >
> 
Hi Stephen Hemminger,

We have tested this patch for ngbe&txgbe driver with the corresponding
hardware.
But this patch can't fix the bug reported in Bugzilla. In fact, most
applications,
including telemetry, call rte_eth_xstats_get twice, fist to get num of
xstats and then
to get the full xstats data.

This bug appeared after dpdk-22.11 because rte_tel_data_add_dict_string
restricted
the characters in dicts, and '[' and ']' were no longer allowed to be used,
so ngbe&txgbe
could not print xstats by telemetry. 

For more details about this restriction, please see:
https://patches.dpdk.org/project/dpdk/patch/20220909093523.471727-2-bruce.ri
chardson@intel.com/

And here is the code in ngbe for setting xstats name, which add '[' and ']'
in them:

static inline int
ngbe_get_name_by_id(uint32_t id, char *name, uint32_t size)
{
...
	if (id < NGBE_NB_HW_STATS) {
		snprintf(name, size, "[hw]%s",
			rte_ngbe_stats_strings[id].name);
		return 0;
	}

To save this issue, could we add '[' and ']' to the allowed characters of
telemetry? Or we
must change the xstats name in ngbe&txgbe driver?	

Thanks.