[dpdk-stable] [PATCH 01/11] net/sfc: fix get xstats by ID callback to use MAC stats lock

[dpdk-stable] [PATCH 01/11] net/sfc: fix get xstats by ID callback to use MAC stats lock

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Add MAC stats lock in get xstats by id callback before reading
number of supported MAC stats.

Fixes: 73280c1e4ff ("net/sfc: support xstats retrieval by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc_ethdev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 88896db1f8..d4ac61ff76 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -789,12 +789,14 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	int ret;
 	int rc;
 
-	if (unlikely(values == NULL) ||
-	    unlikely((ids == NULL) && (n < port->mac_stats_nb_supported)))
-		return port->mac_stats_nb_supported;
-
 	rte_spinlock_lock(&port->mac_stats_lock);
 
+	if (unlikely(values == NULL) ||
+	    unlikely(ids == NULL && n < port->mac_stats_nb_supported)) {
+		ret = port->mac_stats_nb_supported;
+		goto unlock;
+	}
+
 	rc = sfc_port_update_mac_stats(sa);
 	if (rc != 0) {
 		SFC_ASSERT(rc > 0);
-- 
2.30.2

[dpdk-stable] [PATCH 02/11] net/sfc: fix reading adapter state without locking

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update MAC stats function reads adapter state with MAC stats locking
but without adapter locking. Add adapter locking before calling this
function and remove MAC stats locking since there's no point to have
it together with adapter locking. The second place MAC stats locking
is used is MAC stats reset function. It's called with adapter being
already locked so there's no point to use MAC stats locking anymore.

Fixes: 1caab2f1e68 ("net/sfc: add basic statistics")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc.h        |  1 -
 drivers/net/sfc/sfc_ethdev.c | 28 ++++++++++++++++++++--------
 drivers/net/sfc/sfc_port.c   |  9 +++------
 3 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/net/sfc/sfc.h b/drivers/net/sfc/sfc.h
index 546739bd4a..c7b0e5a30d 100644
--- a/drivers/net/sfc/sfc.h
+++ b/drivers/net/sfc/sfc.h
@@ -130,7 +130,6 @@ struct sfc_port {
 	unsigned int			nb_mcast_addrs;
 	uint8_t				*mcast_addrs;
 
-	rte_spinlock_t			mac_stats_lock;
 	uint64_t			*mac_stats_buf;
 	unsigned int			mac_stats_nb_supported;
 	efsys_mem_t			mac_stats_dma_mem;
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index d4ac61ff76..d5417e5e65 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -613,7 +613,7 @@ sfc_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 	uint64_t *mac_stats;
 	int ret;
 
-	rte_spinlock_lock(&port->mac_stats_lock);
+	sfc_adapter_lock(sa);
 
 	ret = sfc_port_update_mac_stats(sa);
 	if (ret != 0)
@@ -686,7 +686,7 @@ sfc_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 	}
 
 unlock:
-	rte_spinlock_unlock(&port->mac_stats_lock);
+	sfc_adapter_unlock(sa);
 	SFC_ASSERT(ret >= 0);
 	return -ret;
 }
@@ -698,12 +698,15 @@ sfc_stats_reset(struct rte_eth_dev *dev)
 	struct sfc_port *port = &sa->port;
 	int rc;
 
+	sfc_adapter_lock(sa);
+
 	if (sa->state != SFC_ADAPTER_STARTED) {
 		/*
 		 * The operation cannot be done if port is not started; it
 		 * will be scheduled to be done during the next port start
 		 */
 		port->mac_stats_reset_pending = B_TRUE;
+		sfc_adapter_unlock(sa);
 		return 0;
 	}
 
@@ -711,6 +714,8 @@ sfc_stats_reset(struct rte_eth_dev *dev)
 	if (rc != 0)
 		sfc_err(sa, "failed to reset statistics (rc = %d)", rc);
 
+	sfc_adapter_unlock(sa);
+
 	SFC_ASSERT(rc >= 0);
 	return -rc;
 }
@@ -726,7 +731,7 @@ sfc_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 	unsigned int i;
 	int nstats = 0;
 
-	rte_spinlock_lock(&port->mac_stats_lock);
+	sfc_adapter_lock(sa);
 
 	rc = sfc_port_update_mac_stats(sa);
 	if (rc != 0) {
@@ -748,7 +753,7 @@ sfc_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 	}
 
 unlock:
-	rte_spinlock_unlock(&port->mac_stats_lock);
+	sfc_adapter_unlock(sa);
 
 	return nstats;
 }
@@ -789,7 +794,7 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	int ret;
 	int rc;
 
-	rte_spinlock_lock(&port->mac_stats_lock);
+	sfc_adapter_lock(sa);
 
 	if (unlikely(values == NULL) ||
 	    unlikely(ids == NULL && n < port->mac_stats_nb_supported)) {
@@ -819,7 +824,7 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	ret = nb_written;
 
 unlock:
-	rte_spinlock_unlock(&port->mac_stats_lock);
+	sfc_adapter_unlock(sa);
 
 	return ret;
 }
@@ -835,9 +840,14 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 	unsigned int nb_written = 0;
 	unsigned int i;
 
+	sfc_adapter_lock(sa);
+
 	if (unlikely(xstats_names == NULL) ||
-	    unlikely((ids == NULL) && (size < port->mac_stats_nb_supported)))
-		return port->mac_stats_nb_supported;
+	    unlikely((ids == NULL) && (size < port->mac_stats_nb_supported))) {
+		nb_supported = port->mac_stats_nb_supported;
+		sfc_adapter_unlock(sa);
+		return nb_supported;
+	}
 
 	for (i = 0; (i < EFX_MAC_NSTATS) && (nb_written < size); ++i) {
 		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
@@ -853,6 +863,8 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 		++nb_supported;
 	}
 
+	sfc_adapter_unlock(sa);
+
 	return nb_written;
 }
 
diff --git a/drivers/net/sfc/sfc_port.c b/drivers/net/sfc/sfc_port.c
index ac117f9c48..cdc0f94f19 100644
--- a/drivers/net/sfc/sfc_port.c
+++ b/drivers/net/sfc/sfc_port.c
@@ -43,7 +43,7 @@ sfc_port_update_mac_stats(struct sfc_adapter *sa)
 	unsigned int nb_attempts = 0;
 	int rc;
 
-	SFC_ASSERT(rte_spinlock_is_locked(&port->mac_stats_lock));
+	SFC_ASSERT(sfc_adapter_is_locked(sa));
 
 	if (sa->state != SFC_ADAPTER_STARTED)
 		return EINVAL;
@@ -103,14 +103,13 @@ sfc_port_reset_sw_stats(struct sfc_adapter *sa)
 int
 sfc_port_reset_mac_stats(struct sfc_adapter *sa)
 {
-	struct sfc_port *port = &sa->port;
 	int rc;
 
-	rte_spinlock_lock(&port->mac_stats_lock);
+	SFC_ASSERT(sfc_adapter_is_locked(sa));
+
 	rc = efx_mac_stats_clear(sa->nic);
 	if (rc == 0)
 		sfc_port_reset_sw_stats(sa);
-	rte_spinlock_unlock(&port->mac_stats_lock);
 
 	return rc;
 }
@@ -416,8 +415,6 @@ sfc_port_attach(struct sfc_adapter *sa)
 		goto fail_mcast_addr_list_buf_alloc;
 	}
 
-	rte_spinlock_init(&port->mac_stats_lock);
-
 	rc = ENOMEM;
 	port->mac_stats_buf = rte_calloc_socket("mac_stats_buf", EFX_MAC_NSTATS,
 						sizeof(uint64_t), 0,
-- 
2.30.2

[dpdk-stable] [PATCH 03/11] ethdev: fix docs of functions getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Document valid combinations of input arguments in accordance with
current implementation in ethdev.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/rte_ethdev.h | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index faf3bd901d..1f63118544 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2873,12 +2873,15 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  *   The port identifier of the Ethernet device.
  * @param xstats_names
  *   An rte_eth_xstat_name array of at least *size* elements to
- *   be filled. If set to NULL, the function returns the required number
- *   of elements.
+ *   be filled. Must not be NULL if @p ids are specified (not NULL).
  * @param ids
- *   IDs array given by app to retrieve specific statistics
+ *   IDs array given by app to retrieve specific statistics. May be NULL
+ *   to retrieve all available statistics.
  * @param size
- *   The size of the xstats_names array (number of elements).
+ *   If @p ids is not NULL, number of elements in the array with requested IDs
+ *   and number of elements in @p xstats_names to put names in. If @p ids is
+ *   NULL, number of elements in @p xstats_names to put all available statistics
+ *   names in.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
@@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int
 rte_eth_xstats_get_names_by_id(uint16_t port_id,
@@ -2900,13 +2903,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  *   The port identifier of the Ethernet device.
  * @param ids
  *   A pointer to an ids array passed by application. This tells which
- *   statistics values function should retrieve. This parameter
- *   can be set to NULL if size is 0. In this case function will retrieve
+ *   statistics values function should retrieve. May be NULL to retrieve
  *   all available statistics.
  * @param values
  *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL if ids are specified (not NULL).
  * @param size
- *   The size of the ids array (number of elements).
+ *   If @p ids is not NULL, number of elements in the array with requested IDs
+ *   and number of elements in values to put statistics in. If @p ids is NULL,
+ *   number of elements in values to put all available statistics in.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
@@ -2914,7 +2919,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 			     uint64_t *values, unsigned int size);
-- 
2.30.2

[dpdk-stable] [PATCH 04/11] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update xstats by IDs callbacks documentation in accordance with
ethdev usage of these callbacks. Document valid combinations of
input arguments to make driver implementation simpler.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/ethdev_driver.h | 43 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 40e474aa7e..fd5b7ca550 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat *stats, unsigned int n);
 /**< @internal Get extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get extended stats of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param values
+ *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL.
+ * @param n
+ *   Element count in @p ids and @p values
+ *
+ * @return
+ *   - A number of filled in stats.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
 				      const uint64_t *ids,
 				      uint64_t *values,
 				      unsigned int n);
-/**< @internal Get extended stats of an Ethernet device. */
 
 /**
  * @internal
@@ -218,10 +235,32 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, unsigned int size);
 /**< @internal Get names of extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get names of extended stats of an Ethernet device.
+ * For name count, set @p xstats_names and @p ids to NULL.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. Can be NULL together with @p ids to retrieve number of
+ *   available statistics.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Can be NULL together
+ *   with @p xstats_names to retrieve number of available statistics.
+ * @param size
+ *   Size of ids and xstats_names arrays.
+ *   Element count in @p ids and @p xstats_names
+ *
+ * @return
+ *   - A number of filled in stats if both xstats_names and ids are not NULL.
+ *   - A number of available stats if both xstats_names and ids are NULL.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
 	unsigned int size);
-/**< @internal Get names of extended stats of an Ethernet device. */
 
 typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
 					     uint16_t queue_id,
-- 
2.30.2

[dpdk-stable] [PATCH 05/11] net/sfc: fix xstats by ID callbacks according to ethdev

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Fix xstats by ID callbacks according to ethdev usage.
Handle combinations of input arguments that are required by ethdev
and sanity check and reject other combinations on callback entry.

