[PATCH] ethdev: fix xstats retrieve by id API

* [PATCH] ethdev: fix xstats retrieve by id API
@ 2017-10-11  7:22 Lee Daly
  2017-10-11 18:33 ` Ferruh Yigit
  2017-10-12 13:31 ` [PATCH v2] " Lee Daly
  0 siblings, 2 replies; 4+ messages in thread
From: Lee Daly @ 2017-10-11  7:22 UTC (permalink / raw)
  To: thomas; +Cc: dev, ferruh.yigit, Lee

From: Lee <lee.daly@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.

Signed-off-by: Lee <lee.daly@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 291 ++++++++++++------------------------------
 lib/librte_ether/rte_ethdev.h |   2 +-
 2 files changed, 83 insertions(+), 210 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..4efa392 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2472,7 +2472,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
  *   - 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

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