[dpdk-dev] [PATCH v2] ethdev: fix xstats retrieve by id API

[Lee Daly]

From: Lee <lee.daly at intel.com>

Fix xstats functions, rte_eth_xstats_get_names_by_id()
and rte_eth_xstats_get_by_id(), in current implementation
ethdev level reads all xstat values and filters out
the ones requested by the application. This behavior doesn't
benefit from PMD ops and doesn't provide the benefit the
API was created in the first place for. APIs are also unnecessarily
complicated. Both APIs have different returns for the same params.

In this fix, instead of reading all the stats and finding the
requested value, drivers can provide ops to get selected xstats.
API no longer crashes with certain params,

rte_eth_get_by_id returned seg fault with
"ids = NULL && values != NULL && n<max”
rte_eth_get_names_by_id returned seg fault with
"ids = NULL && values != NULL && n=0”
These now return max number of stats available, matching the other API.

rte_eth_get_by_id returned seg fault with
"ids != NULL && values = NULL && n<max”
This now returns -22,(EINVAL).

Standardized variable/parameter names between the 2 APIs.

Overall code complexity reduced.

Fixes: 79c913a42f0e ("ethdev: retrieve xstats by ID")
Cc: kubax.kozak at intel.com

Signed-off-by: Lee Daly <lee.daly at intel.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit at intel.com>
---
v2:
+Updated headerfile function doxygen comment for rte_eth_xstats_get_by_id()

 lib/librte_ether/rte_ethdev.c | 291 ++++++++++++------------------------------
 lib/librte_ether/rte_ethdev.h |  10 +-
 2 files changed, 87 insertions(+), 214 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index f20cb25..e0a7dac 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1621,121 +1621,69 @@ rte_eth_xstats_get_id_by_name(uint16_t port_id, const char *xstat_name,
 	return -EINVAL;
 }
 