Fixes: 73280c1e4ff ("net/sfc: support xstats retrieval by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc_ethdev.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index d5417e5e65..fca3f524a1 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -794,13 +794,10 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	int ret;
 	int rc;
 
-	sfc_adapter_lock(sa);
+	if (unlikely(ids == NULL || values == NULL))
+		return -EINVAL;
 
-	if (unlikely(values == NULL) ||
-	    unlikely(ids == NULL && n < port->mac_stats_nb_supported)) {
-		ret = port->mac_stats_nb_supported;
-		goto unlock;
-	}
+	sfc_adapter_lock(sa);
 
 	rc = sfc_port_update_mac_stats(sa);
 	if (rc != 0) {
@@ -815,7 +812,7 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
 			continue;
 
-		if ((ids == NULL) || (ids[nb_written] == nb_supported))
+		if (ids[nb_written] == nb_supported)
 			values[nb_written++] = mac_stats[i];
 
 		++nb_supported;
@@ -840,10 +837,13 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 	unsigned int nb_written = 0;
 	unsigned int i;
 
+	if (unlikely(xstats_names == NULL && ids != NULL) ||
+	    unlikely(xstats_names != NULL && ids == NULL))
+		return -EINVAL;
+
 	sfc_adapter_lock(sa);
 
-	if (unlikely(xstats_names == NULL) ||
-	    unlikely((ids == NULL) && (size < port->mac_stats_nb_supported))) {
+	if (unlikely(xstats_names == NULL && ids == NULL)) {
 		nb_supported = port->mac_stats_nb_supported;
 		sfc_adapter_unlock(sa);
 		return nb_supported;
@@ -853,7 +853,7 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
 			continue;
 
-		if ((ids == NULL) || (ids[nb_written] == nb_supported)) {
+		if (ids[nb_written] == nb_supported) {
 			char *name = xstats_names[nb_written++].name;
 
 			strlcpy(name, efx_mac_stat_name(sa->nic, i),
-- 
2.30.2

[dpdk-stable] [PATCH 06/11] net/sfc: fix accessing xstats by an unsorted list of IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Device may support only some MAC stats. Add mapping from ids to subset
of supported MAC stats for each port.

Fixes: 73280c1e4ff ("net/sfc: support xstats retrieval by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc.h        |  2 ++
 drivers/net/sfc/sfc_ethdev.c | 44 ++++++++++++++++++------------------
 drivers/net/sfc/sfc_port.c   | 29 ++++++++++++++++++------
 3 files changed, 46 insertions(+), 29 deletions(-)

diff --git a/drivers/net/sfc/sfc.h b/drivers/net/sfc/sfc.h
index c7b0e5a30d..972d32606d 100644
--- a/drivers/net/sfc/sfc.h
+++ b/drivers/net/sfc/sfc.h
@@ -141,6 +141,8 @@ struct sfc_port {
 
 	uint32_t		mac_stats_mask[EFX_MAC_STATS_MASK_NPAGES];
 
+	unsigned int			mac_stats_by_id[EFX_MAC_NSTATS];
+
 	uint64_t			ipackets;
 };
 
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index fca3f524a1..ae9304f90f 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -788,8 +788,6 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
 	struct sfc_port *port = &sa->port;
 	uint64_t *mac_stats;
-	unsigned int nb_supported = 0;
-	unsigned int nb_written = 0;
 	unsigned int i;
 	int ret;
 	int rc;
@@ -808,17 +806,19 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 
 	mac_stats = port->mac_stats_buf;
 
-	for (i = 0; (i < EFX_MAC_NSTATS) && (nb_written < n); ++i) {
-		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
-			continue;
-
-		if (ids[nb_written] == nb_supported)
-			values[nb_written++] = mac_stats[i];
+	SFC_ASSERT(port->mac_stats_nb_supported <=
+		   RTE_DIM(port->mac_stats_by_id));
 
-		++nb_supported;
+	for (i = 0; i < n; i++) {
+		if (ids[i] < port->mac_stats_nb_supported) {
+			values[i] = mac_stats[port->mac_stats_by_id[ids[i]]];
+		} else {
+			ret = i;
+			goto unlock;
+		}
 	}
 
-	ret = nb_written;
+	ret = n;
 
 unlock:
 	sfc_adapter_unlock(sa);
@@ -833,8 +833,7 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 {
 	struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
 	struct sfc_port *port = &sa->port;
-	unsigned int nb_supported = 0;
-	unsigned int nb_written = 0;
+	unsigned int nb_supported;
 	unsigned int i;
 
 	if (unlikely(xstats_names == NULL && ids != NULL) ||
@@ -849,23 +848,24 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 		return nb_supported;
 	}
 
-	for (i = 0; (i < EFX_MAC_NSTATS) && (nb_written < size); ++i) {
-		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
-			continue;
-
-		if (ids[nb_written] == nb_supported) {
-			char *name = xstats_names[nb_written++].name;
+	SFC_ASSERT(port->mac_stats_nb_supported <=
+		   RTE_DIM(port->mac_stats_by_id));
 
-			strlcpy(name, efx_mac_stat_name(sa->nic, i),
+	for (i = 0; i < size; i++) {
+		if (ids[i] < port->mac_stats_nb_supported) {
+			strlcpy(xstats_names[i].name,
+				efx_mac_stat_name(sa->nic,
+						 port->mac_stats_by_id[ids[i]]),
 				sizeof(xstats_names[0].name));
+		} else {
+			sfc_adapter_unlock(sa);
+			return i;
 		}
-
-		++nb_supported;
 	}
 
 	sfc_adapter_unlock(sa);
 
-	return nb_written;
+	return size;
 }
 
 static int
diff --git a/drivers/net/sfc/sfc_port.c b/drivers/net/sfc/sfc_port.c
index cdc0f94f19..bb9e01d96b 100644
--- a/drivers/net/sfc/sfc_port.c
+++ b/drivers/net/sfc/sfc_port.c
@@ -157,6 +157,27 @@ sfc_port_phy_caps_to_max_link_speed(uint32_t phy_caps)
 
 #endif
 
+static void
+sfc_port_fill_mac_stats_info(struct sfc_adapter *sa)
+{
+	unsigned int mac_stats_nb_supported = 0;
+	struct sfc_port *port = &sa->port;
+	unsigned int stat_idx;
+
+	efx_mac_stats_get_mask(sa->nic, port->mac_stats_mask,
+			       sizeof(port->mac_stats_mask));
+
+	for (stat_idx = 0; stat_idx < EFX_MAC_NSTATS; ++stat_idx) {
+		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, stat_idx))
+			continue;
+
+		port->mac_stats_by_id[mac_stats_nb_supported] = stat_idx;
+		mac_stats_nb_supported++;
+	}
+
+	port->mac_stats_nb_supported = mac_stats_nb_supported;
+}
+
 int
 sfc_port_start(struct sfc_adapter *sa)
 {
@@ -165,7 +186,6 @@ sfc_port_start(struct sfc_adapter *sa)
 	uint32_t phy_adv_cap;
 	const uint32_t phy_pause_caps =
 		((1u << EFX_PHY_CAP_PAUSE) | (1u << EFX_PHY_CAP_ASYM));
-	unsigned int i;
 
 	sfc_log_init(sa, "entry");
 
@@ -259,12 +279,7 @@ sfc_port_start(struct sfc_adapter *sa)
 		port->mac_stats_reset_pending = B_FALSE;
 	}
 
-	efx_mac_stats_get_mask(sa->nic, port->mac_stats_mask,
-			       sizeof(port->mac_stats_mask));
-
-	for (i = 0, port->mac_stats_nb_supported = 0; i < EFX_MAC_NSTATS; ++i)
-		if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
-			port->mac_stats_nb_supported++;
+	sfc_port_fill_mac_stats_info(sa);
 
 	port->mac_stats_update_generation = 0;
 
-- 
2.30.2

[dpdk-stable] [PATCH 07/11] net/sfc: fix MAC stats update to work for stopped device

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Fixes: 1caab2f1e68 ("net/sfc: add basic statistics")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc.h        |  2 +-
 drivers/net/sfc/sfc_ethdev.c |  6 +++---
 drivers/net/sfc/sfc_port.c   | 11 +++++++----
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/sfc/sfc.h b/drivers/net/sfc/sfc.h
index 972d32606d..1594f934ba 100644
--- a/drivers/net/sfc/sfc.h
+++ b/drivers/net/sfc/sfc.h
@@ -422,7 +422,7 @@ int sfc_port_start(struct sfc_adapter *sa);
 void sfc_port_stop(struct sfc_adapter *sa);
 void sfc_port_link_mode_to_info(efx_link_mode_t link_mode,
 				struct rte_eth_link *link_info);
-int sfc_port_update_mac_stats(struct sfc_adapter *sa);
+int sfc_port_update_mac_stats(struct sfc_adapter *sa, boolean_t manual_update);
 int sfc_port_reset_mac_stats(struct sfc_adapter *sa);
 int sfc_set_rx_mode(struct sfc_adapter *sa);
 int sfc_set_rx_mode_unchecked(struct sfc_adapter *sa);
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index ae9304f90f..bbc22723f6 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -615,7 +615,7 @@ sfc_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 
 	sfc_adapter_lock(sa);
 
-	ret = sfc_port_update_mac_stats(sa);
+	ret = sfc_port_update_mac_stats(sa, B_FALSE);
 	if (ret != 0)
 		goto unlock;
 
@@ -733,7 +733,7 @@ sfc_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 
 	sfc_adapter_lock(sa);
 
-	rc = sfc_port_update_mac_stats(sa);
+	rc = sfc_port_update_mac_stats(sa, B_FALSE);
 	if (rc != 0) {
 		SFC_ASSERT(rc > 0);
 		nstats = -rc;
@@ -797,7 +797,7 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 
 	sfc_adapter_lock(sa);
 
-	rc = sfc_port_update_mac_stats(sa);
+	rc = sfc_port_update_mac_stats(sa, B_FALSE);
 	if (rc != 0) {
 		SFC_ASSERT(rc > 0);
 		ret = -rc;
diff --git a/drivers/net/sfc/sfc_port.c b/drivers/net/sfc/sfc_port.c
index bb9e01d96b..8c432c15f5 100644
--- a/drivers/net/sfc/sfc_port.c
+++ b/drivers/net/sfc/sfc_port.c
@@ -26,7 +26,8 @@
 /**
  * Update MAC statistics in the buffer.
  *
- * @param	sa	Adapter
+ * @param	sa		Adapter
+ * @param	force_upload	Flag to upload MAC stats in any case
  *
  * @return Status code
  * @retval	0	Success
@@ -34,7 +35,7 @@
  * @retval	ENOMEM	Memory allocation failure
  */
 int
-sfc_port_update_mac_stats(struct sfc_adapter *sa)
+sfc_port_update_mac_stats(struct sfc_adapter *sa, boolean_t force_upload)
 {
 	struct sfc_port *port = &sa->port;
 	efsys_mem_t *esmp = &port->mac_stats_dma_mem;
@@ -46,14 +47,14 @@ sfc_port_update_mac_stats(struct sfc_adapter *sa)
 	SFC_ASSERT(sfc_adapter_is_locked(sa));
 
 	if (sa->state != SFC_ADAPTER_STARTED)
-		return EINVAL;
+		return 0;
 
 	/*
 	 * If periodic statistics DMA'ing is off or if not supported,
 	 * make a manual request and keep an eye on timer if need be
 	 */
 	if (!port->mac_stats_periodic_dma_supported ||
-	    (port->mac_stats_update_period_ms == 0)) {
+	    (port->mac_stats_update_period_ms == 0) || force_upload) {
 		if (port->mac_stats_update_period_ms != 0) {
 			uint64_t timestamp = sfc_get_system_msecs();
 
@@ -367,6 +368,8 @@ sfc_port_stop(struct sfc_adapter *sa)
 	(void)efx_mac_stats_periodic(sa->nic, &sa->port.mac_stats_dma_mem,
 				     0, B_FALSE);
 
+	sfc_port_update_mac_stats(sa, B_TRUE);
+
 	efx_port_fini(sa->nic);
 	efx_filter_fini(sa->nic);
 
-- 
2.30.2

Re: [dpdk-stable] [PATCH 03/11] ethdev: fix docs of functions getting xstats by IDs

[Ferruh Yigit]

On 6/4/2021 3:42 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Document valid combinations of input arguments in accordance with
> current implementation in ethdev.
> 
> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
>  lib/ethdev/rte_ethdev.h | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index faf3bd901d..1f63118544 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -2873,12 +2873,15 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>   *   The port identifier of the Ethernet device.
>   * @param xstats_names
>   *   An rte_eth_xstat_name array of at least *size* elements to
> - *   be filled. If set to NULL, the function returns the required number
> - *   of elements.
> + *   be filled. Must not be NULL if @p ids are specified (not NULL).

Removed part is also valid. If both 'ids' & 'xstats_names' are NULL, API returns
number of all elements.

Addition part looks good.

>   * @param ids
> - *   IDs array given by app to retrieve specific statistics
> + *   IDs array given by app to retrieve specific statistics. May be NULL
> + *   to retrieve all available statistics.

ack

>   * @param size
> - *   The size of the xstats_names array (number of elements).
> + *   If @p ids is not NULL, number of elements in the array with requested IDs
> + *   and number of elements in @p xstats_names to put names in. If @p ids is
> + *   NULL, number of elements in @p xstats_names to put all available statistics
> + *   names in.

ack

>   * @return
>   *   - A positive value lower or equal to size: success. The return value
>   *     is the number of entries filled in the stats table.
> @@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>   *     is too small. The return value corresponds to the size that should
>   *     be given to succeed. The entries in the table are not valid and
>   *     shall not be used by the caller.
> - *   - A negative value on error (invalid port id).
> + *   - A negative value on error.

ack


The 'eth_dev_get_xstats_count()' API is flexible but it makes API unnecessarily
complex, not for this patch but for future perhaps we can update the API and it
can return error if either 'ids' or 'xstats_names' is NULL. Remove support to
get all elements or getting number of elements support, these already supported
by non _id version of API.


And as a note for future, if we ever consider updating these _by_id APIs, we can
consider making the parameter order same for both, currently it is:
"rte_eth_xstats_get_names_by_id(port_id, values, size, ids)"
"      rte_eth_xstats_get_by_id(port_id, ids, values, size)"

>   */
>  int
>  rte_eth_xstats_get_names_by_id(uint16_t port_id,
> @@ -2900,13 +2903,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>   *   The port identifier of the Ethernet device.
>   * @param ids
>   *   A pointer to an ids array passed by application. This tells which
> - *   statistics values function should retrieve. This parameter
> - *   can be set to NULL if size is 0. In this case function will retrieve
> + *   statistics values function should retrieve. May be NULL to retrieve
>   *   all available statistics.

Update is good. But what do you think to make it exact same in the both APIs
('rte_eth_xstats_get_names_by_id()' & 'rte_eth_xstats_get_by_id()')? Since it is
used for same purpose and exact same way in both APIs, no need to have slightly
different description.

>   * @param values
>   *   A pointer to a table to be filled with device statistics values.
> + *   Must not be NULL if ids are specified (not NULL).

Same comment on making description similar in both APIs.

Also both 'ids' & 'values' being NULL returns number of all elements should be
addressed.

>   * @param size
> - *   The size of the ids array (number of elements).
> + *   If @p ids is not NULL, number of elements in the array with requested IDs
> + *   and number of elements in values to put statistics in. If @p ids is NULL,
> + *   number of elements in values to put all available statistics in.

ack

>   * @return
>   *   - A positive value lower or equal to size: success. The return value
>   *     is the number of entries filled in the stats table.
> @@ -2914,7 +2919,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>   *     is too small. The return value corresponds to the size that should
>   *     be given to succeed. The entries in the table are not valid and
>   *     shall not be used by the caller.
> - *   - A negative value on error (invalid port id).
> + *   - A negative value on error.

ack

>   */
>  int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
>  			     uint64_t *values, unsigned int size);
> 

Re: [dpdk-stable] [PATCH 04/11] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Ferruh Yigit]

On 6/4/2021 3:42 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Update xstats by IDs callbacks documentation in accordance with
> ethdev usage of these callbacks. Document valid combinations of
> input arguments to make driver implementation simpler.
> 
> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
>  lib/ethdev/ethdev_driver.h | 43 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 40e474aa7e..fd5b7ca550 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat *stats, unsigned int n);
>  /**< @internal Get extended stats of an Ethernet device. */
>  
> +/**
> + * @internal
> + * Get extended stats of an Ethernet device.

Should it mention _by_id detail?

> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param ids
> + *   IDs array to retrieve specific statistics. Must not be NULL.
> + * @param values
> + *   A pointer to a table to be filled with device statistics values.
> + *   Must not be NULL.
> + * @param n
> + *   Element count in @p ids and @p values
> + *
> + * @return
> + *   - A number of filled in stats.
> + *   - A negative value on error.
> + */
>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>  				      const uint64_t *ids,
>  				      uint64_t *values,
>  				      unsigned int n);
> -/**< @internal Get extended stats of an Ethernet device. */
>  
>  /**
>   * @internal
> @@ -218,10 +235,32 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>  /**< @internal Get names of extended stats of an Ethernet device. */
>  
> +/**
> + * @internal
> + * Get names of extended stats of an Ethernet device.

Should it mention _by_id detail?

> + * For name count, set @p xstats_names and @p ids to NULL.
> + *

isn't the 'count' part handled in the API? I think in the devops both should not
be NULL.

> + * @param dev
> + *   ethdev handle of port.
> + * @param xstats_names
> + *   An rte_eth_xstat_name array of at least *size* elements to
> + *   be filled. Can be NULL together with @p ids to retrieve number of
> + *   available statistics.

As far as I understand both _by_id APIs and devops behave same, so argument
descriptions/behavior should be same.

> + * @param ids
> + *   IDs array to retrieve specific statistics. Can be NULL together
> + *   with @p xstats_names to retrieve number of available statistics.
> + * @param size
> + *   Size of ids and xstats_names arrays.
> + *   Element count in @p ids and @p xstats_names
> + *
> + * @return
> + *   - A number of filled in stats if both xstats_names and ids are not NULL.
> + *   - A number of available stats if both xstats_names and ids are NULL.

Again as far as I can see these covered by API, not devops, am I missing something.

> + *   - A negative value on error.
> + */
>  typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
>  	unsigned int size);
> -/**< @internal Get names of extended stats of an Ethernet device. */
>  
>  typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
>  					     uint16_t queue_id,
> 

Re: [dpdk-stable] [PATCH 03/11] ethdev: fix docs of functions getting xstats by IDs

[Andrew Rybchenko]

On 7/20/21 7:25 PM, Ferruh Yigit wrote:
> On 6/4/2021 3:42 PM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> Document valid combinations of input arguments in accordance with
>> current implementation in ethdev.
>>
>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>> ---
>>   lib/ethdev/rte_ethdev.h | 23 ++++++++++++++---------
>>   1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index faf3bd901d..1f63118544 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -2873,12 +2873,15 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>>    *   The port identifier of the Ethernet device.
>>    * @param xstats_names
>>    *   An rte_eth_xstat_name array of at least *size* elements to
>> - *   be filled. If set to NULL, the function returns the required number
>> - *   of elements.
>> + *   be filled. Must not be NULL if @p ids are specified (not NULL).
> 
> Removed part is also valid. If both 'ids' & 'xstats_names' are NULL, API returns
> number of all elements.

Yes, but it is an excessive information. The trigger to return number
of all elements is 'ids == NULL'. Here we are talking about
'xstats_names' parameter. If the parameter is NULL, but ids is not
null, it does not trigger number of all elements return. It is an
invalid input parameters. That's what a new description says.

> 
> Addition part looks good.
> 
>>    * @param ids
>> - *   IDs array given by app to retrieve specific statistics
>> + *   IDs array given by app to retrieve specific statistics. May be NULL
>> + *   to retrieve all available statistics.
> 
> ack
> 
>>    * @param size
>> - *   The size of the xstats_names array (number of elements).
>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>> + *   and number of elements in @p xstats_names to put names in. If @p ids is
>> + *   NULL, number of elements in @p xstats_names to put all available statistics
>> + *   names in.
> 
> ack
> 
>>    * @return
>>    *   - A positive value lower or equal to size: success. The return value
>>    *     is the number of entries filled in the stats table.
>> @@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>>    *     is too small. The return value corresponds to the size that should
>>    *     be given to succeed. The entries in the table are not valid and
>>    *     shall not be used by the caller.
>> - *   - A negative value on error (invalid port id).
>> + *   - A negative value on error.
> 
> ack
> 
> 
> The 'eth_dev_get_xstats_count()' API is flexible but it makes API unnecessarily
> complex, not for this patch but for future perhaps we can update the API and it
> can return error if either 'ids' or 'xstats_names' is NULL. Remove support to
> get all elements or getting number of elements support, these already supported
> by non _id version of API.

I'm not sure that it is a right direction. The support allows
application to use less number of functions and depend on less
number of function prototypes.

> And as a note for future, if we ever consider updating these _by_id APIs, we can
> consider making the parameter order same for both, currently it is:
> "rte_eth_xstats_get_names_by_id(port_id, values, size, ids)"
> "      rte_eth_xstats_get_by_id(port_id, ids, values, size)"

+1, current difference is terribly bad

>>    */
>>   int
>>   rte_eth_xstats_get_names_by_id(uint16_t port_id,
>> @@ -2900,13 +2903,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>    *   The port identifier of the Ethernet device.
>>    * @param ids
>>    *   A pointer to an ids array passed by application. This tells which
>> - *   statistics values function should retrieve. This parameter
>> - *   can be set to NULL if size is 0. In this case function will retrieve
>> + *   statistics values function should retrieve. May be NULL to retrieve
>>    *   all available statistics.
> 
> Update is good. But what do you think to make it exact same in the both APIs
> ('rte_eth_xstats_get_names_by_id()' & 'rte_eth_xstats_get_by_id()')? Since it is
> used for same purpose and exact same way in both APIs, no need to have slightly
> different description.

I agree. I'll fix in v2.

>>    * @param values
>>    *   A pointer to a table to be filled with device statistics values.
>> + *   Must not be NULL if ids are specified (not NULL).
> 
> Same comment on making description similar in both APIs.

OK

> Also both 'ids' & 'values' being NULL returns number of all elements should be
> addressed.

I think it is excessibe. It is sufficient to say so for ids==NULL which
is a trigger to get all elements.

>>    * @param size
>> - *   The size of the ids array (number of elements).
>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>> + *   and number of elements in values to put statistics in. If @p ids is NULL,
>> + *   number of elements in values to put all available statistics in.
> 
> ack
> 
>>    * @return
>>    *   - A positive value lower or equal to size: success. The return value
>>    *     is the number of entries filled in the stats table.
>> @@ -2914,7 +2919,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>    *     is too small. The return value corresponds to the size that should
>>    *     be given to succeed. The entries in the table are not valid and
>>    *     shall not be used by the caller.
>> - *   - A negative value on error (invalid port id).
>> + *   - A negative value on error.
> 
> ack
> 
>>    */
>>   int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
>>   			     uint64_t *values, unsigned int size);
>>

Re: [dpdk-stable] [PATCH 04/11] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

On 7/20/21 7:51 PM, Ferruh Yigit wrote:
> On 6/4/2021 3:42 PM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> Update xstats by IDs callbacks documentation in accordance with
>> ethdev usage of these callbacks. Document valid combinations of
>> input arguments to make driver implementation simpler.
>>
>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>> ---
>>   lib/ethdev/ethdev_driver.h | 43 ++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>> index 40e474aa7e..fd5b7ca550 100644
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>   	struct rte_eth_xstat *stats, unsigned int n);
>>   /**< @internal Get extended stats of an Ethernet device. */
>>   
>> +/**
>> + * @internal
>> + * Get extended stats of an Ethernet device.
> 
> Should it mention _by_id detail?

Yes, will fix in v2.

>> + *
>> + * @param dev
>> + *   ethdev handle of port.
>> + * @param ids
>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>> + * @param values
>> + *   A pointer to a table to be filled with device statistics values.
>> + *   Must not be NULL.
>> + * @param n
>> + *   Element count in @p ids and @p values
>> + *
>> + * @return
>> + *   - A number of filled in stats.
>> + *   - A negative value on error.
>> + */
>>   typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>   				      const uint64_t *ids,
>>   				      uint64_t *values,
>>   				      unsigned int n);
>> -/**< @internal Get extended stats of an Ethernet device. */
>>   
>>   /**
>>    * @internal
>> @@ -218,10 +235,32 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>   	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>   /**< @internal Get names of extended stats of an Ethernet device. */
>>   
>> +/**
>> + * @internal
>> + * Get names of extended stats of an Ethernet device.
> 
> Should it mention _by_id detail?

Yes, will fix in v2.

>> + * For name count, set @p xstats_names and @p ids to NULL.
>> + *
> 
> isn't the 'count' part handled in the API? I think in the devops both should not
> be NULL.

No, eth_dev_get_xstats_count() uses the callback with NULL, NULL, 0.

> 
>> + * @param dev
>> + *   ethdev handle of port.
>> + * @param xstats_names
>> + *   An rte_eth_xstat_name array of at least *size* elements to
>> + *   be filled. Can be NULL together with @p ids to retrieve number of
>> + *   available statistics.
> 
> As far as I understand both _by_id APIs and devops behave same, so argument
> descriptions/behavior should be same.

In fact no, it is slightly different. For example, devops is never
called with NULL ids and not NULL names or non-zero size. It allows to
check less in drivers.

>> + * @param ids
>> + *   IDs array to retrieve specific statistics. Can be NULL together
>> + *   with @p xstats_names to retrieve number of available statistics.
>> + * @param size
>> + *   Size of ids and xstats_names arrays.
>> + *   Element count in @p ids and @p xstats_names
>> + *
>> + * @return
>> + *   - A number of filled in stats if both xstats_names and ids are not NULL.
>> + *   - A number of available stats if both xstats_names and ids are NULL.
> 
> Again as far as I can see these covered by API, not devops, am I missing something.

See eth_dev_get_xstats_count()

> 
>> + *   - A negative value on error.
>> + */
>>   typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
>>   	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
>>   	unsigned int size);
>> -/**< @internal Get names of extended stats of an Ethernet device. */
>>   
>>   typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
>>   					     uint16_t queue_id,
>>

[dpdk-stable] [PATCH v2 01/11] net/sfc: fix get xstats by ID callback to use MAC stats lock

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Add MAC stats lock in get xstats by id callback before reading
number of supported MAC stats.

Fixes: 73280c1e4ff ("net/sfc: support xstats retrieval by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc_ethdev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 88896db1f8..d4ac61ff76 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -789,12 +789,14 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	int ret;
 	int rc;
 
-	if (unlikely(values == NULL) ||
-	    unlikely((ids == NULL) && (n < port->mac_stats_nb_supported)))
-		return port->mac_stats_nb_supported;
-
 	rte_spinlock_lock(&port->mac_stats_lock);
 
+	if (unlikely(values == NULL) ||
+	    unlikely(ids == NULL && n < port->mac_stats_nb_supported)) {
+		ret = port->mac_stats_nb_supported;
+		goto unlock;
+	}
+
 	rc = sfc_port_update_mac_stats(sa);
 	if (rc != 0) {
 		SFC_ASSERT(rc > 0);
-- 
2.30.2

[dpdk-stable] [PATCH v2 02/11] net/sfc: fix reading adapter state without locking

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update MAC stats function reads adapter state with MAC stats locking
but without adapter locking. Add adapter locking before calling this
function and remove MAC stats locking since there's no point to have
it together with adapter locking. The second place MAC stats locking
is used is MAC stats reset function. It's called with adapter being
already locked so there's no point to use MAC stats locking anymore.

Fixes: 1caab2f1e68 ("net/sfc: add basic statistics")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc.h        |  1 -
 drivers/net/sfc/sfc_ethdev.c | 28 ++++++++++++++++++++--------
 drivers/net/sfc/sfc_port.c   |  9 +++------
 3 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/net/sfc/sfc.h b/drivers/net/sfc/sfc.h
index 546739bd4a..c7b0e5a30d 100644
--- a/drivers/net/sfc/sfc.h
+++ b/drivers/net/sfc/sfc.h
@@ -130,7 +130,6 @@ struct sfc_port {
 	unsigned int			nb_mcast_addrs;
 	uint8_t				*mcast_addrs;
 
-	rte_spinlock_t			mac_stats_lock;
 	uint64_t			*mac_stats_buf;
 	unsigned int			mac_stats_nb_supported;
 	efsys_mem_t			mac_stats_dma_mem;
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index d4ac61ff76..d5417e5e65 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -613,7 +613,7 @@ sfc_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 	uint64_t *mac_stats;
 	int ret;
 
-	rte_spinlock_lock(&port->mac_stats_lock);
+	sfc_adapter_lock(sa);
 
 	ret = sfc_port_update_mac_stats(sa);
 	if (ret != 0)
@@ -686,7 +686,7 @@ sfc_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 	}
 
 unlock:
-	rte_spinlock_unlock(&port->mac_stats_lock);
+	sfc_adapter_unlock(sa);
 	SFC_ASSERT(ret >= 0);
 	return -ret;
 }
@@ -698,12 +698,15 @@ sfc_stats_reset(struct rte_eth_dev *dev)
 	struct sfc_port *port = &sa->port;
 	int rc;
 
+	sfc_adapter_lock(sa);
+
 	if (sa->state != SFC_ADAPTER_STARTED) {
 		/*
 		 * The operation cannot be done if port is not started; it
 		 * will be scheduled to be done during the next port start
 		 */
 		port->mac_stats_reset_pending = B_TRUE;
+		sfc_adapter_unlock(sa);
 		return 0;
 	}
 
@@ -711,6 +714,8 @@ sfc_stats_reset(struct rte_eth_dev *dev)
 	if (rc != 0)
 		sfc_err(sa, "failed to reset statistics (rc = %d)", rc);
 
+	sfc_adapter_unlock(sa);
+
 	SFC_ASSERT(rc >= 0);
 	return -rc;
 }
@@ -726,7 +731,7 @@ sfc_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 	unsigned int i;
 	int nstats = 0;
 
-	rte_spinlock_lock(&port->mac_stats_lock);
+	sfc_adapter_lock(sa);
 
 	rc = sfc_port_update_mac_stats(sa);
 	if (rc != 0) {
@@ -748,7 +753,7 @@ sfc_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 	}
 
 unlock:
-	rte_spinlock_unlock(&port->mac_stats_lock);
+	sfc_adapter_unlock(sa);
 
 	return nstats;
 }
@@ -789,7 +794,7 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	int ret;
 	int rc;
 
-	rte_spinlock_lock(&port->mac_stats_lock);
+	sfc_adapter_lock(sa);
 
 	if (unlikely(values == NULL) ||
 	    unlikely(ids == NULL && n < port->mac_stats_nb_supported)) {
@@ -819,7 +824,7 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	ret = nb_written;
 
 unlock:
-	rte_spinlock_unlock(&port->mac_stats_lock);
+	sfc_adapter_unlock(sa);
 
 	return ret;
 }
@@ -835,9 +840,14 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 	unsigned int nb_written = 0;
 	unsigned int i;
 
+	sfc_adapter_lock(sa);
+
 	if (unlikely(xstats_names == NULL) ||
-	    unlikely((ids == NULL) && (size < port->mac_stats_nb_supported)))
-		return port->mac_stats_nb_supported;
+	    unlikely((ids == NULL) && (size < port->mac_stats_nb_supported))) {
+		nb_supported = port->mac_stats_nb_supported;
+		sfc_adapter_unlock(sa);
+		return nb_supported;
+	}
 
 	for (i = 0; (i < EFX_MAC_NSTATS) && (nb_written < size); ++i) {
 		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
@@ -853,6 +863,8 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 		++nb_supported;
 	}
 
+	sfc_adapter_unlock(sa);
+
 	return nb_written;
 }
 
diff --git a/drivers/net/sfc/sfc_port.c b/drivers/net/sfc/sfc_port.c
index ac117f9c48..cdc0f94f19 100644
--- a/drivers/net/sfc/sfc_port.c
+++ b/drivers/net/sfc/sfc_port.c
@@ -43,7 +43,7 @@ sfc_port_update_mac_stats(struct sfc_adapter *sa)
 	unsigned int nb_attempts = 0;
 	int rc;
 
-	SFC_ASSERT(rte_spinlock_is_locked(&port->mac_stats_lock));
+	SFC_ASSERT(sfc_adapter_is_locked(sa));
 
 	if (sa->state != SFC_ADAPTER_STARTED)
 		return EINVAL;
@@ -103,14 +103,13 @@ sfc_port_reset_sw_stats(struct sfc_adapter *sa)
 int
 sfc_port_reset_mac_stats(struct sfc_adapter *sa)
 {
-	struct sfc_port *port = &sa->port;
 	int rc;
 
-	rte_spinlock_lock(&port->mac_stats_lock);
+	SFC_ASSERT(sfc_adapter_is_locked(sa));
+
 	rc = efx_mac_stats_clear(sa->nic);
 	if (rc == 0)
 		sfc_port_reset_sw_stats(sa);
-	rte_spinlock_unlock(&port->mac_stats_lock);
 
 	return rc;
 }
@@ -416,8 +415,6 @@ sfc_port_attach(struct sfc_adapter *sa)
 		goto fail_mcast_addr_list_buf_alloc;
 	}
 
-	rte_spinlock_init(&port->mac_stats_lock);
-
 	rc = ENOMEM;
 	port->mac_stats_buf = rte_calloc_socket("mac_stats_buf", EFX_MAC_NSTATS,
 						sizeof(uint64_t), 0,
-- 
2.30.2

[dpdk-stable] [PATCH v2 03/11] ethdev: fix docs of functions getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Document valid combinations of input arguments in accordance with
current implementation in ethdev.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/rte_ethdev.h | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index d2b27c351f..80c42d2f08 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2872,13 +2872,16 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param xstats_names
- *   An rte_eth_xstat_name array of at least *size* elements to
- *   be filled. If set to NULL, the function returns the required number
- *   of elements.
+ *   An array of at least @p size elements to be filled in.
+ *   Must not be NULL if @p ids are specified (not NULL) or @p size is not 0.
  * @param ids
- *   IDs array given by app to retrieve specific statistics
+ *   IDs array given by app to retrieve specific statistics names.
+ *   May be NULL to retrieve all available statistics names.
  * @param size
- *   The size of the xstats_names array (number of elements).
+ *   If @p ids is not NULL, number of elements in the array with requested IDs
+ *   and number of elements in @p xstats_names to put names in. If @p ids is
+ *   NULL, number of elements in @p xstats_names to put all available statistics
+ *   names in.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
@@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int
 rte_eth_xstats_get_names_by_id(uint16_t port_id,
@@ -2899,14 +2902,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param ids
- *   A pointer to an ids array passed by application. This tells which
- *   statistics values function should retrieve. This parameter
- *   can be set to NULL if size is 0. In this case function will retrieve
- *   all available statistics.
+ *   IDs array given by app to retrieve specific statistics.
+ *   May be NULL to retrieve all available statistics.
  * @param values
- *   A pointer to a table to be filled with device statistics values.
+ *   An array of at least @p size elements to be filled in.
+ *   Must not be NULL if @p ids are specified (not NULL) or @p size is not 0.
  * @param size
- *   The size of the ids array (number of elements).
+ *   If @p ids is not NULL, number of elements in the array with requested IDs
+ *   and number of elements in values to put statistics in. If @p ids is NULL,
+ *   number of elements in values to put all available statistics in.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
@@ -2914,7 +2918,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 			     uint64_t *values, unsigned int size);
-- 
2.30.2

[dpdk-stable] [PATCH v2 04/11] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update xstats by IDs callbacks documentation in accordance with
ethdev usage of these callbacks. Document valid combinations of
input arguments to make driver implementation simpler.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/ethdev_driver.h | 43 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 40e474aa7e..e934be9285 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat *stats, unsigned int n);
 /**< @internal Get extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get extended stats specified by IDs of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param values
+ *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL.
+ * @param n
+ *   Element count in @p ids and @p values
+ *
+ * @return
+ *   - A number of filled in stats.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
 				      const uint64_t *ids,
 				      uint64_t *values,
 				      unsigned int n);
-/**< @internal Get extended stats of an Ethernet device. */
 
 /**
  * @internal
@@ -218,10 +235,32 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, unsigned int size);
 /**< @internal Get names of extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get names of extended stats specified by IDs of an Ethernet device.
+ * For name count, set @p xstats_names and @p ids to NULL.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. Can be NULL together with @p ids to retrieve number of
+ *   available statistics.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Can be NULL together
+ *   with @p xstats_names to retrieve number of available statistics.
+ * @param size
+ *   Size of ids and xstats_names arrays.
+ *   Element count in @p ids and @p xstats_names
+ *
+ * @return
+ *   - A number of filled in stats if both xstats_names and ids are not NULL.
+ *   - A number of available stats if both xstats_names and ids are NULL.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
 	unsigned int size);
-/**< @internal Get names of extended stats of an Ethernet device. */
 
 typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
 					     uint16_t queue_id,
-- 
2.30.2

[dpdk-stable] [PATCH v2 05/11] net/sfc: fix xstats by ID callbacks according to ethdev

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Fix xstats by ID callbacks according to ethdev usage.
Handle combinations of input arguments that are required by ethdev
and sanity check and reject other combinations on callback entry.

Fixes: 73280c1e4ff ("net/sfc: support xstats retrieval by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc_ethdev.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index d5417e5e65..fca3f524a1 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -794,13 +794,10 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	int ret;
 	int rc;
 
-	sfc_adapter_lock(sa);
+	if (unlikely(ids == NULL || values == NULL))
+		return -EINVAL;
 
-	if (unlikely(values == NULL) ||
-	    unlikely(ids == NULL && n < port->mac_stats_nb_supported)) {
-		ret = port->mac_stats_nb_supported;
-		goto unlock;
-	}
+	sfc_adapter_lock(sa);
 
 	rc = sfc_port_update_mac_stats(sa);
 	if (rc != 0) {
@@ -815,7 +812,7 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
 			continue;
 
-		if ((ids == NULL) || (ids[nb_written] == nb_supported))
+		if (ids[nb_written] == nb_supported)
 			values[nb_written++] = mac_stats[i];
 
 		++nb_supported;
@@ -840,10 +837,13 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 	unsigned int nb_written = 0;
 	unsigned int i;
 
+	if (unlikely(xstats_names == NULL && ids != NULL) ||
+	    unlikely(xstats_names != NULL && ids == NULL))
+		return -EINVAL;
+
 	sfc_adapter_lock(sa);
 
-	if (unlikely(xstats_names == NULL) ||
-	    unlikely((ids == NULL) && (size < port->mac_stats_nb_supported))) {
+	if (unlikely(xstats_names == NULL && ids == NULL)) {
 		nb_supported = port->mac_stats_nb_supported;
 		sfc_adapter_unlock(sa);
 		return nb_supported;
@@ -853,7 +853,7 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
 			continue;
 
-		if ((ids == NULL) || (ids[nb_written] == nb_supported)) {
+		if (ids[nb_written] == nb_supported) {
 			char *name = xstats_names[nb_written++].name;
 
 			strlcpy(name, efx_mac_stat_name(sa->nic, i),
-- 
2.30.2

[dpdk-stable] [PATCH v2 06/11] net/sfc: fix accessing xstats by an unsorted list of IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Device may support only some MAC stats. Add mapping from ids to subset
of supported MAC stats for each port.

Fixes: 73280c1e4ff ("net/sfc: support xstats retrieval by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc.h        |  2 ++
 drivers/net/sfc/sfc_ethdev.c | 44 ++++++++++++++++++------------------
 drivers/net/sfc/sfc_port.c   | 29 ++++++++++++++++++------
 3 files changed, 46 insertions(+), 29 deletions(-)

diff --git a/drivers/net/sfc/sfc.h b/drivers/net/sfc/sfc.h
index c7b0e5a30d..972d32606d 100644
--- a/drivers/net/sfc/sfc.h
+++ b/drivers/net/sfc/sfc.h
@@ -141,6 +141,8 @@ struct sfc_port {
 
 	uint32_t		mac_stats_mask[EFX_MAC_STATS_MASK_NPAGES];
 
+	unsigned int			mac_stats_by_id[EFX_MAC_NSTATS];
+
 	uint64_t			ipackets;
 };
 
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index fca3f524a1..ae9304f90f 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -788,8 +788,6 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
 	struct sfc_port *port = &sa->port;
 	uint64_t *mac_stats;
-	unsigned int nb_supported = 0;
-	unsigned int nb_written = 0;
 	unsigned int i;
 	int ret;
 	int rc;
@@ -808,17 +806,19 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 
 	mac_stats = port->mac_stats_buf;
 
-	for (i = 0; (i < EFX_MAC_NSTATS) && (nb_written < n); ++i) {
-		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
-			continue;
-
-		if (ids[nb_written] == nb_supported)
-			values[nb_written++] = mac_stats[i];
+	SFC_ASSERT(port->mac_stats_nb_supported <=
+		   RTE_DIM(port->mac_stats_by_id));
 
-		++nb_supported;
+	for (i = 0; i < n; i++) {
+		if (ids[i] < port->mac_stats_nb_supported) {
+			values[i] = mac_stats[port->mac_stats_by_id[ids[i]]];
+		} else {
+			ret = i;
+			goto unlock;
+		}
 	}
 
-	ret = nb_written;
+	ret = n;
 
 unlock:
 	sfc_adapter_unlock(sa);
@@ -833,8 +833,7 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 {
 	struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
 	struct sfc_port *port = &sa->port;
-	unsigned int nb_supported = 0;
-	unsigned int nb_written = 0;
+	unsigned int nb_supported;
 	unsigned int i;
 
 	if (unlikely(xstats_names == NULL && ids != NULL) ||
@@ -849,23 +848,24 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 		return nb_supported;
 	}
 
-	for (i = 0; (i < EFX_MAC_NSTATS) && (nb_written < size); ++i) {
-		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
-			continue;
-
-		if (ids[nb_written] == nb_supported) {
-			char *name = xstats_names[nb_written++].name;
+	SFC_ASSERT(port->mac_stats_nb_supported <=
+		   RTE_DIM(port->mac_stats_by_id));
 
-			strlcpy(name, efx_mac_stat_name(sa->nic, i),
+	for (i = 0; i < size; i++) {
+		if (ids[i] < port->mac_stats_nb_supported) {
+			strlcpy(xstats_names[i].name,
+				efx_mac_stat_name(sa->nic,
+						 port->mac_stats_by_id[ids[i]]),
 				sizeof(xstats_names[0].name));
+		} else {
+			sfc_adapter_unlock(sa);
+			return i;
 		}
-
-		++nb_supported;
 	}
 
 	sfc_adapter_unlock(sa);
 
-	return nb_written;
+	return size;
 }
 
 static int
diff --git a/drivers/net/sfc/sfc_port.c b/drivers/net/sfc/sfc_port.c
index cdc0f94f19..bb9e01d96b 100644
--- a/drivers/net/sfc/sfc_port.c
+++ b/drivers/net/sfc/sfc_port.c
@@ -157,6 +157,27 @@ sfc_port_phy_caps_to_max_link_speed(uint32_t phy_caps)
 
 #endif
 
+static void
+sfc_port_fill_mac_stats_info(struct sfc_adapter *sa)
+{
+	unsigned int mac_stats_nb_supported = 0;
+	struct sfc_port *port = &sa->port;
+	unsigned int stat_idx;
+
+	efx_mac_stats_get_mask(sa->nic, port->mac_stats_mask,
+			       sizeof(port->mac_stats_mask));
+
+	for (stat_idx = 0; stat_idx < EFX_MAC_NSTATS; ++stat_idx) {
+		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, stat_idx))
+			continue;
+
+		port->mac_stats_by_id[mac_stats_nb_supported] = stat_idx;
+		mac_stats_nb_supported++;
+	}
+
+	port->mac_stats_nb_supported = mac_stats_nb_supported;
+}
+
 int
 sfc_port_start(struct sfc_adapter *sa)
 {
@@ -165,7 +186,6 @@ sfc_port_start(struct sfc_adapter *sa)
 	uint32_t phy_adv_cap;
 	const uint32_t phy_pause_caps =
 		((1u << EFX_PHY_CAP_PAUSE) | (1u << EFX_PHY_CAP_ASYM));
-	unsigned int i;
 
 	sfc_log_init(sa, "entry");
 
@@ -259,12 +279,7 @@ sfc_port_start(struct sfc_adapter *sa)
 		port->mac_stats_reset_pending = B_FALSE;
 	}
 
-	efx_mac_stats_get_mask(sa->nic, port->mac_stats_mask,
-			       sizeof(port->mac_stats_mask));
-
-	for (i = 0, port->mac_stats_nb_supported = 0; i < EFX_MAC_NSTATS; ++i)
-		if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
-			port->mac_stats_nb_supported++;
+	sfc_port_fill_mac_stats_info(sa);
 
 	port->mac_stats_update_generation = 0;
 
-- 
2.30.2

[dpdk-stable] [PATCH v2 07/11] net/sfc: fix MAC stats update to work for stopped device

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Fixes: 1caab2f1e68 ("net/sfc: add basic statistics")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc.h        |  2 +-
 drivers/net/sfc/sfc_ethdev.c |  6 +++---
 drivers/net/sfc/sfc_port.c   | 11 +++++++----
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/sfc/sfc.h b/drivers/net/sfc/sfc.h
index 972d32606d..1594f934ba 100644
--- a/drivers/net/sfc/sfc.h
+++ b/drivers/net/sfc/sfc.h
@@ -422,7 +422,7 @@ int sfc_port_start(struct sfc_adapter *sa);
 void sfc_port_stop(struct sfc_adapter *sa);
 void sfc_port_link_mode_to_info(efx_link_mode_t link_mode,
 				struct rte_eth_link *link_info);
-int sfc_port_update_mac_stats(struct sfc_adapter *sa);
+int sfc_port_update_mac_stats(struct sfc_adapter *sa, boolean_t manual_update);
 int sfc_port_reset_mac_stats(struct sfc_adapter *sa);
 int sfc_set_rx_mode(struct sfc_adapter *sa);
 int sfc_set_rx_mode_unchecked(struct sfc_adapter *sa);
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index ae9304f90f..bbc22723f6 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -615,7 +615,7 @@ sfc_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 
 	sfc_adapter_lock(sa);
 
-	ret = sfc_port_update_mac_stats(sa);
+	ret = sfc_port_update_mac_stats(sa, B_FALSE);
 	if (ret != 0)
 		goto unlock;
 
@@ -733,7 +733,7 @@ sfc_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 
 	sfc_adapter_lock(sa);
 
-	rc = sfc_port_update_mac_stats(sa);
+	rc = sfc_port_update_mac_stats(sa, B_FALSE);
 	if (rc != 0) {
 		SFC_ASSERT(rc > 0);
 		nstats = -rc;
@@ -797,7 +797,7 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 
 	sfc_adapter_lock(sa);
 
-	rc = sfc_port_update_mac_stats(sa);
+	rc = sfc_port_update_mac_stats(sa, B_FALSE);
 	if (rc != 0) {
 		SFC_ASSERT(rc > 0);
 		ret = -rc;
diff --git a/drivers/net/sfc/sfc_port.c b/drivers/net/sfc/sfc_port.c
index bb9e01d96b..8c432c15f5 100644
--- a/drivers/net/sfc/sfc_port.c
+++ b/drivers/net/sfc/sfc_port.c
@@ -26,7 +26,8 @@
 /**
  * Update MAC statistics in the buffer.
  *
- * @param	sa	Adapter
+ * @param	sa		Adapter
+ * @param	force_upload	Flag to upload MAC stats in any case
  *
  * @return Status code
  * @retval	0	Success
@@ -34,7 +35,7 @@
  * @retval	ENOMEM	Memory allocation failure
  */
 int
-sfc_port_update_mac_stats(struct sfc_adapter *sa)
+sfc_port_update_mac_stats(struct sfc_adapter *sa, boolean_t force_upload)
 {
 	struct sfc_port *port = &sa->port;
 	efsys_mem_t *esmp = &port->mac_stats_dma_mem;
@@ -46,14 +47,14 @@ sfc_port_update_mac_stats(struct sfc_adapter *sa)
 	SFC_ASSERT(sfc_adapter_is_locked(sa));
 
 	if (sa->state != SFC_ADAPTER_STARTED)
-		return EINVAL;
+		return 0;
 
 	/*
 	 * If periodic statistics DMA'ing is off or if not supported,
 	 * make a manual request and keep an eye on timer if need be
 	 */
 	if (!port->mac_stats_periodic_dma_supported ||
-	    (port->mac_stats_update_period_ms == 0)) {
+	    (port->mac_stats_update_period_ms == 0) || force_upload) {
 		if (port->mac_stats_update_period_ms != 0) {
 			uint64_t timestamp = sfc_get_system_msecs();
 
@@ -367,6 +368,8 @@ sfc_port_stop(struct sfc_adapter *sa)
 	(void)efx_mac_stats_periodic(sa->nic, &sa->port.mac_stats_dma_mem,
 				     0, B_FALSE);
 
+	sfc_port_update_mac_stats(sa, B_TRUE);
+
 	efx_port_fini(sa->nic);
 	efx_filter_fini(sa->nic);
 
-- 
2.30.2

[dpdk-stable] [PATCH v3 01/11] net/sfc: fix get xstats by ID callback to use MAC stats lock

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Add MAC stats lock in get xstats by id callback before reading
number of supported MAC stats.

Fixes: 73280c1e4ff ("net/sfc: support xstats retrieval by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc_ethdev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 88896db1f8..d4ac61ff76 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -789,12 +789,14 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	int ret;
 	int rc;
 
-	if (unlikely(values == NULL) ||
-	    unlikely((ids == NULL) && (n < port->mac_stats_nb_supported)))
-		return port->mac_stats_nb_supported;
-
 	rte_spinlock_lock(&port->mac_stats_lock);
 
+	if (unlikely(values == NULL) ||
+	    unlikely(ids == NULL && n < port->mac_stats_nb_supported)) {
+		ret = port->mac_stats_nb_supported;
+		goto unlock;
+	}
+
 	rc = sfc_port_update_mac_stats(sa);
 	if (rc != 0) {
 		SFC_ASSERT(rc > 0);
-- 
2.30.2

[dpdk-stable] [PATCH v3 02/11] net/sfc: fix reading adapter state without locking

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update MAC stats function reads adapter state with MAC stats locking
but without adapter locking. Add adapter locking before calling this
function and remove MAC stats locking since there's no point to have
it together with adapter locking. The second place MAC stats locking
is used is MAC stats reset function. It's called with adapter being
already locked so there's no point to use MAC stats locking anymore.

Fixes: 1caab2f1e68 ("net/sfc: add basic statistics")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc.h        |  1 -
 drivers/net/sfc/sfc_ethdev.c | 28 ++++++++++++++++++++--------
 drivers/net/sfc/sfc_port.c   |  9 +++------
 3 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/net/sfc/sfc.h b/drivers/net/sfc/sfc.h
index 546739bd4a..c7b0e5a30d 100644
--- a/drivers/net/sfc/sfc.h
+++ b/drivers/net/sfc/sfc.h
@@ -130,7 +130,6 @@ struct sfc_port {
 	unsigned int			nb_mcast_addrs;
 	uint8_t				*mcast_addrs;
 
-	rte_spinlock_t			mac_stats_lock;
 	uint64_t			*mac_stats_buf;
 	unsigned int			mac_stats_nb_supported;
 	efsys_mem_t			mac_stats_dma_mem;
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index d4ac61ff76..d5417e5e65 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -613,7 +613,7 @@ sfc_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 	uint64_t *mac_stats;
 	int ret;
 
-	rte_spinlock_lock(&port->mac_stats_lock);
+	sfc_adapter_lock(sa);
 
 	ret = sfc_port_update_mac_stats(sa);
 	if (ret != 0)
@@ -686,7 +686,7 @@ sfc_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 	}
 
 unlock:
-	rte_spinlock_unlock(&port->mac_stats_lock);
+	sfc_adapter_unlock(sa);
 	SFC_ASSERT(ret >= 0);
 	return -ret;
 }
@@ -698,12 +698,15 @@ sfc_stats_reset(struct rte_eth_dev *dev)
 	struct sfc_port *port = &sa->port;
 	int rc;
 
+	sfc_adapter_lock(sa);
+
 	if (sa->state != SFC_ADAPTER_STARTED) {
 		/*
 		 * The operation cannot be done if port is not started; it
 		 * will be scheduled to be done during the next port start
 		 */
 		port->mac_stats_reset_pending = B_TRUE;
+		sfc_adapter_unlock(sa);
 		return 0;
 	}
 
@@ -711,6 +714,8 @@ sfc_stats_reset(struct rte_eth_dev *dev)
 	if (rc != 0)
 		sfc_err(sa, "failed to reset statistics (rc = %d)", rc);
 
+	sfc_adapter_unlock(sa);
+
 	SFC_ASSERT(rc >= 0);
 	return -rc;
 }
@@ -726,7 +731,7 @@ sfc_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 	unsigned int i;
 	int nstats = 0;
 
-	rte_spinlock_lock(&port->mac_stats_lock);
+	sfc_adapter_lock(sa);
 
 	rc = sfc_port_update_mac_stats(sa);
 	if (rc != 0) {
@@ -748,7 +753,7 @@ sfc_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 	}
 
 unlock:
-	rte_spinlock_unlock(&port->mac_stats_lock);
+	sfc_adapter_unlock(sa);
 
 	return nstats;
 }
@@ -789,7 +794,7 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	int ret;
 	int rc;
 
-	rte_spinlock_lock(&port->mac_stats_lock);
+	sfc_adapter_lock(sa);
 
 	if (unlikely(values == NULL) ||
 	    unlikely(ids == NULL && n < port->mac_stats_nb_supported)) {
@@ -819,7 +824,7 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	ret = nb_written;
 
 unlock:
-	rte_spinlock_unlock(&port->mac_stats_lock);
+	sfc_adapter_unlock(sa);
 
 	return ret;
 }
@@ -835,9 +840,14 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 	unsigned int nb_written = 0;
 	unsigned int i;
 
+	sfc_adapter_lock(sa);
+
 	if (unlikely(xstats_names == NULL) ||
-	    unlikely((ids == NULL) && (size < port->mac_stats_nb_supported)))
-		return port->mac_stats_nb_supported;
+	    unlikely((ids == NULL) && (size < port->mac_stats_nb_supported))) {
+		nb_supported = port->mac_stats_nb_supported;
+		sfc_adapter_unlock(sa);
+		return nb_supported;
+	}
 
 	for (i = 0; (i < EFX_MAC_NSTATS) && (nb_written < size); ++i) {
 		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
@@ -853,6 +863,8 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 		++nb_supported;
 	}
 
+	sfc_adapter_unlock(sa);
+
 	return nb_written;
 }
 
diff --git a/drivers/net/sfc/sfc_port.c b/drivers/net/sfc/sfc_port.c
index ac117f9c48..cdc0f94f19 100644
--- a/drivers/net/sfc/sfc_port.c
+++ b/drivers/net/sfc/sfc_port.c
@@ -43,7 +43,7 @@ sfc_port_update_mac_stats(struct sfc_adapter *sa)
 	unsigned int nb_attempts = 0;
 	int rc;
 
-	SFC_ASSERT(rte_spinlock_is_locked(&port->mac_stats_lock));
+	SFC_ASSERT(sfc_adapter_is_locked(sa));
 
 	if (sa->state != SFC_ADAPTER_STARTED)
 		return EINVAL;
@@ -103,14 +103,13 @@ sfc_port_reset_sw_stats(struct sfc_adapter *sa)
 int
 sfc_port_reset_mac_stats(struct sfc_adapter *sa)
 {
-	struct sfc_port *port = &sa->port;
 	int rc;
 
-	rte_spinlock_lock(&port->mac_stats_lock);
+	SFC_ASSERT(sfc_adapter_is_locked(sa));
+
 	rc = efx_mac_stats_clear(sa->nic);
 	if (rc == 0)
 		sfc_port_reset_sw_stats(sa);
-	rte_spinlock_unlock(&port->mac_stats_lock);
 
 	return rc;
 }
@@ -416,8 +415,6 @@ sfc_port_attach(struct sfc_adapter *sa)
 		goto fail_mcast_addr_list_buf_alloc;
 	}
 
-	rte_spinlock_init(&port->mac_stats_lock);
-
 	rc = ENOMEM;
 	port->mac_stats_buf = rte_calloc_socket("mac_stats_buf", EFX_MAC_NSTATS,
 						sizeof(uint64_t), 0,
-- 
2.30.2

[dpdk-stable] [PATCH v3 03/11] ethdev: fix docs of functions getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Document valid combinations of input arguments in accordance with
current implementation in ethdev.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/rte_ethdev.h | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index d2b27c351f..28440c46d3 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2873,12 +2873,15 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  *   The port identifier of the Ethernet device.
  * @param xstats_names
  *   An rte_eth_xstat_name array of at least *size* elements to
- *   be filled. If set to NULL, the function returns the required number
- *   of elements.
+ *   be filled. Must not be NULL if @p ids are specified (not NULL).
  * @param ids
- *   IDs array given by app to retrieve specific statistics
+ *   IDs array given by app to retrieve specific statistics. May be NULL
+ *   to retrieve all available statistics.
  * @param size
- *   The size of the xstats_names array (number of elements).
+ *   If @p ids is not NULL, number of elements in the array with requested IDs
+ *   and number of elements in @p xstats_names to put names in. If @p ids is
+ *   NULL, number of elements in @p xstats_names to put all available statistics
+ *   names in.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
@@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int
 rte_eth_xstats_get_names_by_id(uint16_t port_id,
@@ -2900,13 +2903,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  *   The port identifier of the Ethernet device.
  * @param ids
  *   A pointer to an ids array passed by application. This tells which
- *   statistics values function should retrieve. This parameter
- *   can be set to NULL if size is 0. In this case function will retrieve
+ *   statistics values function should retrieve. May be NULL to retrieve
  *   all available statistics.
  * @param values
  *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL if ids are specified (not NULL).
  * @param size
- *   The size of the ids array (number of elements).
+ *   If @p ids is not NULL, number of elements in the array with requested IDs
+ *   and number of elements in values to put statistics in. If @p ids is NULL,
+ *   number of elements in values to put all available statistics in.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
@@ -2914,7 +2919,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 			     uint64_t *values, unsigned int size);
-- 
2.30.2

[dpdk-stable] [PATCH v3 04/11] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update xstats by IDs callbacks documentation in accordance with
ethdev usage of these callbacks. Document valid combinations of
input arguments to make driver implementation simpler.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/ethdev_driver.h | 43 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 40e474aa7e..fd5b7ca550 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat *stats, unsigned int n);
 /**< @internal Get extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get extended stats of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param values
+ *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL.
+ * @param n
+ *   Element count in @p ids and @p values
+ *
+ * @return
+ *   - A number of filled in stats.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
 				      const uint64_t *ids,
 				      uint64_t *values,
 				      unsigned int n);
-/**< @internal Get extended stats of an Ethernet device. */
 
 /**
  * @internal
@@ -218,10 +235,32 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, unsigned int size);
 /**< @internal Get names of extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get names of extended stats of an Ethernet device.
+ * For name count, set @p xstats_names and @p ids to NULL.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. Can be NULL together with @p ids to retrieve number of
+ *   available statistics.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Can be NULL together
+ *   with @p xstats_names to retrieve number of available statistics.
+ * @param size
+ *   Size of ids and xstats_names arrays.
+ *   Element count in @p ids and @p xstats_names
+ *
+ * @return
+ *   - A number of filled in stats if both xstats_names and ids are not NULL.
+ *   - A number of available stats if both xstats_names and ids are NULL.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
 	unsigned int size);
-/**< @internal Get names of extended stats of an Ethernet device. */
 
 typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
 					     uint16_t queue_id,
-- 
2.30.2

[dpdk-stable] [PATCH v3 05/11] net/sfc: fix xstats by ID callbacks according to ethdev

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Fix xstats by ID callbacks according to ethdev usage.
Handle combinations of input arguments that are required by ethdev
and sanity check and reject other combinations on callback entry.

Fixes: 73280c1e4ff ("net/sfc: support xstats retrieval by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc_ethdev.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index d5417e5e65..fca3f524a1 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -794,13 +794,10 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	int ret;
 	int rc;
 
-	sfc_adapter_lock(sa);
+	if (unlikely(ids == NULL || values == NULL))
+		return -EINVAL;
 
-	if (unlikely(values == NULL) ||
-	    unlikely(ids == NULL && n < port->mac_stats_nb_supported)) {
-		ret = port->mac_stats_nb_supported;
-		goto unlock;
-	}
+	sfc_adapter_lock(sa);
 
 	rc = sfc_port_update_mac_stats(sa);
 	if (rc != 0) {
@@ -815,7 +812,7 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
 			continue;
 
-		if ((ids == NULL) || (ids[nb_written] == nb_supported))
+		if (ids[nb_written] == nb_supported)
 			values[nb_written++] = mac_stats[i];
 
 		++nb_supported;
@@ -840,10 +837,13 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 	unsigned int nb_written = 0;
 	unsigned int i;
 
+	if (unlikely(xstats_names == NULL && ids != NULL) ||
+	    unlikely(xstats_names != NULL && ids == NULL))
+		return -EINVAL;
+
 	sfc_adapter_lock(sa);
 
-	if (unlikely(xstats_names == NULL) ||
-	    unlikely((ids == NULL) && (size < port->mac_stats_nb_supported))) {
+	if (unlikely(xstats_names == NULL && ids == NULL)) {
 		nb_supported = port->mac_stats_nb_supported;
 		sfc_adapter_unlock(sa);
 		return nb_supported;
@@ -853,7 +853,7 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
 			continue;
 
-		if ((ids == NULL) || (ids[nb_written] == nb_supported)) {
+		if (ids[nb_written] == nb_supported) {
 			char *name = xstats_names[nb_written++].name;
 
 			strlcpy(name, efx_mac_stat_name(sa->nic, i),
-- 
2.30.2

[dpdk-stable] [PATCH v3 06/11] net/sfc: fix accessing xstats by an unsorted list of IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Device may support only some MAC stats. Add mapping from ids to subset
of supported MAC stats for each port.

Fixes: 73280c1e4ff ("net/sfc: support xstats retrieval by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc.h        |  2 ++
 drivers/net/sfc/sfc_ethdev.c | 44 ++++++++++++++++++------------------
 drivers/net/sfc/sfc_port.c   | 29 ++++++++++++++++++------
 3 files changed, 46 insertions(+), 29 deletions(-)

diff --git a/drivers/net/sfc/sfc.h b/drivers/net/sfc/sfc.h
index c7b0e5a30d..972d32606d 100644
--- a/drivers/net/sfc/sfc.h
+++ b/drivers/net/sfc/sfc.h
@@ -141,6 +141,8 @@ struct sfc_port {
 
 	uint32_t		mac_stats_mask[EFX_MAC_STATS_MASK_NPAGES];
 
+	unsigned int			mac_stats_by_id[EFX_MAC_NSTATS];
+
 	uint64_t			ipackets;
 };
 
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index fca3f524a1..ae9304f90f 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -788,8 +788,6 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 	struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
 	struct sfc_port *port = &sa->port;
 	uint64_t *mac_stats;
-	unsigned int nb_supported = 0;
-	unsigned int nb_written = 0;
 	unsigned int i;
 	int ret;
 	int rc;
@@ -808,17 +806,19 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 
 	mac_stats = port->mac_stats_buf;
 
-	for (i = 0; (i < EFX_MAC_NSTATS) && (nb_written < n); ++i) {
-		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
-			continue;
-
-		if (ids[nb_written] == nb_supported)
-			values[nb_written++] = mac_stats[i];
+	SFC_ASSERT(port->mac_stats_nb_supported <=
+		   RTE_DIM(port->mac_stats_by_id));
 
-		++nb_supported;
+	for (i = 0; i < n; i++) {
+		if (ids[i] < port->mac_stats_nb_supported) {
+			values[i] = mac_stats[port->mac_stats_by_id[ids[i]]];
+		} else {
+			ret = i;
+			goto unlock;
+		}
 	}
 
-	ret = nb_written;
+	ret = n;
 
 unlock:
 	sfc_adapter_unlock(sa);
@@ -833,8 +833,7 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 {
 	struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
 	struct sfc_port *port = &sa->port;
-	unsigned int nb_supported = 0;
-	unsigned int nb_written = 0;
+	unsigned int nb_supported;
 	unsigned int i;
 
 	if (unlikely(xstats_names == NULL && ids != NULL) ||
@@ -849,23 +848,24 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 		return nb_supported;
 	}
 
-	for (i = 0; (i < EFX_MAC_NSTATS) && (nb_written < size); ++i) {
-		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
-			continue;
-
-		if (ids[nb_written] == nb_supported) {
-			char *name = xstats_names[nb_written++].name;
+	SFC_ASSERT(port->mac_stats_nb_supported <=
+		   RTE_DIM(port->mac_stats_by_id));
 
-			strlcpy(name, efx_mac_stat_name(sa->nic, i),
+	for (i = 0; i < size; i++) {
+		if (ids[i] < port->mac_stats_nb_supported) {
+			strlcpy(xstats_names[i].name,
+				efx_mac_stat_name(sa->nic,
+						 port->mac_stats_by_id[ids[i]]),
 				sizeof(xstats_names[0].name));
+		} else {
+			sfc_adapter_unlock(sa);
+			return i;
 		}
-
-		++nb_supported;
 	}
 
 	sfc_adapter_unlock(sa);
 
-	return nb_written;
+	return size;
 }
 
 static int
diff --git a/drivers/net/sfc/sfc_port.c b/drivers/net/sfc/sfc_port.c
index cdc0f94f19..bb9e01d96b 100644
--- a/drivers/net/sfc/sfc_port.c
+++ b/drivers/net/sfc/sfc_port.c
@@ -157,6 +157,27 @@ sfc_port_phy_caps_to_max_link_speed(uint32_t phy_caps)
 
 #endif
 
+static void
+sfc_port_fill_mac_stats_info(struct sfc_adapter *sa)
+{
+	unsigned int mac_stats_nb_supported = 0;
+	struct sfc_port *port = &sa->port;
+	unsigned int stat_idx;
+
+	efx_mac_stats_get_mask(sa->nic, port->mac_stats_mask,
+			       sizeof(port->mac_stats_mask));
+
+	for (stat_idx = 0; stat_idx < EFX_MAC_NSTATS; ++stat_idx) {
+		if (!EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, stat_idx))
+			continue;
+
+		port->mac_stats_by_id[mac_stats_nb_supported] = stat_idx;
+		mac_stats_nb_supported++;
+	}
+
+	port->mac_stats_nb_supported = mac_stats_nb_supported;
+}
+
 int
 sfc_port_start(struct sfc_adapter *sa)
 {
@@ -165,7 +186,6 @@ sfc_port_start(struct sfc_adapter *sa)
 	uint32_t phy_adv_cap;
 	const uint32_t phy_pause_caps =
 		((1u << EFX_PHY_CAP_PAUSE) | (1u << EFX_PHY_CAP_ASYM));
-	unsigned int i;
 
 	sfc_log_init(sa, "entry");
 
@@ -259,12 +279,7 @@ sfc_port_start(struct sfc_adapter *sa)
 		port->mac_stats_reset_pending = B_FALSE;
 	}
 
-	efx_mac_stats_get_mask(sa->nic, port->mac_stats_mask,
-			       sizeof(port->mac_stats_mask));
-
-	for (i = 0, port->mac_stats_nb_supported = 0; i < EFX_MAC_NSTATS; ++i)
-		if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i))
-			port->mac_stats_nb_supported++;
+	sfc_port_fill_mac_stats_info(sa);
 
 	port->mac_stats_update_generation = 0;
 
-- 
2.30.2

[dpdk-stable] [PATCH v3 07/11] net/sfc: fix MAC stats update to work for stopped device

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Fixes: 1caab2f1e68 ("net/sfc: add basic statistics")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 drivers/net/sfc/sfc.h        |  2 +-
 drivers/net/sfc/sfc_ethdev.c |  6 +++---
 drivers/net/sfc/sfc_port.c   | 11 +++++++----
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/sfc/sfc.h b/drivers/net/sfc/sfc.h
index 972d32606d..1594f934ba 100644
--- a/drivers/net/sfc/sfc.h
+++ b/drivers/net/sfc/sfc.h
@@ -422,7 +422,7 @@ int sfc_port_start(struct sfc_adapter *sa);
 void sfc_port_stop(struct sfc_adapter *sa);
 void sfc_port_link_mode_to_info(efx_link_mode_t link_mode,
 				struct rte_eth_link *link_info);
-int sfc_port_update_mac_stats(struct sfc_adapter *sa);
+int sfc_port_update_mac_stats(struct sfc_adapter *sa, boolean_t manual_update);
 int sfc_port_reset_mac_stats(struct sfc_adapter *sa);
 int sfc_set_rx_mode(struct sfc_adapter *sa);
 int sfc_set_rx_mode_unchecked(struct sfc_adapter *sa);
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index ae9304f90f..bbc22723f6 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -615,7 +615,7 @@ sfc_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 
 	sfc_adapter_lock(sa);
 
-	ret = sfc_port_update_mac_stats(sa);
+	ret = sfc_port_update_mac_stats(sa, B_FALSE);
 	if (ret != 0)
 		goto unlock;
 
@@ -733,7 +733,7 @@ sfc_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 
 	sfc_adapter_lock(sa);
 
-	rc = sfc_port_update_mac_stats(sa);
+	rc = sfc_port_update_mac_stats(sa, B_FALSE);
 	if (rc != 0) {
 		SFC_ASSERT(rc > 0);
 		nstats = -rc;
@@ -797,7 +797,7 @@ sfc_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
 
 	sfc_adapter_lock(sa);
 
-	rc = sfc_port_update_mac_stats(sa);
+	rc = sfc_port_update_mac_stats(sa, B_FALSE);
 	if (rc != 0) {
 		SFC_ASSERT(rc > 0);
 		ret = -rc;
diff --git a/drivers/net/sfc/sfc_port.c b/drivers/net/sfc/sfc_port.c
index bb9e01d96b..8c432c15f5 100644
--- a/drivers/net/sfc/sfc_port.c
+++ b/drivers/net/sfc/sfc_port.c
@@ -26,7 +26,8 @@
 /**
  * Update MAC statistics in the buffer.
  *
- * @param	sa	Adapter
+ * @param	sa		Adapter
+ * @param	force_upload	Flag to upload MAC stats in any case
  *
  * @return Status code
  * @retval	0	Success
@@ -34,7 +35,7 @@
  * @retval	ENOMEM	Memory allocation failure
  */
 int
-sfc_port_update_mac_stats(struct sfc_adapter *sa)
+sfc_port_update_mac_stats(struct sfc_adapter *sa, boolean_t force_upload)
 {
 	struct sfc_port *port = &sa->port;
 	efsys_mem_t *esmp = &port->mac_stats_dma_mem;
@@ -46,14 +47,14 @@ sfc_port_update_mac_stats(struct sfc_adapter *sa)
 	SFC_ASSERT(sfc_adapter_is_locked(sa));
 
 	if (sa->state != SFC_ADAPTER_STARTED)
-		return EINVAL;
+		return 0;
 
 	/*
 	 * If periodic statistics DMA'ing is off or if not supported,
 	 * make a manual request and keep an eye on timer if need be
 	 */
 	if (!port->mac_stats_periodic_dma_supported ||
-	    (port->mac_stats_update_period_ms == 0)) {
+	    (port->mac_stats_update_period_ms == 0) || force_upload) {
 		if (port->mac_stats_update_period_ms != 0) {
 			uint64_t timestamp = sfc_get_system_msecs();
 
@@ -367,6 +368,8 @@ sfc_port_stop(struct sfc_adapter *sa)
 	(void)efx_mac_stats_periodic(sa->nic, &sa->port.mac_stats_dma_mem,
 				     0, B_FALSE);
 
+	sfc_port_update_mac_stats(sa, B_TRUE);
+
 	efx_port_fini(sa->nic);
 	efx_filter_fini(sa->nic);
 
-- 
2.30.2

Re: [dpdk-stable] [PATCH 03/11] ethdev: fix docs of functions getting xstats by IDs

[Ferruh Yigit]

On 7/22/2021 10:12 AM, Andrew Rybchenko wrote:
> On 7/20/21 7:25 PM, Ferruh Yigit wrote:
>> On 6/4/2021 3:42 PM, Andrew Rybchenko wrote:
>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>
>>> Document valid combinations of input arguments in accordance with
>>> current implementation in ethdev.
>>>
>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>> ---
>>>   lib/ethdev/rte_ethdev.h | 23 ++++++++++++++---------
>>>   1 file changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>>> index faf3bd901d..1f63118544 100644
>>> --- a/lib/ethdev/rte_ethdev.h
>>> +++ b/lib/ethdev/rte_ethdev.h
>>> @@ -2873,12 +2873,15 @@ int rte_eth_xstats_get(uint16_t port_id, struct
>>> rte_eth_xstat *xstats,
>>>    *   The port identifier of the Ethernet device.
>>>    * @param xstats_names
>>>    *   An rte_eth_xstat_name array of at least *size* elements to
>>> - *   be filled. If set to NULL, the function returns the required number
>>> - *   of elements.
>>> + *   be filled. Must not be NULL if @p ids are specified (not NULL).
>>
>> Removed part is also valid. If both 'ids' & 'xstats_names' are NULL, API returns
>> number of all elements.
> 
> Yes, but it is an excessive information. The trigger to return number
> of all elements is 'ids == NULL'. Here we are talking about
> 'xstats_names' parameter. If the parameter is NULL, but ids is not
> null, it does not trigger number of all elements return. It is an
> invalid input parameters. That's what a new description says.
> 

   ids		xstats_names
a) NULL		NULL		valid, return _number_ of all elements
b) NULL		!NULL		valid, return all elements (if size big enough)
c) !NULL	NULL		invalid

Above a) is a valid option, the previous comment tries to describe it via "If
set to NULL, the function returns the required number of elements." and that
part is removed.
Since it is valid option I think we should keep it documented, location or
wording is up to you.

>>
>> Addition part looks good.
>>
>>>    * @param ids
>>> - *   IDs array given by app to retrieve specific statistics
>>> + *   IDs array given by app to retrieve specific statistics. May be NULL
>>> + *   to retrieve all available statistics.
>>
>> ack
>>
>>>    * @param size
>>> - *   The size of the xstats_names array (number of elements).
>>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>>> + *   and number of elements in @p xstats_names to put names in. If @p ids is
>>> + *   NULL, number of elements in @p xstats_names to put all available
>>> statistics
>>> + *   names in.
>>
>> ack
>>
>>>    * @return
>>>    *   - A positive value lower or equal to size: success. The return value
>>>    *     is the number of entries filled in the stats table.
>>> @@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct
>>> rte_eth_xstat *xstats,
>>>    *     is too small. The return value corresponds to the size that should
>>>    *     be given to succeed. The entries in the table are not valid and
>>>    *     shall not be used by the caller.
>>> - *   - A negative value on error (invalid port id).
>>> + *   - A negative value on error.
>>
>> ack
>>
>>
>> The 'eth_dev_get_xstats_count()' API is flexible but it makes API unnecessarily
>> complex, not for this patch but for future perhaps we can update the API and it
>> can return error if either 'ids' or 'xstats_names' is NULL. Remove support to
>> get all elements or getting number of elements support, these already supported
>> by non _id version of API.
> 
> I'm not sure that it is a right direction. The support allows
> application to use less number of functions and depend on less
> number of function prototypes.
> 

True, but makes API more complex, and creates multiple way to do same thing (get
number of all entries), trade off.

>> And as a note for future, if we ever consider updating these _by_id APIs, we can
>> consider making the parameter order same for both, currently it is:
>> "rte_eth_xstats_get_names_by_id(port_id, values, size, ids)"
>> "      rte_eth_xstats_get_by_id(port_id, ids, values, size)"
> 
> +1, current difference is terribly bad
> 
>>>    */
>>>   int
>>>   rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>> @@ -2900,13 +2903,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>>    *   The port identifier of the Ethernet device.
>>>    * @param ids
>>>    *   A pointer to an ids array passed by application. This tells which
>>> - *   statistics values function should retrieve. This parameter
>>> - *   can be set to NULL if size is 0. In this case function will retrieve
>>> + *   statistics values function should retrieve. May be NULL to retrieve
>>>    *   all available statistics.
>>
>> Update is good. But what do you think to make it exact same in the both APIs
>> ('rte_eth_xstats_get_names_by_id()' & 'rte_eth_xstats_get_by_id()')? Since it is
>> used for same purpose and exact same way in both APIs, no need to have slightly
>> different description.
> 
> I agree. I'll fix in v2.
> 
>>>    * @param values
>>>    *   A pointer to a table to be filled with device statistics values.
>>> + *   Must not be NULL if ids are specified (not NULL).
>>
>> Same comment on making description similar in both APIs.
> 
> OK
> 
>> Also both 'ids' & 'values' being NULL returns number of all elements should be
>> addressed.
> 
> I think it is excessibe. It is sufficient to say so for ids==NULL which
> is a trigger to get all elements.
> 

But these two are different things,
one fills values with stats and return number of filled elements
other doesn't fill any value, but only return number of all elements

>>>    * @param size
>>> - *   The size of the ids array (number of elements).
>>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>>> + *   and number of elements in values to put statistics in. If @p ids is NULL,
>>> + *   number of elements in values to put all available statistics in.
>>
>> ack
>>
>>>    * @return
>>>    *   - A positive value lower or equal to size: success. The return value
>>>    *     is the number of entries filled in the stats table.
>>> @@ -2914,7 +2919,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>>    *     is too small. The return value corresponds to the size that should
>>>    *     be given to succeed. The entries in the table are not valid and
>>>    *     shall not be used by the caller.
>>> - *   - A negative value on error (invalid port id).
>>> + *   - A negative value on error.
>>
>> ack
>>
>>>    */
>>>   int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
>>>                    uint64_t *values, unsigned int size);
>>>
> 

Re: [dpdk-stable] [PATCH 04/11] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Ferruh Yigit]

On 7/22/2021 10:33 AM, Andrew Rybchenko wrote:
> On 7/20/21 7:51 PM, Ferruh Yigit wrote:
>> On 6/4/2021 3:42 PM, Andrew Rybchenko wrote:
>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>
>>> Update xstats by IDs callbacks documentation in accordance with
>>> ethdev usage of these callbacks. Document valid combinations of
>>> input arguments to make driver implementation simpler.
>>>
>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>> ---
>>>   lib/ethdev/ethdev_driver.h | 43 ++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 41 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>> index 40e474aa7e..fd5b7ca550 100644
>>> --- a/lib/ethdev/ethdev_driver.h
>>> +++ b/lib/ethdev/ethdev_driver.h
>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>       struct rte_eth_xstat *stats, unsigned int n);
>>>   /**< @internal Get extended stats of an Ethernet device. */
>>>   +/**
>>> + * @internal
>>> + * Get extended stats of an Ethernet device.
>>
>> Should it mention _by_id detail?
> 
> Yes, will fix in v2.
> 
>>> + *
>>> + * @param dev
>>> + *   ethdev handle of port.
>>> + * @param ids
>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>> + * @param values
>>> + *   A pointer to a table to be filled with device statistics values.
>>> + *   Must not be NULL.
>>> + * @param n
>>> + *   Element count in @p ids and @p values
>>> + *
>>> + * @return
>>> + *   - A number of filled in stats.
>>> + *   - A negative value on error.
>>> + */
>>>   typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>                         const uint64_t *ids,
>>>                         uint64_t *values,
>>>                         unsigned int n);
>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>     /**
>>>    * @internal
>>> @@ -218,10 +235,32 @@ typedef int (*eth_xstats_get_names_t)(struct
>>> rte_eth_dev *dev,
>>>       struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>   /**< @internal Get names of extended stats of an Ethernet device. */
>>>   +/**
>>> + * @internal
>>> + * Get names of extended stats of an Ethernet device.
>>
>> Should it mention _by_id detail?
> 
> Yes, will fix in v2.
> 
>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>> + *
>>
>> isn't the 'count' part handled in the API? I think in the devops both should not
>> be NULL.
> 
> No, eth_dev_get_xstats_count() uses the callback with NULL, NULL, 0.
> 

Right, I missed it, xstats_get_names_by_id() seems used outside of the ethdev
_by_id() APIs.

>>
>>> + * @param dev
>>> + *   ethdev handle of port.
>>> + * @param xstats_names
>>> + *   An rte_eth_xstat_name array of at least *size* elements to
>>> + *   be filled. Can be NULL together with @p ids to retrieve number of
>>> + *   available statistics.
>>
>> As far as I understand both _by_id APIs and devops behave same, so argument
>> descriptions/behavior should be same.
> 
> In fact no, it is slightly different. For example, devops is never
> called with NULL ids and not NULL names or non-zero size. It allows to
> check less in drivers.
> 

I am not sure if this difference is intentional.

'eth_dev_get_xstats_count()' is ethdev internal function, what do you think
removing 'xstats_get_names_by_id()' call from it. And make both _by_id() dev_ops
doesn't accept NULL ids and NULL values/names, so simplifies PMD implementations.

>>> + * @param ids
>>> + *   IDs array to retrieve specific statistics. Can be NULL together
>>> + *   with @p xstats_names to retrieve number of available statistics.
>>> + * @param size
>>> + *   Size of ids and xstats_names arrays.
>>> + *   Element count in @p ids and @p xstats_names
>>> + *
>>> + * @return
>>> + *   - A number of filled in stats if both xstats_names and ids are not NULL.
>>> + *   - A number of available stats if both xstats_names and ids are NULL.
>>
>> Again as far as I can see these covered by API, not devops, am I missing
>> something.
> 
> See eth_dev_get_xstats_count()
> 
>>
>>> + *   - A negative value on error.
>>> + */
>>>   typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
>>>       struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
>>>       unsigned int size);
>>> -/**< @internal Get names of extended stats of an Ethernet device. */
>>>     typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
>>>                            uint16_t queue_id,
>>>
> 

Re: [dpdk-stable] [PATCH v3 03/11] ethdev: fix docs of functions getting xstats by IDs

[Ferruh Yigit]

On 7/23/2021 2:15 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Document valid combinations of input arguments in accordance with
> current implementation in ethdev.
> 
> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
>  lib/ethdev/rte_ethdev.h | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index d2b27c351f..28440c46d3 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -2873,12 +2873,15 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>   *   The port identifier of the Ethernet device.
>   * @param xstats_names
>   *   An rte_eth_xstat_name array of at least *size* elements to
> - *   be filled. If set to NULL, the function returns the required number
> - *   of elements.
> + *   be filled. Must not be NULL if @p ids are specified (not NULL).
>   * @param ids
> - *   IDs array given by app to retrieve specific statistics
> + *   IDs array given by app to retrieve specific statistics. May be NULL
> + *   to retrieve all available statistics.
>   * @param size
> - *   The size of the xstats_names array (number of elements).
> + *   If @p ids is not NULL, number of elements in the array with requested IDs
> + *   and number of elements in @p xstats_names to put names in. If @p ids is
> + *   NULL, number of elements in @p xstats_names to put all available statistics
> + *   names in.
>   * @return
>   *   - A positive value lower or equal to size: success. The return value
>   *     is the number of entries filled in the stats table.
> @@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>   *     is too small. The return value corresponds to the size that should
>   *     be given to succeed. The entries in the table are not valid and
>   *     shall not be used by the caller.
> - *   - A negative value on error (invalid port id).
> + *   - A negative value on error.
>   */
>  int
>  rte_eth_xstats_get_names_by_id(uint16_t port_id,
> @@ -2900,13 +2903,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>   *   The port identifier of the Ethernet device.
>   * @param ids
>   *   A pointer to an ids array passed by application. This tells which
> - *   statistics values function should retrieve. This parameter
> - *   can be set to NULL if size is 0. In this case function will retrieve
> + *   statistics values function should retrieve. May be NULL to retrieve
>   *   all available statistics.

'ids' parameter in 'rte_eth_xstats_get_names_by_id()' &
'rte_eth_xstats_get_by_id()' are exactly same thing, and description is same but
wording is different.

Do you think does it make sense to use exact same wording, to clarify that there
is no difference in this parameter within APIs?

>   * @param values
>   *   A pointer to a table to be filled with device statistics values.
> + *   Must not be NULL if ids are specified (not NULL).

Similar comment on this one, two different API get 'name' and 'value' part of
key-value pair. The description between APIs can be almost same.

>   * @param size
> - *   The size of the ids array (number of elements).
> + *   If @p ids is not NULL, number of elements in the array with requested IDs
> + *   and number of elements in values to put statistics in. If @p ids is NULL,
> + *   number of elements in values to put all available statistics in.

And same comment again on using exact same comment on two APIs.

>   * @return
>   *   - A positive value lower or equal to size: success. The return value
>   *     is the number of entries filled in the stats table.
> @@ -2914,7 +2919,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>   *     is too small. The return value corresponds to the size that should
>   *     be given to succeed. The entries in the table are not valid and
>   *     shall not be used by the caller.
> - *   - A negative value on error (invalid port id).
> + *   - A negative value on error.
>   */
>  int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
>  			     uint64_t *values, unsigned int size);
> 

Re: [dpdk-stable] [PATCH v3 04/11] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Ferruh Yigit]

On 7/23/2021 2:15 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Update xstats by IDs callbacks documentation in accordance with
> ethdev usage of these callbacks. Document valid combinations of
> input arguments to make driver implementation simpler.
> 
> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
>  lib/ethdev/ethdev_driver.h | 43 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 

Some discussion on the v1 is still valid here, so I am not putting same comments
here until v1 ones clarified.

> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 40e474aa7e..fd5b7ca550 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat *stats, unsigned int n);
>  /**< @internal Get extended stats of an Ethernet device. */
>  
> +/**
> + * @internal
> + * Get extended stats of an Ethernet device.
> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param ids
> + *   IDs array to retrieve specific statistics. Must not be NULL.
> + * @param values
> + *   A pointer to a table to be filled with device statistics values.
> + *   Must not be NULL.
> + * @param n
> + *   Element count in @p ids and @p values
> + *
> + * @return
> + *   - A number of filled in stats.
> + *   - A negative value on error.
> + */
>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>  				      const uint64_t *ids,
>  				      uint64_t *values,
>  				      unsigned int n);
> -/**< @internal Get extended stats of an Ethernet device. */
>  
>  /**
>   * @internal
> @@ -218,10 +235,32 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>  /**< @internal Get names of extended stats of an Ethernet device. */
>  
> +/**
> + * @internal
> + * Get names of extended stats of an Ethernet device.
> + * For name count, set @p xstats_names and @p ids to NULL.
> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param xstats_names
> + *   An rte_eth_xstat_name array of at least *size* elements to
> + *   be filled. Can be NULL together with @p ids to retrieve number of
> + *   available statistics.
> + * @param ids
> + *   IDs array to retrieve specific statistics. Can be NULL together
> + *   with @p xstats_names to retrieve number of available statistics.
> + * @param size
> + *   Size of ids and xstats_names arrays.
> + *   Element count in @p ids and @p xstats_names
> + *
> + * @return
> + *   - A number of filled in stats if both xstats_names and ids are not NULL.
> + *   - A number of available stats if both xstats_names and ids are NULL.
> + *   - A negative value on error.
> + */
>  typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
>  	unsigned int size);
> -/**< @internal Get names of extended stats of an Ethernet device. */
>  
>  typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
>  					     uint16_t queue_id,
> 

Re: [dpdk-stable] [PATCH 04/11] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

On 7/23/21 5:31 PM, Ferruh Yigit wrote:
> On 7/22/2021 10:33 AM, Andrew Rybchenko wrote:
>> On 7/20/21 7:51 PM, Ferruh Yigit wrote:
>>> On 6/4/2021 3:42 PM, Andrew Rybchenko wrote:
>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>
>>>> Update xstats by IDs callbacks documentation in accordance with
>>>> ethdev usage of these callbacks. Document valid combinations of
>>>> input arguments to make driver implementation simpler.
>>>>
>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>

[snip]

>>>> + * @param dev
>>>> + *   ethdev handle of port.
>>>> + * @param xstats_names
>>>> + *   An rte_eth_xstat_name array of at least *size* elements to
>>>> + *   be filled. Can be NULL together with @p ids to retrieve number of
>>>> + *   available statistics.
>>>
>>> As far as I understand both _by_id APIs and devops behave same, so argument
>>> descriptions/behavior should be same.
>>
>> In fact no, it is slightly different. For example, devops is never
>> called with NULL ids and not NULL names or non-zero size. It allows to
>> check less in drivers.
>>
> 
> I am not sure if this difference is intentional.

What we're trying to do here is to document existing usage of these
callbacks to more strictly define valid input parameters to allow
drivers to support less combinations (to make implementation simpler).

So, intentional.

If you're trying to say that API functions could handle less variants,
the answer is simple - it could be discussed, but out of scope of the
patch series since it will be API change.
> 'eth_dev_get_xstats_count()' is ethdev internal function, what do you think
> removing 'xstats_get_names_by_id()' call from it. And make both _by_id() dev_ops
> doesn't accept NULL ids and NULL values/names, so simplifies PMD implementations.
>
In fact the function eth_dev_get_xstats_count() looks buggy since it
does not use  xstats_get_names_by_id positive result if
xstats_get_names is not NULL. Really confusing.

Again, it will be slight change on PMD requirements. May be it is OK
to do, but I'd conduct it separately since it is logically a separate
thing. Current patches just clarify documentation.

Re: [dpdk-stable] [PATCH 03/11] ethdev: fix docs of functions getting xstats by IDs

[Andrew Rybchenko]

On 7/23/21 5:19 PM, Ferruh Yigit wrote:
> On 7/22/2021 10:12 AM, Andrew Rybchenko wrote:
>> On 7/20/21 7:25 PM, Ferruh Yigit wrote:
>>> On 6/4/2021 3:42 PM, Andrew Rybchenko wrote:
>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>
>>>> Document valid combinations of input arguments in accordance with
>>>> current implementation in ethdev.
>>>>
>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>>> ---
>>>>    lib/ethdev/rte_ethdev.h | 23 ++++++++++++++---------
>>>>    1 file changed, 14 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>>>> index faf3bd901d..1f63118544 100644
>>>> --- a/lib/ethdev/rte_ethdev.h
>>>> +++ b/lib/ethdev/rte_ethdev.h
>>>> @@ -2873,12 +2873,15 @@ int rte_eth_xstats_get(uint16_t port_id, struct
>>>> rte_eth_xstat *xstats,
>>>>     *   The port identifier of the Ethernet device.
>>>>     * @param xstats_names
>>>>     *   An rte_eth_xstat_name array of at least *size* elements to
>>>> - *   be filled. If set to NULL, the function returns the required number
>>>> - *   of elements.
>>>> + *   be filled. Must not be NULL if @p ids are specified (not NULL).
>>>
>>> Removed part is also valid. If both 'ids' & 'xstats_names' are NULL, API returns
>>> number of all elements.
>>
>> Yes, but it is an excessive information. The trigger to return number
>> of all elements is 'ids == NULL'. Here we are talking about
>> 'xstats_names' parameter. If the parameter is NULL, but ids is not
>> null, it does not trigger number of all elements return. It is an
>> invalid input parameters. That's what a new description says.
>>
> 
>     ids		xstats_names
> a) NULL		NULL		valid, return _number_ of all elements
> b) NULL		!NULL		valid, return all elements (if size big enough)
> c) !NULL	NULL		invalid
> 
> Above a) is a valid option, the previous comment tries to describe it via "If
> set to NULL, the function returns the required number of elements." and that
> part is removed.
> Since it is valid option I think we should keep it documented, location or
> wording is up to you.

OK, I see. IMHO, it looks better if it is mentioned in ids parameter
description.  See v4.

>>>
>>> Addition part looks good.
>>>
>>>>     * @param ids
>>>> - *   IDs array given by app to retrieve specific statistics
>>>> + *   IDs array given by app to retrieve specific statistics. May be NULL
>>>> + *   to retrieve all available statistics.
>>>
>>> ack
>>>
>>>>     * @param size
>>>> - *   The size of the xstats_names array (number of elements).
>>>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>>>> + *   and number of elements in @p xstats_names to put names in. If @p ids is
>>>> + *   NULL, number of elements in @p xstats_names to put all available
>>>> statistics
>>>> + *   names in.
>>>
>>> ack
>>>
>>>>     * @return
>>>>     *   - A positive value lower or equal to size: success. The return value
>>>>     *     is the number of entries filled in the stats table.
>>>> @@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct
>>>> rte_eth_xstat *xstats,
>>>>     *     is too small. The return value corresponds to the size that should
>>>>     *     be given to succeed. The entries in the table are not valid and
>>>>     *     shall not be used by the caller.
>>>> - *   - A negative value on error (invalid port id).
>>>> + *   - A negative value on error.
>>>
>>> ack
>>>
>>>
>>> The 'eth_dev_get_xstats_count()' API is flexible but it makes API unnecessarily
>>> complex, not for this patch but for future perhaps we can update the API and it
>>> can return error if either 'ids' or 'xstats_names' is NULL. Remove support to
>>> get all elements or getting number of elements support, these already supported
>>> by non _id version of API.
>>
>> I'm not sure that it is a right direction. The support allows
>> application to use less number of functions and depend on less
>> number of function prototypes.
>>
> 
> True, but makes API more complex, and creates multiple way to do same thing (get
> number of all entries), trade off.

Yes.

>>> And as a note for future, if we ever consider updating these _by_id APIs, we can
>>> consider making the parameter order same for both, currently it is:
>>> "rte_eth_xstats_get_names_by_id(port_id, values, size, ids)"
>>> "      rte_eth_xstats_get_by_id(port_id, ids, values, size)"
>>
>> +1, current difference is terribly bad
>>
>>>>     */
>>>>    int
>>>>    rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>>> @@ -2900,13 +2903,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>>>     *   The port identifier of the Ethernet device.
>>>>     * @param ids
>>>>     *   A pointer to an ids array passed by application. This tells which
>>>> - *   statistics values function should retrieve. This parameter
>>>> - *   can be set to NULL if size is 0. In this case function will retrieve
>>>> + *   statistics values function should retrieve. May be NULL to retrieve
>>>>     *   all available statistics.
>>>
>>> Update is good. But what do you think to make it exact same in the both APIs
>>> ('rte_eth_xstats_get_names_by_id()' & 'rte_eth_xstats_get_by_id()')? Since it is
>>> used for same purpose and exact same way in both APIs, no need to have slightly
>>> different description.
>>
>> I agree. I'll fix in v2.
>>
>>>>     * @param values
>>>>     *   A pointer to a table to be filled with device statistics values.
>>>> + *   Must not be NULL if ids are specified (not NULL).
>>>
>>> Same comment on making description similar in both APIs.
>>
>> OK
>>
>>> Also both 'ids' & 'values' being NULL returns number of all elements should be
>>> addressed.
>>
>> I think it is excessibe. It is sufficient to say so for ids==NULL which
>> is a trigger to get all elements.
>>
> 
> But these two are different things,
> one fills values with stats and return number of filled elements
> other doesn't fill any value, but only return number of all elements

I get it. See v4. Hopefully I'll address it in ids parameter
description.

>>>>     * @param size
>>>> - *   The size of the ids array (number of elements).
>>>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>>>> + *   and number of elements in values to put statistics in. If @p ids is NULL,
>>>> + *   number of elements in values to put all available statistics in.
>>>
>>> ack
>>>
>>>>     * @return
>>>>     *   - A positive value lower or equal to size: success. The return value
>>>>     *     is the number of entries filled in the stats table.
>>>> @@ -2914,7 +2919,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>>>     *     is too small. The return value corresponds to the size that should
>>>>     *     be given to succeed. The entries in the table are not valid and
>>>>     *     shall not be used by the caller.
>>>> - *   - A negative value on error (invalid port id).
>>>> + *   - A negative value on error.
>>>
>>> ack
>>>
>>>>     */
>>>>    int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
>>>>                     uint64_t *values, unsigned int size);
>>>>
>>

Re: [dpdk-stable] [PATCH v3 03/11] ethdev: fix docs of functions getting xstats by IDs

[Andrew Rybchenko]

On 7/23/21 5:42 PM, Ferruh Yigit wrote:
> On 7/23/2021 2:15 PM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> Document valid combinations of input arguments in accordance with
>> current implementation in ethdev.
>>
>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>> ---
>>   lib/ethdev/rte_ethdev.h | 23 ++++++++++++++---------
>>   1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index d2b27c351f..28440c46d3 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -2873,12 +2873,15 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>>    *   The port identifier of the Ethernet device.
>>    * @param xstats_names
>>    *   An rte_eth_xstat_name array of at least *size* elements to
>> - *   be filled. If set to NULL, the function returns the required number
>> - *   of elements.
>> + *   be filled. Must not be NULL if @p ids are specified (not NULL).
>>    * @param ids
>> - *   IDs array given by app to retrieve specific statistics
>> + *   IDs array given by app to retrieve specific statistics. May be NULL
>> + *   to retrieve all available statistics.
>>    * @param size
>> - *   The size of the xstats_names array (number of elements).
>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>> + *   and number of elements in @p xstats_names to put names in. If @p ids is
>> + *   NULL, number of elements in @p xstats_names to put all available statistics
>> + *   names in.
>>    * @return
>>    *   - A positive value lower or equal to size: success. The return value
>>    *     is the number of entries filled in the stats table.
>> @@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>>    *     is too small. The return value corresponds to the size that should
>>    *     be given to succeed. The entries in the table are not valid and
>>    *     shall not be used by the caller.
>> - *   - A negative value on error (invalid port id).
>> + *   - A negative value on error.
>>    */
>>   int
>>   rte_eth_xstats_get_names_by_id(uint16_t port_id,
>> @@ -2900,13 +2903,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>    *   The port identifier of the Ethernet device.
>>    * @param ids
>>    *   A pointer to an ids array passed by application. This tells which
>> - *   statistics values function should retrieve. This parameter
>> - *   can be set to NULL if size is 0. In this case function will retrieve
>> + *   statistics values function should retrieve. May be NULL to retrieve
>>    *   all available statistics.
> 
> 'ids' parameter in 'rte_eth_xstats_get_names_by_id()' &
> 'rte_eth_xstats_get_by_id()' are exactly same thing, and description is same but
> wording is different.
> 
> Do you think does it make sense to use exact same wording, to clarify that there
> is no difference in this parameter within APIs?

I thought that I've fixed it, but it looks like I've lost it on the way.
Will fix in v4. IMHO, it looks awkward if description is absolutely the
same, but description structure should be definitely the same.

>>    * @param values
>>    *   A pointer to a table to be filled with device statistics values.
>> + *   Must not be NULL if ids are specified (not NULL).
> 
> Similar comment on this one, two different API get 'name' and 'value' part of
> key-value pair. The description between APIs can be almost same.

Will fix in v4 similar to above.

>>    * @param size
>> - *   The size of the ids array (number of elements).
>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>> + *   and number of elements in values to put statistics in. If @p ids is NULL,
>> + *   number of elements in values to put all available statistics in.
> 
> And same comment again on using exact same comment on two APIs.

It is almost the same since it refers to other parameters which differ.
Other than that the description is equal.

>>    * @return
>>    *   - A positive value lower or equal to size: success. The return value
>>    *     is the number of entries filled in the stats table.
>> @@ -2914,7 +2919,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>    *     is too small. The return value corresponds to the size that should
>>    *     be given to succeed. The entries in the table are not valid and
>>    *     shall not be used by the caller.
>> - *   - A negative value on error (invalid port id).
>> + *   - A negative value on error.
>>    */
>>   int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
>>   			     uint64_t *values, unsigned int size);
>>

[dpdk-stable] [PATCH v4 1/2] ethdev: fix docs of functions getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Document valid combinations of input arguments in accordance with
current implementation in ethdev.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/rte_ethdev.h | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index d2b27c351f..b14067fe7e 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2872,13 +2872,17 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param xstats_names
- *   An rte_eth_xstat_name array of at least *size* elements to
- *   be filled. If set to NULL, the function returns the required number
- *   of elements.
+ *   Array to be filled in with names of requested device statistics.
+ *   Must not be NULL if @p ids are specified (not NULL).
  * @param ids
- *   IDs array given by app to retrieve specific statistics
+ *   IDs array given by app to retrieve specific statistics. May be NULL to
+ *   retrieve names of all available statistics or, if @p xstats_names is
+ *   NULL as well,  just a number of available statistics.
  * @param size
- *   The size of the xstats_names array (number of elements).
+ *   If @p ids is not NULL, number of elements in the array with requested IDs
+ *   and number of elements in @p xstats_names to put names in. If @p ids is
+ *   NULL, number of elements in @p xstats_names to put all available statistics
+ *   names in.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
@@ -2886,7 +2890,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int
 rte_eth_xstats_get_names_by_id(uint16_t port_id,
@@ -2899,14 +2903,16 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param ids
- *   A pointer to an ids array passed by application. This tells which
- *   statistics values function should retrieve. This parameter
- *   can be set to NULL if size is 0. In this case function will retrieve
- *   all available statistics.
+ *   IDs array given by app to retrieve specific statistics. May be NULL to
+ *   retrieve all available statistics or, if @p values is NULL as well,
+ *   just a number of available statistics.
  * @param values
- *   A pointer to a table to be filled with device statistics values.
+ *   Array to be filled in with requested device statistics.
+ *   Must not be NULL if ids are specified (not NULL).
  * @param size
- *   The size of the ids array (number of elements).
+ *   If @p ids is not NULL, number of elements in the array with requested IDs
+ *   and number of elements in @p values to put statistics in. If @p ids is NULL,
+ *   number of elements in @p values to put all available statistics in.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
@@ -2914,7 +2920,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 			     uint64_t *values, unsigned int size);
-- 
2.30.2

[dpdk-stable] [PATCH v4 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update xstats by IDs callbacks documentation in accordance with
ethdev usage of these callbacks. Document valid combinations of
input arguments to make driver implementation simpler.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/ethdev_driver.h | 43 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 40e474aa7e..fd5b7ca550 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat *stats, unsigned int n);
 /**< @internal Get extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get extended stats of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param values
+ *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL.
+ * @param n
+ *   Element count in @p ids and @p values
+ *
+ * @return
+ *   - A number of filled in stats.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
 				      const uint64_t *ids,
 				      uint64_t *values,
 				      unsigned int n);
-/**< @internal Get extended stats of an Ethernet device. */
 
 /**
  * @internal
@@ -218,10 +235,32 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, unsigned int size);
 /**< @internal Get names of extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get names of extended stats of an Ethernet device.
+ * For name count, set @p xstats_names and @p ids to NULL.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. Can be NULL together with @p ids to retrieve number of
+ *   available statistics.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Can be NULL together
+ *   with @p xstats_names to retrieve number of available statistics.
+ * @param size
+ *   Size of ids and xstats_names arrays.
+ *   Element count in @p ids and @p xstats_names
+ *
+ * @return
+ *   - A number of filled in stats if both xstats_names and ids are not NULL.
+ *   - A number of available stats if both xstats_names and ids are NULL.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
 	unsigned int size);
-/**< @internal Get names of extended stats of an Ethernet device. */
 
 typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
 					     uint16_t queue_id,
-- 
2.30.2

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 1/2] ethdev: fix docs of functions getting xstats by IDs

[Olivier Matz]

Hi Andrew,

Some comments below.

On Sat, Jul 24, 2021 at 03:33:13PM +0300, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Document valid combinations of input arguments in accordance with
> current implementation in ethdev.
> 
> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
>  lib/ethdev/rte_ethdev.h | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index d2b27c351f..b14067fe7e 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -2872,13 +2872,17 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>   * @param port_id
>   *   The port identifier of the Ethernet device.
>   * @param xstats_names
> - *   An rte_eth_xstat_name array of at least *size* elements to
> - *   be filled. If set to NULL, the function returns the required number
> - *   of elements.
> + *   Array to be filled in with names of requested device statistics.
> + *   Must not be NULL if @p ids are specified (not NULL).
>   * @param ids
> - *   IDs array given by app to retrieve specific statistics
> + *   IDs array given by app to retrieve specific statistics. May be NULL to
> + *   retrieve names of all available statistics or, if @p xstats_names is
> + *   NULL as well,  just a number of available statistics.

double spaces before "just"

"a number" -> "the number"?

>   * @param size
> - *   The size of the xstats_names array (number of elements).
> + *   If @p ids is not NULL, number of elements in the array with requested IDs
> + *   and number of elements in @p xstats_names to put names in. If @p ids is
> + *   NULL, number of elements in @p xstats_names to put all available statistics
> + *   names in.

Just a suggestion here, I feel the following description would be clearer:

  Number of elements in @p xstats_names array (if not NULL) and
  in @p ids array (if not NULL).

Shouldn't we say that it has to be 0 if both arrays are NULL?


Also, the order of arguments is not the same in comment and in
the function. I think it can make sense to align the comment
to the prototype.


>   * @return
>   *   - A positive value lower or equal to size: success. The return value
>   *     is the number of entries filled in the stats table.

Not seen in the patch, but right after this line, there is:

 *   - A positive value higher than size: error, the given statistics table
 *     is too small. The return value corresponds to the size that should
 *     be given to succeed. The entries in the table are not valid and
 *     shall not be used by the caller.

I wonder if it shouldn't be slighly reworded to remove 'error'. After
all, passing NULL arrays (and size == 0) is a valid, so the return is
not an error.


> @@ -2886,7 +2890,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>   *     is too small. The return value corresponds to the size that should
>   *     be given to succeed. The entries in the table are not valid and
>   *     shall not be used by the caller.
> - *   - A negative value on error (invalid port id).
> + *   - A negative value on error.
>   */
>  int
>  rte_eth_xstats_get_names_by_id(uint16_t port_id,
> @@ -2899,14 +2903,16 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>   * @param port_id
>   *   The port identifier of the Ethernet device.
>   * @param ids
> - *   A pointer to an ids array passed by application. This tells which
> - *   statistics values function should retrieve. This parameter
> - *   can be set to NULL if size is 0. In this case function will retrieve
> - *   all available statistics.
> + *   IDs array given by app to retrieve specific statistics. May be NULL to
> + *   retrieve all available statistics or, if @p values is NULL as well,
> + *   just a number of available statistics.
>   * @param values
> - *   A pointer to a table to be filled with device statistics values.
> + *   Array to be filled in with requested device statistics.
> + *   Must not be NULL if ids are specified (not NULL).
>   * @param size
> - *   The size of the ids array (number of elements).
> + *   If @p ids is not NULL, number of elements in the array with requested IDs
> + *   and number of elements in @p values to put statistics in. If @p ids is NULL,
> + *   number of elements in @p values to put all available statistics in.
>   * @return
>   *   - A positive value lower or equal to size: success. The return value
>   *     is the number of entries filled in the stats table.
> @@ -2914,7 +2920,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>   *     is too small. The return value corresponds to the size that should
>   *     be given to succeed. The entries in the table are not valid and
>   *     shall not be used by the caller.
> - *   - A negative value on error (invalid port id).
> + *   - A negative value on error.
>   */

Some of the comments above also apply here.

>  int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
>  			     uint64_t *values, unsigned int size);
> -- 
> 2.30.2
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Olivier Matz]

On Sat, Jul 24, 2021 at 03:33:14PM +0300, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Update xstats by IDs callbacks documentation in accordance with
> ethdev usage of these callbacks. Document valid combinations of
> input arguments to make driver implementation simpler.
> 
> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
>  lib/ethdev/ethdev_driver.h | 43 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 40e474aa7e..fd5b7ca550 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat *stats, unsigned int n);
>  /**< @internal Get extended stats of an Ethernet device. */
>  
> +/**
> + * @internal
> + * Get extended stats of an Ethernet device.
> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param ids
> + *   IDs array to retrieve specific statistics. Must not be NULL.
> + * @param values
> + *   A pointer to a table to be filled with device statistics values.
> + *   Must not be NULL.
> + * @param n
> + *   Element count in @p ids and @p values
> + *
> + * @return
> + *   - A number of filled in stats.
> + *   - A negative value on error.
> + */
>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>  				      const uint64_t *ids,
>  				      uint64_t *values,
>  				      unsigned int n);
> -/**< @internal Get extended stats of an Ethernet device. */
>  
>  /**
>   * @internal
> @@ -218,10 +235,32 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>  /**< @internal Get names of extended stats of an Ethernet device. */
>  
> +/**
> + * @internal
> + * Get names of extended stats of an Ethernet device.
> + * For name count, set @p xstats_names and @p ids to NULL.
> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param xstats_names
> + *   An rte_eth_xstat_name array of at least *size* elements to
> + *   be filled. Can be NULL together with @p ids to retrieve number of
> + *   available statistics.
> + * @param ids
> + *   IDs array to retrieve specific statistics. Can be NULL together
> + *   with @p xstats_names to retrieve number of available statistics.
> + * @param size
> + *   Size of ids and xstats_names arrays.
> + *   Element count in @p ids and @p xstats_names

I think only the second line should be kept.

Shouldn't we also say here that size should be 0 if arrays are NULL?

> + *
> + * @return
> + *   - A number of filled in stats if both xstats_names and ids are not NULL.
> + *   - A number of available stats if both xstats_names and ids are NULL.
> + *   - A negative value on error.
> + */
>  typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
>  	unsigned int size);
> -/**< @internal Get names of extended stats of an Ethernet device. */
>  
>  typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
>  					     uint16_t queue_id,
> -- 
> 2.30.2
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 1/2] ethdev: fix docs of functions getting xstats by IDs

[Andrew Rybchenko]

Hi Olivier,

I apologize for so long delay with reply. I simply lost it from
my view.

Many thanks for review notes. See below.

On 7/26/21 1:13 PM, Olivier Matz wrote:
> Hi Andrew,
> 
> Some comments below.
> 
> On Sat, Jul 24, 2021 at 03:33:13PM +0300, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> Document valid combinations of input arguments in accordance with
>> current implementation in ethdev.
>>
>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>> ---
>>  lib/ethdev/rte_ethdev.h | 32 +++++++++++++++++++-------------
>>  1 file changed, 19 insertions(+), 13 deletions(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index d2b27c351f..b14067fe7e 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -2872,13 +2872,17 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>>   * @param port_id
>>   *   The port identifier of the Ethernet device.
>>   * @param xstats_names
>> - *   An rte_eth_xstat_name array of at least *size* elements to
>> - *   be filled. If set to NULL, the function returns the required number
>> - *   of elements.
>> + *   Array to be filled in with names of requested device statistics.
>> + *   Must not be NULL if @p ids are specified (not NULL).
>>   * @param ids
>> - *   IDs array given by app to retrieve specific statistics
>> + *   IDs array given by app to retrieve specific statistics. May be NULL to
>> + *   retrieve names of all available statistics or, if @p xstats_names is
>> + *   NULL as well,  just a number of available statistics.
> 
> double spaces before "just"
> 
> "a number" -> "the number"?

Fixed in v5

>>   * @param size
>> - *   The size of the xstats_names array (number of elements).
>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>> + *   and number of elements in @p xstats_names to put names in. If @p ids is
>> + *   NULL, number of elements in @p xstats_names to put all available statistics
>> + *   names in.
> 
> Just a suggestion here, I feel the following description would be clearer:
> 
>   Number of elements in @p xstats_names array (if not NULL) and
>   in @p ids array (if not NULL).

I agree that it is better to avoid here details about
behaviour of one or another array pointer is NULL.
Descriptions of corresponding parameters cover it.

Fixed in v5.

>
> Shouldn't we say that it has to be 0 if both arrays are NULL?

Yes, I think it is useful. Will add in v5.

> 
> Also, the order of arguments is not the same in comment and in
> the function. I think it can make sense to align the comment
> to the prototype.

Fixed in v5.

> 
> 
>>   * @return
>>   *   - A positive value lower or equal to size: success. The return value
>>   *     is the number of entries filled in the stats table.
> 
> Not seen in the patch, but right after this line, there is:
> 
>  *   - A positive value higher than size: error, the given statistics table
>  *     is too small. The return value corresponds to the size that should
>  *     be given to succeed. The entries in the table are not valid and
>  *     shall not be used by the caller.
> 
> I wonder if it shouldn't be slighly reworded to remove 'error'. After
> all, passing NULL arrays (and size == 0) is a valid, so the return is
> not an error.

I agree that it should not be treated as an error. It is just a
special case of success when return is partially provided (just
a number of stats).

Fixed in v5.

> 
>> @@ -2886,7 +2890,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>>   *     is too small. The return value corresponds to the size that should
>>   *     be given to succeed. The entries in the table are not valid and
>>   *     shall not be used by the caller.
>> - *   - A negative value on error (invalid port id).
>> + *   - A negative value on error.
>>   */
>>  int
>>  rte_eth_xstats_get_names_by_id(uint16_t port_id,
>> @@ -2899,14 +2903,16 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>   * @param port_id
>>   *   The port identifier of the Ethernet device.
>>   * @param ids
>> - *   A pointer to an ids array passed by application. This tells which
>> - *   statistics values function should retrieve. This parameter
>> - *   can be set to NULL if size is 0. In this case function will retrieve
>> - *   all available statistics.
>> + *   IDs array given by app to retrieve specific statistics. May be NULL to
>> + *   retrieve all available statistics or, if @p values is NULL as well,
>> + *   just a number of available statistics.
>>   * @param values
>> - *   A pointer to a table to be filled with device statistics values.
>> + *   Array to be filled in with requested device statistics.
>> + *   Must not be NULL if ids are specified (not NULL).
>>   * @param size
>> - *   The size of the ids array (number of elements).
>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>> + *   and number of elements in @p values to put statistics in. If @p ids is NULL,
>> + *   number of elements in @p values to put all available statistics in.
>>   * @return
>>   *   - A positive value lower or equal to size: success. The return value
>>   *     is the number of entries filled in the stats table.
>> @@ -2914,7 +2920,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>   *     is too small. The return value corresponds to the size that should
>>   *     be given to succeed. The entries in the table are not valid and
>>   *     shall not be used by the caller.
>> - *   - A negative value on error (invalid port id).
>> + *   - A negative value on error.
>>   */
> 
> Some of the comments above also apply here.

Done.

> 
>>  int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
>>  			     uint64_t *values, unsigned int size);
>> -- 
>> 2.30.2
>>

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

On 7/26/21 1:13 PM, Olivier Matz wrote:
> On Sat, Jul 24, 2021 at 03:33:14PM +0300, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> Update xstats by IDs callbacks documentation in accordance with
>> ethdev usage of these callbacks. Document valid combinations of
>> input arguments to make driver implementation simpler.
>>
>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>> ---
>>  lib/ethdev/ethdev_driver.h | 43 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>> index 40e474aa7e..fd5b7ca550 100644
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>  	struct rte_eth_xstat *stats, unsigned int n);
>>  /**< @internal Get extended stats of an Ethernet device. */
>>  
>> +/**
>> + * @internal
>> + * Get extended stats of an Ethernet device.
>> + *
>> + * @param dev
>> + *   ethdev handle of port.
>> + * @param ids
>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>> + * @param values
>> + *   A pointer to a table to be filled with device statistics values.
>> + *   Must not be NULL.
>> + * @param n
>> + *   Element count in @p ids and @p values
>> + *
>> + * @return
>> + *   - A number of filled in stats.
>> + *   - A negative value on error.
>> + */
>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>  				      const uint64_t *ids,
>>  				      uint64_t *values,
>>  				      unsigned int n);
>> -/**< @internal Get extended stats of an Ethernet device. */
>>  
>>  /**
>>   * @internal
>> @@ -218,10 +235,32 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>  
>> +/**
>> + * @internal
>> + * Get names of extended stats of an Ethernet device.
>> + * For name count, set @p xstats_names and @p ids to NULL.
>> + *
>> + * @param dev
>> + *   ethdev handle of port.
>> + * @param xstats_names
>> + *   An rte_eth_xstat_name array of at least *size* elements to
>> + *   be filled. Can be NULL together with @p ids to retrieve number of
>> + *   available statistics.
>> + * @param ids
>> + *   IDs array to retrieve specific statistics. Can be NULL together
>> + *   with @p xstats_names to retrieve number of available statistics.
>> + * @param size
>> + *   Size of ids and xstats_names arrays.
>> + *   Element count in @p ids and @p xstats_names
> 
> I think only the second line should be kept.

Thanks, fixed in v5.

> 
> Shouldn't we also say here that size should be 0 if arrays are NULL?

In this particular case I don't think so. ethdev layer
guarantees that the parameter is 0 if both array pointers
are NULL. PMD should not care about it.

> 
>> + *
>> + * @return
>> + *   - A number of filled in stats if both xstats_names and ids are not NULL.
>> + *   - A number of available stats if both xstats_names and ids are NULL.
>> + *   - A negative value on error.
>> + */
>>  typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
>>  	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
>>  	unsigned int size);
>> -/**< @internal Get names of extended stats of an Ethernet device. */
>>  
>>  typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
>>  					     uint16_t queue_id,
>> -- 
>> 2.30.2
>>

[dpdk-stable] [PATCH v5 1/2] ethdev: fix docs of functions getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Document valid combinations of input arguments in accordance with
current implementation in ethdev.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/rte_ethdev.h | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 224c6c980a..f597171c97 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2899,21 +2899,23 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param xstats_names
- *   An rte_eth_xstat_name array of at least *size* elements to
- *   be filled. If set to NULL, the function returns the required number
- *   of elements.
- * @param ids
- *   IDs array given by app to retrieve specific statistics
+ *   Array to be filled in with names of requested device statistics.
+ *   Must not be NULL if @p ids are specified (not NULL).
  * @param size
- *   The size of the xstats_names array (number of elements).
+ *   Number of elements in @p xstats_names array (if not NULL) and in
+ *   @p ids array (if not NULL). Must be 0 if both array pointers are NULL.
+ * @param ids
+ *   IDs array given by app to retrieve specific statistics. May be NULL to
+ *   retrieve names of all available statistics or, if @p xstats_names is
+ *   NULL as well, just the number of available statistics.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
- *   - A positive value higher than size: error, the given statistics table
+ *   - A positive value higher than size: success. The given statistics table
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int
 rte_eth_xstats_get_names_by_id(uint16_t port_id,
@@ -2926,22 +2928,23 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param ids
- *   A pointer to an ids array passed by application. This tells which
- *   statistics values function should retrieve. This parameter
- *   can be set to NULL if size is 0. In this case function will retrieve
- *   all available statistics.
+ *   IDs array given by app to retrieve specific statistics. May be NULL to
+ *   retrieve all available statistics or, if @p values is NULL as well,
+ *   just the number of available statistics.
  * @param values
- *   A pointer to a table to be filled with device statistics values.
+ *   Array to be filled in with requested device statistics.
+ *   Must not be NULL if ids are specified (not NULL).
  * @param size
- *   The size of the ids array (number of elements).
+ *   Number of elements in @p values array (if not NULL) and in @p ids
+ *   array (if not NULL). Must be 0 if both array pointers are NULL.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
- *   - A positive value higher than size: error, the given statistics table
+ *   - A positive value higher than size: success: The given statistics table
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 			     uint64_t *values, unsigned int size);
-- 
2.30.2

[dpdk-stable] [PATCH v5 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update xstats by IDs callbacks documentation in accordance with
ethdev usage of these callbacks. Document valid combinations of
input arguments to make driver implementation simpler.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 40e474aa7e..c89eefcc42 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat *stats, unsigned int n);
 /**< @internal Get extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get extended stats of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param values
+ *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL.
+ * @param n
+ *   Element count in @p ids and @p values.
+ *
+ * @return
+ *   - A number of filled in stats.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
 				      const uint64_t *ids,
 				      uint64_t *values,
 				      unsigned int n);
-/**< @internal Get extended stats of an Ethernet device. */
 
 /**
  * @internal
@@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, unsigned int size);
 /**< @internal Get names of extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get names of extended stats of an Ethernet device.
+ * For name count, set @p xstats_names and @p ids to NULL.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. Can be NULL together with @p ids to retrieve number of
+ *   available statistics.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Can be NULL together
+ *   with @p xstats_names to retrieve number of available statistics.
+ * @param size
+ *   Element count in @p ids and @p xstats_names.
+ *
+ * @return
+ *   - A number of filled in stats if both xstats_names and ids are not NULL.
+ *   - A number of available stats if both xstats_names and ids are NULL.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
 	unsigned int size);
-/**< @internal Get names of extended stats of an Ethernet device. */
 
 typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
 					     uint16_t queue_id,
-- 
2.30.2

Re: [dpdk-stable] [PATCH v5 1/2] ethdev: fix docs of functions getting xstats by IDs

[Ferruh Yigit]

On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Document valid combinations of input arguments in accordance with
> current implementation in ethdev.
> 
> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Only minor issue below.

> @@ -2926,22 +2928,23 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>   * @param port_id
>   *   The port identifier of the Ethernet device.
>   * @param ids
> - *   A pointer to an ids array passed by application. This tells which
> - *   statistics values function should retrieve. This parameter
> - *   can be set to NULL if size is 0. In this case function will retrieve
> - *   all available statistics.
> + *   IDs array given by app to retrieve specific statistics. May be NULL to
> + *   retrieve all available statistics or, if @p values is NULL as well,
> + *   just the number of available statistics.
>   * @param values
> - *   A pointer to a table to be filled with device statistics values.
> + *   Array to be filled in with requested device statistics.
> + *   Must not be NULL if ids are specified (not NULL).

Minor issue, @p marker is missed before 'ids', comparing the
'rte_eth_xstats_get_names_by_id()' comment.

Re: [dpdk-stable] [PATCH v5 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Ferruh Yigit]

On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Update xstats by IDs callbacks documentation in accordance with
> ethdev usage of these callbacks. Document valid combinations of
> input arguments to make driver implementation simpler.
> 
> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 40e474aa7e..c89eefcc42 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat *stats, unsigned int n);
>  /**< @internal Get extended stats of an Ethernet device. */
>  
> +/**
> + * @internal
> + * Get extended stats of an Ethernet device.
> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param ids
> + *   IDs array to retrieve specific statistics. Must not be NULL.
> + * @param values
> + *   A pointer to a table to be filled with device statistics values.
> + *   Must not be NULL.
> + * @param n
> + *   Element count in @p ids and @p values.
> + *
> + * @return
> + *   - A number of filled in stats.
> + *   - A negative value on error.
> + */
>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>  				      const uint64_t *ids,
>  				      uint64_t *values,
>  				      unsigned int n);
> -/**< @internal Get extended stats of an Ethernet device. */
>  
>  /**
>   * @internal
> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>  /**< @internal Get names of extended stats of an Ethernet device. */
>  
> +/**
> + * @internal
> + * Get names of extended stats of an Ethernet device.
> + * For name count, set @p xstats_names and @p ids to NULL.

Why limiting this behavior to 'xstats_get_names_by_id'?

Internally 'xstats_get_names_by_id' is used to get the count, but I think this
is not intentionally selected, just one of the xstats_*_by_id dev_ops used.

I think it is confusing to have this support for one of the '_by_id' dev_ops but
not for other. Why not require both to support returning 'count'?

> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param xstats_names
> + *   An rte_eth_xstat_name array of at least *size* elements to
> + *   be filled. Can be NULL together with @p ids to retrieve number of
> + *   available statistics.
> + * @param ids
> + *   IDs array to retrieve specific statistics. Can be NULL together
> + *   with @p xstats_names to retrieve number of available statistics.
> + * @param size
> + *   Element count in @p ids and @p xstats_names.
> + *
> + * @return
> + *   - A number of filled in stats if both xstats_names and ids are not NULL.
> + *   - A number of available stats if both xstats_names and ids are NULL.
> + *   - A negative value on error.
> + */
>  typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
>  	unsigned int size);
> -/**< @internal Get names of extended stats of an Ethernet device. */
>  
>  typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
>  					     uint16_t queue_id,
> 

Re: [dpdk-stable] [PATCH v5 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

On 9/28/21 7:50 PM, Ferruh Yigit wrote:
> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> Update xstats by IDs callbacks documentation in accordance with
>> ethdev usage of these callbacks. Document valid combinations of
>> input arguments to make driver implementation simpler.
>>
>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>> ---
>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>> index 40e474aa7e..c89eefcc42 100644
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>  	struct rte_eth_xstat *stats, unsigned int n);
>>  /**< @internal Get extended stats of an Ethernet device. */
>>  
>> +/**
>> + * @internal
>> + * Get extended stats of an Ethernet device.
>> + *
>> + * @param dev
>> + *   ethdev handle of port.
>> + * @param ids
>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>> + * @param values
>> + *   A pointer to a table to be filled with device statistics values.
>> + *   Must not be NULL.
>> + * @param n
>> + *   Element count in @p ids and @p values.
>> + *
>> + * @return
>> + *   - A number of filled in stats.
>> + *   - A negative value on error.
>> + */
>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>  				      const uint64_t *ids,
>>  				      uint64_t *values,
>>  				      unsigned int n);
>> -/**< @internal Get extended stats of an Ethernet device. */
>>  
>>  /**
>>   * @internal
>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>  
>> +/**
>> + * @internal
>> + * Get names of extended stats of an Ethernet device.
>> + * For name count, set @p xstats_names and @p ids to NULL.
> 
> Why limiting this behavior to 'xstats_get_names_by_id'?
> 
> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
> 
> I think it is confusing to have this support for one of the '_by_id' dev_ops but
> not for other. Why not require both to support returning 'count'?

Simply because it is dead code. There is no point to require
from driver to have dead code.

Re: [dpdk-stable] [PATCH v5 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Ferruh Yigit]

On 9/28/2021 5:53 PM, Andrew Rybchenko wrote:
> On 9/28/21 7:50 PM, Ferruh Yigit wrote:
>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>
>>> Update xstats by IDs callbacks documentation in accordance with
>>> ethdev usage of these callbacks. Document valid combinations of
>>> input arguments to make driver implementation simpler.
>>>
>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>> ---
>>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>> index 40e474aa7e..c89eefcc42 100644
>>> --- a/lib/ethdev/ethdev_driver.h
>>> +++ b/lib/ethdev/ethdev_driver.h
>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>  	struct rte_eth_xstat *stats, unsigned int n);
>>>  /**< @internal Get extended stats of an Ethernet device. */
>>>  
>>> +/**
>>> + * @internal
>>> + * Get extended stats of an Ethernet device.
>>> + *
>>> + * @param dev
>>> + *   ethdev handle of port.
>>> + * @param ids
>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>> + * @param values
>>> + *   A pointer to a table to be filled with device statistics values.
>>> + *   Must not be NULL.
>>> + * @param n
>>> + *   Element count in @p ids and @p values.
>>> + *
>>> + * @return
>>> + *   - A number of filled in stats.
>>> + *   - A negative value on error.
>>> + */
>>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>  				      const uint64_t *ids,
>>>  				      uint64_t *values,
>>>  				      unsigned int n);
>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>  
>>>  /**
>>>   * @internal
>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>>  
>>> +/**
>>> + * @internal
>>> + * Get names of extended stats of an Ethernet device.
>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>
>> Why limiting this behavior to 'xstats_get_names_by_id'?
>>
>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
>>
>> I think it is confusing to have this support for one of the '_by_id' dev_ops but
>> not for other. Why not require both to support returning 'count'?
> 
> Simply because it is dead code. There is no point to require
> from driver to have dead code.
> 

Let me step back a little, both ethdev APIs can be used to return xstats count
by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0:
'rte_eth_xstats_get_names_by_id()'
'rte_eth_xstats_get_by_id()'

But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count,
as said above I believe this selection is done unintentionally.


I am for below two options:

a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats
count, and doesn't support getting xstats count for both '_by_id' dev_ops, this
simplifies driver code. As far as I remember I suggested this before, still I
prefer this one.

b) If we will support getting xstats count from '_by_id' dev_ops, I think both
should support it, to not make it more complex to figure out which one support
what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats
count, not just one.

Re: [dpdk-stable] [PATCH v5 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

On 9/29/21 11:44 AM, Ferruh Yigit wrote:
> On 9/28/2021 5:53 PM, Andrew Rybchenko wrote:
>> On 9/28/21 7:50 PM, Ferruh Yigit wrote:
>>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>
>>>> Update xstats by IDs callbacks documentation in accordance with
>>>> ethdev usage of these callbacks. Document valid combinations of
>>>> input arguments to make driver implementation simpler.
>>>>
>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>>> ---
>>>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>> index 40e474aa7e..c89eefcc42 100644
>>>> --- a/lib/ethdev/ethdev_driver.h
>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>>  	struct rte_eth_xstat *stats, unsigned int n);
>>>>  /**< @internal Get extended stats of an Ethernet device. */
>>>>  
>>>> +/**
>>>> + * @internal
>>>> + * Get extended stats of an Ethernet device.
>>>> + *
>>>> + * @param dev
>>>> + *   ethdev handle of port.
>>>> + * @param ids
>>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>>> + * @param values
>>>> + *   A pointer to a table to be filled with device statistics values.
>>>> + *   Must not be NULL.
>>>> + * @param n
>>>> + *   Element count in @p ids and @p values.
>>>> + *
>>>> + * @return
>>>> + *   - A number of filled in stats.
>>>> + *   - A negative value on error.
>>>> + */
>>>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>>  				      const uint64_t *ids,
>>>>  				      uint64_t *values,
>>>>  				      unsigned int n);
>>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>>  
>>>>  /**
>>>>   * @internal
>>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>>>  
>>>> +/**
>>>> + * @internal
>>>> + * Get names of extended stats of an Ethernet device.
>>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>>
>>> Why limiting this behavior to 'xstats_get_names_by_id'?
>>>
>>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
>>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
>>>
>>> I think it is confusing to have this support for one of the '_by_id' dev_ops but
>>> not for other. Why not require both to support returning 'count'?
>>
>> Simply because it is dead code. There is no point to require
>> from driver to have dead code.
>>
> 
> Let me step back a little, both ethdev APIs can be used to return xstats count
> by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0:
> 'rte_eth_xstats_get_names_by_id()'
> 'rte_eth_xstats_get_by_id()'
> 
> But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count,
> as said above I believe this selection is done unintentionally.
> 
> 
> I am for below two options:
> 
> a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats
> count, and doesn't support getting xstats count for both '_by_id' dev_ops, this
> simplifies driver code. As far as I remember I suggested this before, still I
> prefer this one.
> 
> b) If we will support getting xstats count from '_by_id' dev_ops, I think both
> should support it, to not make it more complex to figure out which one support
> what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats
> count, not just one.
> 

In (b) do you suggest to change ethdev to use xstats_get_by_id
driver op if users asks for a number of xstats using
rte_eth_xstats_get_by_id()? It will complicate ethdev and
will complicate drivers. Just for the symmetry?

The patch does not change external API, does not change etcdev
bahaviour. It just clarify requirements on driver ops to
allow to check less in all drivers and support less cases
in all drivers.

If we make a one more step back, frankly speaking I think we
have too many functions to retrieve statistics. I can
understand from ethdev API point of view taking API stability
into account etc. But why do we have so many complicated
driver callbacks?

First of all I'd like to do one more cleanup:
change eth_xstats_get_names_by_id_t prototype to
have ids before xstats_names.
I.e. eth_xstats_get_by_id_t(dev, ids, values, size)
eth_xstats_get_names_by_id_t(dev, ids, names, size)

Second, merge eth_xstats_get_names_t and eth_xstats_get_names_by_id_t
callbacks to keep
name of the first, but prototype from the second.
The reason is equal functionality if ids==NULL,
shorter name of the first and optional ids (i.e. no
reason to mention optional parameter in name).
Drivers which do not implement by_id_t,
but implement eth_xstats_get_names_t, will simply
return ENOTSUP if ids!=NULL.

The case of values ops is more complicated,
however since:

2834  * There is an assumption that 'xstat_names' and 'xstats' arrays
are matched
2835  * by array index:
2836  *  xstats_names[i].name => xstats[i].value
2837  *
2838  * And the array index is same with id field of 'struct rte_eth_xstat':
2839  *  xstats[i].id == i
2840  *
2841  * This assumption makes key-value pair matching less flexible but
simpler.

we can switch to eth_xstats_get_by_id_t only callback as
well and fill in stats->id equal to index on ethdev layer.
However, it will require extra buffer for
uint64_t *values and copying in the rte_eth_xstats_get()
implementation. So, I doubt in this case.

In fact, it is sad that we still do not move forward
in accordance with Thomas presentation made 2 years ago.

Re: [dpdk-stable] [PATCH v5 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Ferruh Yigit]

On 9/29/2021 12:54 PM, Andrew Rybchenko wrote:
> On 9/29/21 11:44 AM, Ferruh Yigit wrote:
>> On 9/28/2021 5:53 PM, Andrew Rybchenko wrote:
>>> On 9/28/21 7:50 PM, Ferruh Yigit wrote:
>>>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>
>>>>> Update xstats by IDs callbacks documentation in accordance with
>>>>> ethdev usage of these callbacks. Document valid combinations of
>>>>> input arguments to make driver implementation simpler.
>>>>>
>>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>>>> ---
>>>>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>>> index 40e474aa7e..c89eefcc42 100644
>>>>> --- a/lib/ethdev/ethdev_driver.h
>>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>>>  	struct rte_eth_xstat *stats, unsigned int n);
>>>>>  /**< @internal Get extended stats of an Ethernet device. */
>>>>>  
>>>>> +/**
>>>>> + * @internal
>>>>> + * Get extended stats of an Ethernet device.
>>>>> + *
>>>>> + * @param dev
>>>>> + *   ethdev handle of port.
>>>>> + * @param ids
>>>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>>>> + * @param values
>>>>> + *   A pointer to a table to be filled with device statistics values.
>>>>> + *   Must not be NULL.
>>>>> + * @param n
>>>>> + *   Element count in @p ids and @p values.
>>>>> + *
>>>>> + * @return
>>>>> + *   - A number of filled in stats.
>>>>> + *   - A negative value on error.
>>>>> + */
>>>>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>>>  				      const uint64_t *ids,
>>>>>  				      uint64_t *values,
>>>>>  				      unsigned int n);
>>>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>>>  
>>>>>  /**
>>>>>   * @internal
>>>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>>>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>>>>  
>>>>> +/**
>>>>> + * @internal
>>>>> + * Get names of extended stats of an Ethernet device.
>>>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>>>
>>>> Why limiting this behavior to 'xstats_get_names_by_id'?
>>>>
>>>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
>>>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
>>>>
>>>> I think it is confusing to have this support for one of the '_by_id' dev_ops but
>>>> not for other. Why not require both to support returning 'count'?
>>>
>>> Simply because it is dead code. There is no point to require
>>> from driver to have dead code.
>>>
>>
>> Let me step back a little, both ethdev APIs can be used to return xstats count
>> by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0:
>> 'rte_eth_xstats_get_names_by_id()'
>> 'rte_eth_xstats_get_by_id()'
>>
>> But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count,
>> as said above I believe this selection is done unintentionally.
>>
>>
>> I am for below two options:
>>
>> a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats
>> count, and doesn't support getting xstats count for both '_by_id' dev_ops, this
>> simplifies driver code. As far as I remember I suggested this before, still I
>> prefer this one.
>>
>> b) If we will support getting xstats count from '_by_id' dev_ops, I think both
>> should support it, to not make it more complex to figure out which one support
>> what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats
>> count, not just one.
>>
> 
> In (b) do you suggest to change ethdev to use xstats_get_by_id
> driver op if users asks for a number of xstats using
> rte_eth_xstats_get_by_id()? It will complicate ethdev and
> will complicate drivers. Just for the symmetry?
> 

Not just for symmetry, but simplify the logic. Both dev_ops are very similar and
they are having different behavior with same parameters is confusing.
If you want to simplify the drivers why not go with option (a)? Option (a) also
doesn't change external API, and has only minor change in the ethdev layer.

> The patch does not change external API, does not change etcdev
> bahaviour. It just clarify requirements on driver ops to
> allow to check less in all drivers and support less cases
> in all drivers.
> 

It is not clarifying the requirements of dev_ops, but changing it. Previously
there was nothing saying only one of the '_by_id' dev_ops should support
returning element count.

> If we make a one more step back, frankly speaking I think we
> have too many functions to retrieve statistics. I can
> understand from ethdev API point of view taking API stability
> into account etc. But why do we have so many complicated
> driver callbacks?
> > First of all I'd like to do one more cleanup:
> change eth_xstats_get_names_by_id_t prototype to
> have ids before xstats_names.
> I.e. eth_xstats_get_by_id_t(dev, ids, values, size)
> eth_xstats_get_names_by_id_t(dev, ids, names, size)
> 

+1

> Second, merge eth_xstats_get_names_t and eth_xstats_get_names_by_id_t
> callbacks to keep
> name of the first, but prototype from the second.
> The reason is equal functionality if ids==NULL,
> shorter name of the first and optional ids (i.e. no
> reason to mention optional parameter in name).
> Drivers which do not implement by_id_t,
> but implement eth_xstats_get_names_t, will simply
> return ENOTSUP if ids!=NULL.
> 

No objection, _by_id() version is already superset of the other.

> The case of values ops is more complicated,
> however since:
> 
> 2834  * There is an assumption that 'xstat_names' and 'xstats' arrays
> are matched
> 2835  * by array index:
> 2836  *  xstats_names[i].name => xstats[i].value
> 2837  *
> 2838  * And the array index is same with id field of 'struct rte_eth_xstat':
> 2839  *  xstats[i].id == i
> 2840  *
> 2841  * This assumption makes key-value pair matching less flexible but
> simpler.
> 
> we can switch to eth_xstats_get_by_id_t only callback as
> well and fill in stats->id equal to index on ethdev layer.

When ids != NULL, the index from 'ids' can be used, isn't it.

> However, it will require extra buffer for
> uint64_t *values and copying in the rte_eth_xstats_get()
> implementation. So, I doubt in this case.
> 

Overall merging xstats _by_id APIs doesn't look bad idea, but since it will
require change in applications, I am not really sure if benefit worth the
trouble it brings to users.

> In fact, it is sad that we still do not move forward
> in accordance with Thomas presentation made 2 years ago.
> 

In my experience things don't move forward without proper plan (who, what, when).

Re: [dpdk-stable] [PATCH v5 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

On 9/30/21 3:08 PM, Ferruh Yigit wrote:
> On 9/29/2021 12:54 PM, Andrew Rybchenko wrote:
>> On 9/29/21 11:44 AM, Ferruh Yigit wrote:
>>> On 9/28/2021 5:53 PM, Andrew Rybchenko wrote:
>>>> On 9/28/21 7:50 PM, Ferruh Yigit wrote:
>>>>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>>>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>>
>>>>>> Update xstats by IDs callbacks documentation in accordance with
>>>>>> ethdev usage of these callbacks. Document valid combinations of
>>>>>> input arguments to make driver implementation simpler.
>>>>>>
>>>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>>>>> ---
>>>>>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>>>> index 40e474aa7e..c89eefcc42 100644
>>>>>> --- a/lib/ethdev/ethdev_driver.h
>>>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>>>>  	struct rte_eth_xstat *stats, unsigned int n);
>>>>>>  /**< @internal Get extended stats of an Ethernet device. */
>>>>>>  
>>>>>> +/**
>>>>>> + * @internal
>>>>>> + * Get extended stats of an Ethernet device.
>>>>>> + *
>>>>>> + * @param dev
>>>>>> + *   ethdev handle of port.
>>>>>> + * @param ids
>>>>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>>>>> + * @param values
>>>>>> + *   A pointer to a table to be filled with device statistics values.
>>>>>> + *   Must not be NULL.
>>>>>> + * @param n
>>>>>> + *   Element count in @p ids and @p values.
>>>>>> + *
>>>>>> + * @return
>>>>>> + *   - A number of filled in stats.
>>>>>> + *   - A negative value on error.
>>>>>> + */
>>>>>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>>>>  				      const uint64_t *ids,
>>>>>>  				      uint64_t *values,
>>>>>>  				      unsigned int n);
>>>>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>>>>  
>>>>>>  /**
>>>>>>   * @internal
>>>>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>>>>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>>>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>>>>>  
>>>>>> +/**
>>>>>> + * @internal
>>>>>> + * Get names of extended stats of an Ethernet device.
>>>>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>>>>
>>>>> Why limiting this behavior to 'xstats_get_names_by_id'?
>>>>>
>>>>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
>>>>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
>>>>>
>>>>> I think it is confusing to have this support for one of the '_by_id' dev_ops but
>>>>> not for other. Why not require both to support returning 'count'?
>>>>
>>>> Simply because it is dead code. There is no point to require
>>>> from driver to have dead code.
>>>>
>>>
>>> Let me step back a little, both ethdev APIs can be used to return xstats count
>>> by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0:
>>> 'rte_eth_xstats_get_names_by_id()'
>>> 'rte_eth_xstats_get_by_id()'
>>>
>>> But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count,
>>> as said above I believe this selection is done unintentionally.
>>>
>>>
>>> I am for below two options:
>>>
>>> a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats
>>> count, and doesn't support getting xstats count for both '_by_id' dev_ops, this
>>> simplifies driver code. As far as I remember I suggested this before, still I
>>> prefer this one.
>>>
>>> b) If we will support getting xstats count from '_by_id' dev_ops, I think both
>>> should support it, to not make it more complex to figure out which one support
>>> what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats
>>> count, not just one.
>>>
>>
>> In (b) do you suggest to change ethdev to use xstats_get_by_id
>> driver op if users asks for a number of xstats using
>> rte_eth_xstats_get_by_id()? It will complicate ethdev and
>> will complicate drivers. Just for the symmetry?
>>
> 
> Not just for symmetry, but simplify the logic. Both dev_ops are very similar and

I'm sorry, but could you point of which logic you'd
like to simply. Less requirements on driver ops
means less code required inside.

> they are having different behavior with same parameters is confusing.

Ah, logic from PMD maintainer point of view. Does it
really worse to require extra code inside because of it?

> If you want to simplify the drivers why not go with option (a)? Option (a) also
> doesn't change external API, and has only minor change in the ethdev layer.

Is two extra patches in v6 (discussed below) a step
towards (a)?

>> The patch does not change external API, does not change etcdev
>> bahaviour. It just clarify requirements on driver ops to
>> allow to check less in all drivers and support less cases
>> in all drivers.
>>
> 
> It is not clarifying the requirements of dev_ops, but changing it. Previously
> there was nothing saying only one of the '_by_id' dev_ops should support
> returning element count.

I say "clarifying" since I adjust it a real source of
truth for internals - implementation, usage of these
callback by ethdev layer.

Yes, I agree that it changes documentation.

> 
>> If we make a one more step back, frankly speaking I think we
>> have too many functions to retrieve statistics. I can
>> understand from ethdev API point of view taking API stability
>> into account etc. But why do we have so many complicated
>> driver callbacks?
>>> First of all I'd like to do one more cleanup:
>> change eth_xstats_get_names_by_id_t prototype to
>> have ids before xstats_names.
>> I.e. eth_xstats_get_by_id_t(dev, ids, values, size)
>> eth_xstats_get_names_by_id_t(dev, ids, names, size)
>>
> 
> +1

See patch 3/4 in v6

> 
>> Second, merge eth_xstats_get_names_t and eth_xstats_get_names_by_id_t
>> callbacks to keep
>> name of the first, but prototype from the second.
>> The reason is equal functionality if ids==NULL,
>> shorter name of the first and optional ids (i.e. no
>> reason to mention optional parameter in name).
>> Drivers which do not implement by_id_t,
>> but implement eth_xstats_get_names_t, will simply
>> return ENOTSUP if ids!=NULL.
>>
> 
> No objection, _by_id() version is already superset of the other.

See patch 4/4 in v6

> 
>> The case of values ops is more complicated,
>> however since:
>>
>> 2834  * There is an assumption that 'xstat_names' and 'xstats' arrays
>> are matched
>> 2835  * by array index:
>> 2836  *  xstats_names[i].name => xstats[i].value
>> 2837  *
>> 2838  * And the array index is same with id field of 'struct rte_eth_xstat':
>> 2839  *  xstats[i].id == i
>> 2840  *
>> 2841  * This assumption makes key-value pair matching less flexible but
>> simpler.
>>
>> we can switch to eth_xstats_get_by_id_t only callback as
>> well and fill in stats->id equal to index on ethdev layer.
> 
> When ids != NULL, the index from 'ids' can be used, isn't it.

Yes, of course, but above documentation is for API without IDs.

> 
>> However, it will require extra buffer for
>> uint64_t *values and copying in the rte_eth_xstats_get()
>> implementation. So, I doubt in this case.
>>
> 
> Overall merging xstats _by_id APIs doesn't look bad idea, but since it will
> require change in applications, I am not really sure if benefit worth the
> trouble it brings to users.

Yes, I agree. I'm not going to change external API.

>> In fact, it is sad that we still do not move forward
>> in accordance with Thomas presentation made 2 years ago.
>>
> 
> In my experience things don't move forward without proper plan (who, what, when).
> 

+1

[dpdk-stable] [PATCH v6 1/4] ethdev: fix docs of functions getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Document valid combinations of input arguments in accordance with
current implementation in ethdev.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/rte_ethdev.h | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index afdc53b674..2ad2bf2019 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2896,21 +2896,23 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param xstats_names
- *   An rte_eth_xstat_name array of at least *size* elements to
- *   be filled. If set to NULL, the function returns the required number
- *   of elements.
- * @param ids
- *   IDs array given by app to retrieve specific statistics
+ *   Array to be filled in with names of requested device statistics.
+ *   Must not be NULL if @p ids are specified (not NULL).
  * @param size
- *   The size of the xstats_names array (number of elements).
+ *   Number of elements in @p xstats_names array (if not NULL) and in
+ *   @p ids array (if not NULL). Must be 0 if both array pointers are NULL.
+ * @param ids
+ *   IDs array given by app to retrieve specific statistics. May be NULL to
+ *   retrieve names of all available statistics or, if @p xstats_names is
+ *   NULL as well, just the number of available statistics.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
- *   - A positive value higher than size: error, the given statistics table
+ *   - A positive value higher than size: success. The given statistics table
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int
 rte_eth_xstats_get_names_by_id(uint16_t port_id,
@@ -2923,22 +2925,23 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param ids
- *   A pointer to an ids array passed by application. This tells which
- *   statistics values function should retrieve. This parameter
- *   can be set to NULL if size is 0. In this case function will retrieve
- *   all available statistics.
+ *   IDs array given by app to retrieve specific statistics. May be NULL to
+ *   retrieve all available statistics or, if @p values is NULL as well,
+ *   just the number of available statistics.
  * @param values
- *   A pointer to a table to be filled with device statistics values.
+ *   Array to be filled in with requested device statistics.
+ *   Must not be NULL if ids are specified (not NULL).
  * @param size
- *   The size of the ids array (number of elements).
+ *   Number of elements in @p values array (if not NULL) and in @p ids
+ *   array (if not NULL). Must be 0 if both array pointers are NULL.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
- *   - A positive value higher than size: error, the given statistics table
+ *   - A positive value higher than size: success: The given statistics table
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 			     uint64_t *values, unsigned int size);
-- 
2.30.2

[dpdk-stable] [PATCH v6 2/4] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update xstats by IDs callbacks documentation in accordance with
ethdev usage of these callbacks. Document valid combinations of
input arguments to make driver implementation simpler.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index cc2c75261c..c05fdbf6fe 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -183,11 +183,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat *stats, unsigned int n);
 /**< @internal Get extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get extended stats of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param values
+ *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL.
+ * @param n
+ *   Element count in @p ids and @p values.
+ *
+ * @return
+ *   - A number of filled in stats.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
 				      const uint64_t *ids,
 				      uint64_t *values,
 				      unsigned int n);
-/**< @internal Get extended stats of an Ethernet device. */
 
 /**
  * @internal
@@ -214,10 +231,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, unsigned int size);
 /**< @internal Get names of extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get names of extended stats of an Ethernet device.
+ * For name count, set @p xstats_names and @p ids to NULL.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. Can be NULL together with @p ids to retrieve number of
+ *   available statistics.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Can be NULL together
+ *   with @p xstats_names to retrieve number of available statistics.
+ * @param size
+ *   Element count in @p ids and @p xstats_names.
+ *
+ * @return
+ *   - A number of filled in stats if both xstats_names and ids are not NULL.
+ *   - A number of available stats if both xstats_names and ids are NULL.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
 	unsigned int size);
-/**< @internal Get names of extended stats of an Ethernet device. */
 
 typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
 					     uint16_t queue_id,
-- 
2.30.2

Re: [dpdk-stable] [PATCH v5 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Ferruh Yigit]

On 9/30/2021 3:01 PM, Andrew Rybchenko wrote:
> On 9/30/21 3:08 PM, Ferruh Yigit wrote:
>> On 9/29/2021 12:54 PM, Andrew Rybchenko wrote:
>>> On 9/29/21 11:44 AM, Ferruh Yigit wrote:
>>>> On 9/28/2021 5:53 PM, Andrew Rybchenko wrote:
>>>>> On 9/28/21 7:50 PM, Ferruh Yigit wrote:
>>>>>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>>>>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>>>
>>>>>>> Update xstats by IDs callbacks documentation in accordance with
>>>>>>> ethdev usage of these callbacks. Document valid combinations of
>>>>>>> input arguments to make driver implementation simpler.
>>>>>>>
>>>>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>>>>>> Cc: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>>>>>> ---
>>>>>>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>>>>> index 40e474aa7e..c89eefcc42 100644
>>>>>>> --- a/lib/ethdev/ethdev_driver.h
>>>>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>>>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>>>>>  	struct rte_eth_xstat *stats, unsigned int n);
>>>>>>>  /**< @internal Get extended stats of an Ethernet device. */
>>>>>>>  
>>>>>>> +/**
>>>>>>> + * @internal
>>>>>>> + * Get extended stats of an Ethernet device.
>>>>>>> + *
>>>>>>> + * @param dev
>>>>>>> + *   ethdev handle of port.
>>>>>>> + * @param ids
>>>>>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>>>>>> + * @param values
>>>>>>> + *   A pointer to a table to be filled with device statistics values.
>>>>>>> + *   Must not be NULL.
>>>>>>> + * @param n
>>>>>>> + *   Element count in @p ids and @p values.
>>>>>>> + *
>>>>>>> + * @return
>>>>>>> + *   - A number of filled in stats.
>>>>>>> + *   - A negative value on error.
>>>>>>> + */
>>>>>>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>>>>>  				      const uint64_t *ids,
>>>>>>>  				      uint64_t *values,
>>>>>>>  				      unsigned int n);
>>>>>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>>>>>  
>>>>>>>  /**
>>>>>>>   * @internal
>>>>>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>>>>>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>>>>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>>>>>>  
>>>>>>> +/**
>>>>>>> + * @internal
>>>>>>> + * Get names of extended stats of an Ethernet device.
>>>>>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>>>>>
>>>>>> Why limiting this behavior to 'xstats_get_names_by_id'?
>>>>>>
>>>>>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
>>>>>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
>>>>>>
>>>>>> I think it is confusing to have this support for one of the '_by_id' dev_ops but
>>>>>> not for other. Why not require both to support returning 'count'?
>>>>>
>>>>> Simply because it is dead code. There is no point to require
>>>>> from driver to have dead code.
>>>>>
>>>>
>>>> Let me step back a little, both ethdev APIs can be used to return xstats count
>>>> by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0:
>>>> 'rte_eth_xstats_get_names_by_id()'
>>>> 'rte_eth_xstats_get_by_id()'
>>>>
>>>> But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count,
>>>> as said above I believe this selection is done unintentionally.
>>>>
>>>>
>>>> I am for below two options:
>>>>
>>>> a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats
>>>> count, and doesn't support getting xstats count for both '_by_id' dev_ops, this
>>>> simplifies driver code. As far as I remember I suggested this before, still I
>>>> prefer this one.
>>>>
>>>> b) If we will support getting xstats count from '_by_id' dev_ops, I think both
>>>> should support it, to not make it more complex to figure out which one support
>>>> what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats
>>>> count, not just one.
>>>>
>>>
>>> In (b) do you suggest to change ethdev to use xstats_get_by_id
>>> driver op if users asks for a number of xstats using
>>> rte_eth_xstats_get_by_id()? It will complicate ethdev and
>>> will complicate drivers. Just for the symmetry?
>>>
>>
>> Not just for symmetry, but simplify the logic. Both dev_ops are very similar and
> 
> I'm sorry, but could you point of which logic you'd
> like to simply. Less requirements on driver ops
> means less code required inside.
> 
>> they are having different behavior with same parameters is confusing.
> 
> Ah, logic from PMD maintainer point of view. Does it
> really worse to require extra code inside because of it?
> 
>> If you want to simplify the drivers why not go with option (a)? Option (a) also
>> doesn't change external API, and has only minor change in the ethdev layer.
> 
> Is two extra patches in v6 (discussed below) a step
> towards (a)?
> 

I am not sure of it, for (a) ethdev layer will handle getting all stats or
getting count using 'xstats_get_names' || 'xstats_get', and '*_by_id()' dev_ops
will become simpler since they won't accept NULL 'ids' & 'values'.

Also when dev_ops merged, it forces PMDs to implement the getting by ids, but
now it is covered by ethdev layer for the PMDs that doesn't implement '_by_id()'.

>>> The patch does not change external API, does not change etcdev
>>> bahaviour. It just clarify requirements on driver ops to
>>> allow to check less in all drivers and support less cases
>>> in all drivers.
>>>
>>
>> It is not clarifying the requirements of dev_ops, but changing it. Previously
>> there was nothing saying only one of the '_by_id' dev_ops should support
>> returning element count.
> 
> I say "clarifying" since I adjust it a real source of
> truth for internals - implementation, usage of these
> callback by ethdev layer.
> 
> Yes, I agree that it changes documentation.
> 
>>
>>> If we make a one more step back, frankly speaking I think we
>>> have too many functions to retrieve statistics. I can
>>> understand from ethdev API point of view taking API stability
>>> into account etc. But why do we have so many complicated
>>> driver callbacks?
>>>> First of all I'd like to do one more cleanup:
>>> change eth_xstats_get_names_by_id_t prototype to
>>> have ids before xstats_names.
>>> I.e. eth_xstats_get_by_id_t(dev, ids, values, size)
>>> eth_xstats_get_names_by_id_t(dev, ids, names, size)
>>>
>>
>> +1
> 
> See patch 3/4 in v6
> 
>>
>>> Second, merge eth_xstats_get_names_t and eth_xstats_get_names_by_id_t
>>> callbacks to keep
>>> name of the first, but prototype from the second.
>>> The reason is equal functionality if ids==NULL,
>>> shorter name of the first and optional ids (i.e. no
>>> reason to mention optional parameter in name).
>>> Drivers which do not implement by_id_t,
>>> but implement eth_xstats_get_names_t, will simply
>>> return ENOTSUP if ids!=NULL.
>>>
>>
>> No objection, _by_id() version is already superset of the other.
> 
> See patch 4/4 in v6
> 
>>
>>> The case of values ops is more complicated,
>>> however since:
>>>
>>> 2834  * There is an assumption that 'xstat_names' and 'xstats' arrays
>>> are matched
>>> 2835  * by array index:
>>> 2836  *  xstats_names[i].name => xstats[i].value
>>> 2837  *
>>> 2838  * And the array index is same with id field of 'struct rte_eth_xstat':
>>> 2839  *  xstats[i].id == i
>>> 2840  *
>>> 2841  * This assumption makes key-value pair matching less flexible but
>>> simpler.
>>>
>>> we can switch to eth_xstats_get_by_id_t only callback as
>>> well and fill in stats->id equal to index on ethdev layer.
>>
>> When ids != NULL, the index from 'ids' can be used, isn't it.
> 
> Yes, of course, but above documentation is for API without IDs.
> 
>>
>>> However, it will require extra buffer for
>>> uint64_t *values and copying in the rte_eth_xstats_get()
>>> implementation. So, I doubt in this case.
>>>
>>
>> Overall merging xstats _by_id APIs doesn't look bad idea, but since it will
>> require change in applications, I am not really sure if benefit worth the
>> trouble it brings to users.
> 
> Yes, I agree. I'm not going to change external API.
> 
>>> In fact, it is sad that we still do not move forward
>>> in accordance with Thomas presentation made 2 years ago.
>>>
>>
>> In my experience things don't move forward without proper plan (who, what, when).
>>
> 
> +1
> 

[dpdk-stable] [PATCH v7 1/5] ethdev: fix docs of functions getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Document valid combinations of input arguments in accordance with
current implementation in ethdev.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 lib/ethdev/rte_ethdev.h | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index afdc53b674..2ad2bf2019 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2896,21 +2896,23 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param xstats_names
- *   An rte_eth_xstat_name array of at least *size* elements to
- *   be filled. If set to NULL, the function returns the required number
- *   of elements.
- * @param ids
- *   IDs array given by app to retrieve specific statistics
+ *   Array to be filled in with names of requested device statistics.
+ *   Must not be NULL if @p ids are specified (not NULL).
  * @param size
- *   The size of the xstats_names array (number of elements).
+ *   Number of elements in @p xstats_names array (if not NULL) and in
+ *   @p ids array (if not NULL). Must be 0 if both array pointers are NULL.
+ * @param ids
+ *   IDs array given by app to retrieve specific statistics. May be NULL to
+ *   retrieve names of all available statistics or, if @p xstats_names is
+ *   NULL as well, just the number of available statistics.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
- *   - A positive value higher than size: error, the given statistics table
+ *   - A positive value higher than size: success. The given statistics table
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int
 rte_eth_xstats_get_names_by_id(uint16_t port_id,
@@ -2923,22 +2925,23 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param ids
- *   A pointer to an ids array passed by application. This tells which
- *   statistics values function should retrieve. This parameter
- *   can be set to NULL if size is 0. In this case function will retrieve
- *   all available statistics.
+ *   IDs array given by app to retrieve specific statistics. May be NULL to
+ *   retrieve all available statistics or, if @p values is NULL as well,
+ *   just the number of available statistics.
  * @param values
- *   A pointer to a table to be filled with device statistics values.
+ *   Array to be filled in with requested device statistics.
+ *   Must not be NULL if ids are specified (not NULL).
  * @param size
- *   The size of the ids array (number of elements).
+ *   Number of elements in @p values array (if not NULL) and in @p ids
+ *   array (if not NULL). Must be 0 if both array pointers are NULL.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
- *   - A positive value higher than size: error, the given statistics table
+ *   - A positive value higher than size: success: The given statistics table
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 			     uint64_t *values, unsigned int size);
-- 
2.30.2

[dpdk-stable] [PATCH v7 2/4] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update xstats by IDs callbacks documentation in accordance with
ethdev usage of these callbacks. Document valid combinations of
input arguments to make driver implementation simpler.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/ethdev_driver.h | 39 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index cc2c75261c..74af878bb8 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -183,11 +183,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat *stats, unsigned int n);
 /**< @internal Get extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get extended stats of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param values
+ *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL.
+ * @param n
+ *   Element count in @p ids and @p values.
+ *
+ * @return
+ *   - A number of filled in stats.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
 				      const uint64_t *ids,
 				      uint64_t *values,
 				      unsigned int n);
-/**< @internal Get extended stats of an Ethernet device. */
 
 /**
  * @internal
@@ -214,10 +231,28 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, unsigned int size);
 /**< @internal Get names of extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get names of extended stats of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. Must not be NULL.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param size
+ *   Element count in @p ids and @p xstats_names.
+ *
+ * @return
+ *   - A number of filled in stats if both xstats_names and ids are not NULL.
+ *   - A number of available stats if both xstats_names and ids are NULL.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
 	unsigned int size);
-/**< @internal Get names of extended stats of an Ethernet device. */
 
 typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
 					     uint16_t queue_id,