+/* retrieve ethdev extended statistics names */
 int
 rte_eth_xstats_get_names_by_id(uint16_t port_id,
 	struct rte_eth_xstat_name *xstats_names, unsigned int size,
 	uint64_t *ids)
 {
-	/* Get all xstats */
+	struct rte_eth_xstat_name *xstats_names_copy;
+	unsigned int expected_entries;
+	struct rte_eth_dev *dev;
+	unsigned int i;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	expected_entries = get_xstats_count(port_id);
+	dev = &rte_eth_devices[port_id];
+
+	/* Return max number of stats if no ids given */
 	if (!ids) {
-		struct rte_eth_dev *dev;
-		int cnt_used_entries;
-		int cnt_expected_entries;
-		int cnt_driver_entries;
-		uint32_t idx, id_queue;
-		uint16_t num_q;
-
-		cnt_expected_entries = get_xstats_count(port_id);
-		if (xstats_names == NULL || cnt_expected_entries < 0 ||
-				(int)size < cnt_expected_entries)
-			return cnt_expected_entries;
-
-		/* port_id checked in get_xstats_count() */
-		dev = &rte_eth_devices[port_id];
-		cnt_used_entries = 0;
-
-		for (idx = 0; idx < RTE_NB_STATS; idx++) {
-			snprintf(xstats_names[cnt_used_entries].name,
-				sizeof(xstats_names[0].name),
-				"%s", rte_stats_strings[idx].name);
-			cnt_used_entries++;
-		}
-		num_q = RTE_MIN(dev->data->nb_rx_queues,
-				RTE_ETHDEV_QUEUE_STAT_CNTRS);
-		for (id_queue = 0; id_queue < num_q; id_queue++) {
-			for (idx = 0; idx < RTE_NB_RXQ_STATS; idx++) {
-				snprintf(xstats_names[cnt_used_entries].name,
-					sizeof(xstats_names[0].name),
-					"rx_q%u%s",
-					id_queue,
-					rte_rxq_stats_strings[idx].name);
-				cnt_used_entries++;
-			}
+		if (!xstats_names)
+			return expected_entries;
+		else if (xstats_names && size < expected_entries)
+			return expected_entries;
+	}
 
-		}
-		num_q = RTE_MIN(dev->data->nb_tx_queues,
-				RTE_ETHDEV_QUEUE_STAT_CNTRS);
-		for (id_queue = 0; id_queue < num_q; id_queue++) {
-			for (idx = 0; idx < RTE_NB_TXQ_STATS; idx++) {
-				snprintf(xstats_names[cnt_used_entries].name,
-					sizeof(xstats_names[0].name),
-					"tx_q%u%s",
-					id_queue,
-					rte_txq_stats_strings[idx].name);
-				cnt_used_entries++;
-			}
-		}
+	if (ids && !xstats_names)
+		return -EINVAL;
 
-		if (dev->dev_ops->xstats_get_names_by_id != NULL) {
-			/* If there are any driver-specific xstats, append them
-			 * to end of list.
-			 */
-			cnt_driver_entries =
-				(*dev->dev_ops->xstats_get_names_by_id)(
-				dev,
-				xstats_names + cnt_used_entries,
-				NULL,
-				size - cnt_used_entries);
-			if (cnt_driver_entries < 0)
-				return cnt_driver_entries;
-			cnt_used_entries += cnt_driver_entries;
-
-		} else if (dev->dev_ops->xstats_get_names != NULL) {
-			/* If there are any driver-specific xstats, append them
-			 * to end of list.
-			 */
-			cnt_driver_entries = (*dev->dev_ops->xstats_get_names)(
-				dev,
-				xstats_names + cnt_used_entries,
-				size - cnt_used_entries);
-			if (cnt_driver_entries < 0)
-				return cnt_driver_entries;
-			cnt_used_entries += cnt_driver_entries;
-		}
+	if (dev->dev_ops->xstats_get_names_by_id != NULL)
+		return (*dev->dev_ops->xstats_get_names_by_id)(
+				dev, xstats_names, ids, size);
 
-		return cnt_used_entries;
+	/* Retrieve all stats */
+	if (!ids) {
+		int num_stats = rte_eth_xstats_get_names(port_id, xstats_names,
+				expected_entries);
+		if (num_stats < 0 || num_stats > (int)expected_entries)
+			return num_stats;
+		else
+			return expected_entries;
 	}
-	/* Get only xstats given by IDS */
-	else {
-		uint16_t len, i;
-		struct rte_eth_xstat_name *xstats_names_copy;
 
-		len = rte_eth_xstats_get_names_by_id(port_id, NULL, 0, NULL);
+	xstats_names_copy = calloc(expected_entries,
+		sizeof(struct rte_eth_xstat_name));
 
-		xstats_names_copy =
-				malloc(sizeof(struct rte_eth_xstat_name) * len);
-		if (!xstats_names_copy) {
-			RTE_PMD_DEBUG_TRACE(
-			     "ERROR: can't allocate memory for values_copy\n");
+	if (!xstats_names_copy) {
+		RTE_PMD_DEBUG_TRACE("ERROR: can't allocate memory");
+		return -ENOMEM;
+	}
+
+	/* Fill xstats_names_copy structure */
+	rte_eth_xstats_get_names(port_id, xstats_names_copy, expected_entries);
+
+	/* Filter stats */
+	for (i = 0; i < size; i++) {
+		if (ids[i] >= expected_entries) {
+			RTE_PMD_DEBUG_TRACE("ERROR: id value isn't valid\n");
 			free(xstats_names_copy);
 			return -1;
 		}
-
-		rte_eth_xstats_get_names_by_id(port_id, xstats_names_copy,
-				len, NULL);
-
-		for (i = 0; i < size; i++) {
-			if (ids[i] >= len) {
-				RTE_PMD_DEBUG_TRACE(
-					"ERROR: id value isn't valid\n");
-				return -1;
-			}
-			strcpy(xstats_names[i].name,
-					xstats_names_copy[ids[i]].name);
-		}
-		free(xstats_names_copy);
-		return size;
+		xstats_names[i] = xstats_names_copy[ids[i]];
 	}
+
+	free(xstats_names_copy);
+	return size;
 }
 
 int
@@ -1806,128 +1754,53 @@ rte_eth_xstats_get_names(uint16_t port_id,
 /* retrieve ethdev extended statistics */
 int
 rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
-			 uint64_t *values, unsigned int n)
+			 uint64_t *values, unsigned int size)
 {
-	/* If need all xstats */
-	if (!ids) {
-		struct rte_eth_stats eth_stats;
-		struct rte_eth_dev *dev;
-		unsigned int count = 0, i, q;
-		signed int xcount = 0;
-		uint64_t val, *stats_ptr;
-		uint16_t nb_rxqs, nb_txqs;
-
-		RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
-		dev = &rte_eth_devices[port_id];
-
-		nb_rxqs = RTE_MIN(dev->data->nb_rx_queues,
-				RTE_ETHDEV_QUEUE_STAT_CNTRS);
-		nb_txqs = RTE_MIN(dev->data->nb_tx_queues,
-				RTE_ETHDEV_QUEUE_STAT_CNTRS);
-
-		/* Return generic statistics */
-		count = RTE_NB_STATS + (nb_rxqs * RTE_NB_RXQ_STATS) +
-			(nb_txqs * RTE_NB_TXQ_STATS);
-
-
-		/* implemented by the driver */
-		if (dev->dev_ops->xstats_get_by_id != NULL) {
-			/* Retrieve the xstats from the driver at the end of the
-			 * xstats struct. Retrieve all xstats.
-			 */
-			xcount = (*dev->dev_ops->xstats_get_by_id)(dev,
-					NULL,
-					values ? values + count : NULL,
-					(n > count) ? n - count : 0);
-
-			if (xcount < 0)
-				return xcount;
-		/* implemented by the driver */
-		} else if (dev->dev_ops->xstats_get != NULL) {
-			/* Retrieve the xstats from the driver at the end of the
-			 * xstats struct. Retrieve all xstats.
-			 * Compatibility for PMD without xstats_get_by_ids
-			 */
-			unsigned int size = (n > count) ? n - count : 1;
-			struct rte_eth_xstat xstats[size];
-
-			xcount = (*dev->dev_ops->xstats_get)(dev,
-					values ? xstats : NULL,	size);
-
-			if (xcount < 0)
-				return xcount;
-
-			if (values != NULL)
-				for (i = 0 ; i < (unsigned int)xcount; i++)
-					values[i + count] = xstats[i].value;
-		}
+	unsigned int num_xstats_filled;
+	uint16_t expected_entries;
+	struct rte_eth_dev *dev;
+	unsigned int i;
 
-		if (n < count + xcount || values == NULL)
-			return count + xcount;
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	expected_entries = get_xstats_count(port_id);
+	struct rte_eth_xstat xstats[expected_entries];
+	dev = &rte_eth_devices[port_id];
 
-		/* now fill the xstats structure */
-		count = 0;
-		rte_eth_stats_get(port_id, &eth_stats);
+	/* Return max number of stats if no ids given */
+	if (!ids) {
+		if (!values)
+			return expected_entries;
+		else if (values && size < expected_entries)
+			return expected_entries;
+	}
 
-		/* global stats */
-		for (i = 0; i < RTE_NB_STATS; i++) {
-			stats_ptr = RTE_PTR_ADD(&eth_stats,
-						rte_stats_strings[i].offset);
-			val = *stats_ptr;
-			values[count++] = val;
-		}
+	if (ids && !values)
+		return -EINVAL;
 
-		/* per-rxq stats */
-		for (q = 0; q < nb_rxqs; q++) {
-			for (i = 0; i < RTE_NB_RXQ_STATS; i++) {
-				stats_ptr = RTE_PTR_ADD(&eth_stats,
-					    rte_rxq_stats_strings[i].offset +
-					    q * sizeof(uint64_t));
-				val = *stats_ptr;
-				values[count++] = val;
-			}
-		}
+	if (dev->dev_ops->xstats_get_by_id != NULL)
+		return (*dev->dev_ops->xstats_get_by_id)(dev, ids, values,
+			size);
 
-		/* per-txq stats */
-		for (q = 0; q < nb_txqs; q++) {
-			for (i = 0; i < RTE_NB_TXQ_STATS; i++) {
-				stats_ptr = RTE_PTR_ADD(&eth_stats,
-					    rte_txq_stats_strings[i].offset +
-					    q * sizeof(uint64_t));
-				val = *stats_ptr;
-				values[count++] = val;
-			}
-		}
+	/* Fill the xstats structure */
+	num_xstats_filled = rte_eth_xstats_get(port_id, xstats,
+		expected_entries);
 
-		return count + xcount;
+	/* Return all stats */
+	if (!ids) {
+		for (i = 0; i < num_xstats_filled; i++)
+			values[i] = xstats[i].value;
+		return expected_entries;
 	}
-	/* Need only xstats given by IDS array */
-	else {
-		uint16_t i, size;
-		uint64_t *values_copy;
-
-		size = rte_eth_xstats_get_by_id(port_id, NULL, NULL, 0);
 
-		values_copy = malloc(sizeof(*values_copy) * size);
-		if (!values_copy) {
-			RTE_PMD_DEBUG_TRACE(
-			    "ERROR: can't allocate memory for values_copy\n");
+	/* Filter stats */
+	for (i = 0; i < size; i++) {
+		if (ids[i] >= expected_entries) {
+			RTE_PMD_DEBUG_TRACE("ERROR: id value isn't valid\n");
 			return -1;
 		}
-
-		rte_eth_xstats_get_by_id(port_id, NULL, values_copy, size);
-
-		for (i = 0; i < n; i++) {
-			if (ids[i] >= size) {
-				RTE_PMD_DEBUG_TRACE(
-					"ERROR: id value isn't valid\n");
-				return -1;
-			}
-			values[i] = values_copy[ids[i]];
-		}
-		free(values_copy);
-		return n;
+		values[i] = xstats[ids[i]].value;
 	}
+	return size;
 }
 
 int
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 8bc1477..cc9427e 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2456,23 +2456,23 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  * @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 n is 0. In this case function will retrieve
+ *   can be set to NULL if size is 0. In this case function will retrieve
  *   all avalible statistics.
  * @param values
  *   A pointer to a table to be filled with device statistics values.
- * @param n
+ * @param size
  *   The size of the ids array (number of elements).
  * @return
- *   - A positive value lower or equal to n: success. The return value
+ *   - 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 n: error, the given statistics table
+ *   - 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.
  *   - A negative value on error (invalid port id).
  */
 int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
-			     uint64_t *values, unsigned int n);
+			     uint64_t *values, unsigned int size);
 
 /**
  * Gets the ID of a statistic from its name.
-- 
2.9.5