-- 
2.30.2

Re: [dpdk-stable] [PATCH v5 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

On 9/30/21 6:30 PM, Ferruh Yigit wrote:
> On 9/30/2021 3:01 PM, Andrew Rybchenko wrote:
>> On 9/30/21 3:08 PM, Ferruh Yigit wrote:
>>> On 9/29/2021 12:54 PM, Andrew Rybchenko wrote:
>>>> On 9/29/21 11:44 AM, Ferruh Yigit wrote:
>>>>> On 9/28/2021 5:53 PM, Andrew Rybchenko wrote:
>>>>>> On 9/28/21 7:50 PM, Ferruh Yigit wrote:
>>>>>>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>>>>>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>>>>
>>>>>>>> Update xstats by IDs callbacks documentation in accordance with
>>>>>>>> ethdev usage of these callbacks. Document valid combinations of
>>>>>>>> input arguments to make driver implementation simpler.
>>>>>>>>
>>>>>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>
>>>>>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>>>>>>> ---
>>>>>>>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>>>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>>>>>> index 40e474aa7e..c89eefcc42 100644
>>>>>>>> --- a/lib/ethdev/ethdev_driver.h
>>>>>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>>>>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>>>>>>  	struct rte_eth_xstat *stats, unsigned int n);
>>>>>>>>  /**< @internal Get extended stats of an Ethernet device. */
>>>>>>>>  
>>>>>>>> +/**
>>>>>>>> + * @internal
>>>>>>>> + * Get extended stats of an Ethernet device.
>>>>>>>> + *
>>>>>>>> + * @param dev
>>>>>>>> + *   ethdev handle of port.
>>>>>>>> + * @param ids
>>>>>>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>>>>>>> + * @param values
>>>>>>>> + *   A pointer to a table to be filled with device statistics values.
>>>>>>>> + *   Must not be NULL.
>>>>>>>> + * @param n
>>>>>>>> + *   Element count in @p ids and @p values.
>>>>>>>> + *
>>>>>>>> + * @return
>>>>>>>> + *   - A number of filled in stats.
>>>>>>>> + *   - A negative value on error.
>>>>>>>> + */
>>>>>>>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>>>>>>  				      const uint64_t *ids,
>>>>>>>>  				      uint64_t *values,
>>>>>>>>  				      unsigned int n);
>>>>>>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>>>>>>  
>>>>>>>>  /**
>>>>>>>>   * @internal
>>>>>>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>>>>>>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>>>>>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>>>>>>>  
>>>>>>>> +/**
>>>>>>>> + * @internal
>>>>>>>> + * Get names of extended stats of an Ethernet device.
>>>>>>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>>>>>>
>>>>>>> Why limiting this behavior to 'xstats_get_names_by_id'?
>>>>>>>
>>>>>>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
>>>>>>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
>>>>>>>
>>>>>>> I think it is confusing to have this support for one of the '_by_id' dev_ops but
>>>>>>> not for other. Why not require both to support returning 'count'?
>>>>>>
>>>>>> Simply because it is dead code. There is no point to require
>>>>>> from driver to have dead code.
>>>>>>
>>>>>
>>>>> Let me step back a little, both ethdev APIs can be used to return xstats count
>>>>> by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0:
>>>>> 'rte_eth_xstats_get_names_by_id()'
>>>>> 'rte_eth_xstats_get_by_id()'
>>>>>
>>>>> But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count,
>>>>> as said above I believe this selection is done unintentionally.
>>>>>
>>>>>
>>>>> I am for below two options:
>>>>>
>>>>> a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats
>>>>> count, and doesn't support getting xstats count for both '_by_id' dev_ops, this
>>>>> simplifies driver code. As far as I remember I suggested this before, still I
>>>>> prefer this one.
>>>>>
>>>>> b) If we will support getting xstats count from '_by_id' dev_ops, I think both
>>>>> should support it, to not make it more complex to figure out which one support
>>>>> what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats
>>>>> count, not just one.
>>>>>
>>>>
>>>> In (b) do you suggest to change ethdev to use xstats_get_by_id
>>>> driver op if users asks for a number of xstats using
>>>> rte_eth_xstats_get_by_id()? It will complicate ethdev and
>>>> will complicate drivers. Just for the symmetry?
>>>>
>>>
>>> Not just for symmetry, but simplify the logic. Both dev_ops are very similar and
>>
>> I'm sorry, but could you point of which logic you'd
>> like to simply. Less requirements on driver ops
>> means less code required inside.
>>
>>> they are having different behavior with same parameters is confusing.
>>
>> Ah, logic from PMD maintainer point of view. Does it
>> really worse to require extra code inside because of it?
>>
>>> If you want to simplify the drivers why not go with option (a)? Option (a) also
>>> doesn't change external API, and has only minor change in the ethdev layer.
>>
>> Is two extra patches in v6 (discussed below) a step
>> towards (a)?
>>
> 
> I am not sure of it, for (a) ethdev layer will handle getting all stats or
> getting count using 'xstats_get_names' || 'xstats_get', and '*_by_id()' dev_ops
> will become simpler since they won't accept NULL 'ids' & 'values'.

Got it. Will fix it in v7. I'll remove requirement in a
dedicated patch, but may be it should be squashed with
this one on apply.

> Also when dev_ops merged, it forces PMDs to implement the getting by ids, but
> now it is covered by ethdev layer for the PMDs that doesn't implement '_by_id()'.

Thanks, good catch. I'll handle it v7 as well.

[dpdk-stable] [PATCH v7 3/5] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update xstats by IDs callbacks documentation in accordance with
ethdev usage of these callbacks. Document valid combinations of
input arguments to make driver implementation simpler.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/ethdev_driver.h | 39 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index cc2c75261c..74af878bb8 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -183,11 +183,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat *stats, unsigned int n);
 /**< @internal Get extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get extended stats of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param values
+ *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL.
+ * @param n
+ *   Element count in @p ids and @p values.
+ *
+ * @return
+ *   - A number of filled in stats.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
 				      const uint64_t *ids,
 				      uint64_t *values,
 				      unsigned int n);
-/**< @internal Get extended stats of an Ethernet device. */
 
 /**
  * @internal
@@ -214,10 +231,28 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, unsigned int size);
 /**< @internal Get names of extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get names of extended stats of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. Must not be NULL.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param size
+ *   Element count in @p ids and @p xstats_names.
+ *
+ * @return
+ *   - A number of filled in stats if both xstats_names and ids are not NULL.
+ *   - A number of available stats if both xstats_names and ids are NULL.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
 	unsigned int size);
-/**< @internal Get names of extended stats of an Ethernet device. */
 
 typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
 					     uint16_t queue_id,
-- 
2.30.2

[dpdk-stable] [PATCH v8 1/5] ethdev: fix docs of functions getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Document valid combinations of input arguments in accordance with
current implementation in ethdev.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 lib/ethdev/rte_ethdev.h | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index afdc53b674..2ad2bf2019 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2896,21 +2896,23 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param xstats_names
- *   An rte_eth_xstat_name array of at least *size* elements to
- *   be filled. If set to NULL, the function returns the required number
- *   of elements.
- * @param ids
- *   IDs array given by app to retrieve specific statistics
+ *   Array to be filled in with names of requested device statistics.
+ *   Must not be NULL if @p ids are specified (not NULL).
  * @param size
- *   The size of the xstats_names array (number of elements).
+ *   Number of elements in @p xstats_names array (if not NULL) and in
+ *   @p ids array (if not NULL). Must be 0 if both array pointers are NULL.
+ * @param ids
+ *   IDs array given by app to retrieve specific statistics. May be NULL to
+ *   retrieve names of all available statistics or, if @p xstats_names is
+ *   NULL as well, just the number of available statistics.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
- *   - A positive value higher than size: error, the given statistics table
+ *   - A positive value higher than size: success. The given statistics table
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int
 rte_eth_xstats_get_names_by_id(uint16_t port_id,
@@ -2923,22 +2925,23 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param ids
- *   A pointer to an ids array passed by application. This tells which
- *   statistics values function should retrieve. This parameter
- *   can be set to NULL if size is 0. In this case function will retrieve
- *   all available statistics.
+ *   IDs array given by app to retrieve specific statistics. May be NULL to
+ *   retrieve all available statistics or, if @p values is NULL as well,
+ *   just the number of available statistics.
  * @param values
- *   A pointer to a table to be filled with device statistics values.
+ *   Array to be filled in with requested device statistics.
+ *   Must not be NULL if ids are specified (not NULL).
  * @param size
- *   The size of the ids array (number of elements).
+ *   Number of elements in @p values array (if not NULL) and in @p ids
+ *   array (if not NULL). Must be 0 if both array pointers are NULL.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
- *   - A positive value higher than size: error, the given statistics table
+ *   - A positive value higher than size: success: The given statistics table
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 			     uint64_t *values, unsigned int size);
-- 
2.30.2

[dpdk-stable] [PATCH v8 3/5] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update xstats by IDs callbacks documentation in accordance with
ethdev usage of these callbacks. Document valid combinations of
input arguments to make driver implementation simpler.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/ethdev_driver.h | 39 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index cc2c75261c..74af878bb8 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -183,11 +183,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat *stats, unsigned int n);
 /**< @internal Get extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get extended stats of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param values
+ *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL.
+ * @param n
+ *   Element count in @p ids and @p values.
+ *
+ * @return
+ *   - A number of filled in stats.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
 				      const uint64_t *ids,
 				      uint64_t *values,
 				      unsigned int n);
-/**< @internal Get extended stats of an Ethernet device. */
 
 /**
  * @internal
@@ -214,10 +231,28 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, unsigned int size);
 /**< @internal Get names of extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get names of extended stats of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. Must not be NULL.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param size
+ *   Element count in @p ids and @p xstats_names.
+ *
+ * @return
+ *   - A number of filled in stats if both xstats_names and ids are not NULL.
+ *   - A number of available stats if both xstats_names and ids are NULL.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
 	unsigned int size);
-/**< @internal Get names of extended stats of an Ethernet device. */
 
 typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
 					     uint16_t queue_id,
-- 
2.30.2

Re: [dpdk-stable] [PATCH v8 3/5] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Ferruh Yigit]

On 9/30/2021 5:05 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Update xstats by IDs callbacks documentation in accordance with
> ethdev usage of these callbacks. Document valid combinations of
> input arguments to make driver implementation simpler.
> 
> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
>  lib/ethdev/ethdev_driver.h | 39 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index cc2c75261c..74af878bb8 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -183,11 +183,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat *stats, unsigned int n);
>  /**< @internal Get extended stats of an Ethernet device. */
>  
> +/**
> + * @internal
> + * Get extended stats of an Ethernet device.
> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param ids
> + *   IDs array to retrieve specific statistics. Must not be NULL.
> + * @param values
> + *   A pointer to a table to be filled with device statistics values.
> + *   Must not be NULL.
> + * @param n
> + *   Element count in @p ids and @p values.
> + *
> + * @return
> + *   - A number of filled in stats.
> + *   - A negative value on error.
> + */
>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>  				      const uint64_t *ids,
>  				      uint64_t *values,
>  				      unsigned int n);
> -/**< @internal Get extended stats of an Ethernet device. */
>  
>  /**
>   * @internal
> @@ -214,10 +231,28 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>  /**< @internal Get names of extended stats of an Ethernet device. */
>  
> +/**
> + * @internal
> + * Get names of extended stats of an Ethernet device.
> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param xstats_names
> + *   An rte_eth_xstat_name array of at least *size* elements to
> + *   be filled. Must not be NULL.
> + * @param ids
> + *   IDs array to retrieve specific statistics. Must not be NULL.
> + * @param size
> + *   Element count in @p ids and @p xstats_names.
> + *
> + * @return
> + *   - A number of filled in stats if both xstats_names and ids are not NULL.
> + *   - A number of available stats if both xstats_names and ids are NULL.
> + *   - A negative value on error.
> + */

Isn't return also should be same with 'eth_xstats_get_by_id_t'?

Re: [dpdk-stable] [PATCH v8 3/5] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

On 9/30/21 7:33 PM, Ferruh Yigit wrote:
> On 9/30/2021 5:05 PM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> Update xstats by IDs callbacks documentation in accordance with
>> ethdev usage of these callbacks. Document valid combinations of
>> input arguments to make driver implementation simpler.
>>
>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>> ---
>>  lib/ethdev/ethdev_driver.h | 39 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>> index cc2c75261c..74af878bb8 100644
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -183,11 +183,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>  	struct rte_eth_xstat *stats, unsigned int n);
>>  /**< @internal Get extended stats of an Ethernet device. */
>>  
>> +/**
>> + * @internal
>> + * Get extended stats of an Ethernet device.
>> + *
>> + * @param dev
>> + *   ethdev handle of port.
>> + * @param ids
>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>> + * @param values
>> + *   A pointer to a table to be filled with device statistics values.
>> + *   Must not be NULL.
>> + * @param n
>> + *   Element count in @p ids and @p values.
>> + *
>> + * @return
>> + *   - A number of filled in stats.
>> + *   - A negative value on error.
>> + */
>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>  				      const uint64_t *ids,
>>  				      uint64_t *values,
>>  				      unsigned int n);
>> -/**< @internal Get extended stats of an Ethernet device. */
>>  
>>  /**
>>   * @internal
>> @@ -214,10 +231,28 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>  
>> +/**
>> + * @internal
>> + * Get names of extended stats of an Ethernet device.
>> + *
>> + * @param dev
>> + *   ethdev handle of port.
>> + * @param xstats_names
>> + *   An rte_eth_xstat_name array of at least *size* elements to
>> + *   be filled. Must not be NULL.
>> + * @param ids
>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>> + * @param size
>> + *   Element count in @p ids and @p xstats_names.
>> + *
>> + * @return
>> + *   - A number of filled in stats if both xstats_names and ids are not NULL.
>> + *   - A number of available stats if both xstats_names and ids are NULL.
>> + *   - A negative value on error.
>> + */
> 
> Isn't return also should be same with 'eth_xstats_get_by_id_t'?
> 

Thanks, forgot about it. Thanks, fixed in v9.

[dpdk-stable] [PATCH v9 1/5] ethdev: fix docs of functions getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Document valid combinations of input arguments in accordance with
current implementation in ethdev.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 lib/ethdev/rte_ethdev.h | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index afdc53b674..2ad2bf2019 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2896,21 +2896,23 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param xstats_names
- *   An rte_eth_xstat_name array of at least *size* elements to
- *   be filled. If set to NULL, the function returns the required number
- *   of elements.
- * @param ids
- *   IDs array given by app to retrieve specific statistics
+ *   Array to be filled in with names of requested device statistics.
+ *   Must not be NULL if @p ids are specified (not NULL).
  * @param size
- *   The size of the xstats_names array (number of elements).
+ *   Number of elements in @p xstats_names array (if not NULL) and in
+ *   @p ids array (if not NULL). Must be 0 if both array pointers are NULL.
+ * @param ids
+ *   IDs array given by app to retrieve specific statistics. May be NULL to
+ *   retrieve names of all available statistics or, if @p xstats_names is
+ *   NULL as well, just the number of available statistics.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
- *   - A positive value higher than size: error, the given statistics table
+ *   - A positive value higher than size: success. The given statistics table
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int
 rte_eth_xstats_get_names_by_id(uint16_t port_id,
@@ -2923,22 +2925,23 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param ids
- *   A pointer to an ids array passed by application. This tells which
- *   statistics values function should retrieve. This parameter
- *   can be set to NULL if size is 0. In this case function will retrieve
- *   all available statistics.
+ *   IDs array given by app to retrieve specific statistics. May be NULL to
+ *   retrieve all available statistics or, if @p values is NULL as well,
+ *   just the number of available statistics.
  * @param values
- *   A pointer to a table to be filled with device statistics values.
+ *   Array to be filled in with requested device statistics.
+ *   Must not be NULL if ids are specified (not NULL).
  * @param size
- *   The size of the ids array (number of elements).
+ *   Number of elements in @p values array (if not NULL) and in @p ids
+ *   array (if not NULL). Must be 0 if both array pointers are NULL.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
- *   - A positive value higher than size: error, the given statistics table
+ *   - A positive value higher than size: success: The given statistics table
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 			     uint64_t *values, unsigned int size);
-- 
2.30.2

[dpdk-stable] [PATCH v9 3/5] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update xstats by IDs callbacks documentation in accordance with
ethdev usage of these callbacks. Document valid combinations of
input arguments to make driver implementation simpler.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
v9:
    - cleanup return values documenation for eth_xstats_get_names_by_id_t

 lib/ethdev/ethdev_driver.h | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index cc2c75261c..81dee87586 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -183,11 +183,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat *stats, unsigned int n);
 /**< @internal Get extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get extended stats of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param values
+ *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL.
+ * @param n
+ *   Element count in @p ids and @p values.
+ *
+ * @return
+ *   - A number of filled in stats.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
 				      const uint64_t *ids,
 				      uint64_t *values,
 				      unsigned int n);
-/**< @internal Get extended stats of an Ethernet device. */
 
 /**
  * @internal
@@ -214,10 +231,27 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, unsigned int size);
 /**< @internal Get names of extended stats of an Ethernet device. */
 
+/**
+ * @internal
+ * Get names of extended stats of an Ethernet device.
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least @p size elements to be filled.
+ *   Must not be NULL.
+ * @param ids
+ *   IDs array to retrieve specific statistics. Must not be NULL.
+ * @param size
+ *   Element count in @p ids and @p xstats_names.
+ *
+ * @return
+ *   - A number of filled in stats.
+ *   - A negative value on error.
+ */
 typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
 	unsigned int size);
-/**< @internal Get names of extended stats of an Ethernet device. */
 
 typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
 					     uint16_t queue_id,
-- 
2.30.2

Re: [dpdk-stable] [PATCH v9 3/5] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Ferruh Yigit]

On 10/1/2021 10:07 AM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Update xstats by IDs callbacks documentation in accordance with
> ethdev usage of these callbacks. Document valid combinations of
> input arguments to make driver implementation simpler.
> 
> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-stable] [PATCH v9 3/5] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Andrew Rybchenko]

On 10/1/21 12:42 PM, Ferruh Yigit wrote:
> On 10/1/2021 10:07 AM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> Update xstats by IDs callbacks documentation in accordance with
>> ethdev usage of these callbacks. Document valid combinations of
>> input arguments to make driver implementation simpler.
>>
>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 

Ah, I guess we should drop Fixes and Cc to stable here
since it depends on 2/5 which is not a fix and should
not be backported.

Re: [dpdk-stable] [PATCH v9 3/5] ethdev: fix docs of drivers callbacks getting xstats by IDs

[Ferruh Yigit]

On 10/1/2021 11:22 AM, Andrew Rybchenko wrote:
> On 10/1/21 12:42 PM, Ferruh Yigit wrote:
>> On 10/1/2021 10:07 AM, Andrew Rybchenko wrote:
>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>
>>> Update xstats by IDs callbacks documentation in accordance with
>>> ethdev usage of these callbacks. Document valid combinations of
>>> input arguments to make driver implementation simpler.
>>>
>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>
>> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>
> 
> Ah, I guess we should drop Fixes and Cc to stable here
> since it depends on 2/5 which is not a fix and should
> not be backported.
> 

Agree, also patch is changing the behavior, not exactly fixing,
so will change the patch title too as s/: fix/: update/

Re: [dpdk-stable] [PATCH v9 1/5] ethdev: fix docs of functions getting xstats by IDs

[Ferruh Yigit]

On 10/1/2021 10:07 AM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Document valid combinations of input arguments in accordance with
> current implementation in ethdev.
> 
> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Except from last patch (5/5),
Series applied to dpdk-next-net/main, thanks.