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

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

[Lee Daly]

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

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

[Ferruh Yigit]

On 10/11/2017 8:22 AM, Lee Daly wrote:
> 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.

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

> 
> Signed-off-by: Lee <lee.daly@intel.com>

Please use full name:
Signed-off-by: Lee Daly <lee.daly@intel.com>

<...>

> @@ -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);

Function doxygen comment also should be updated to reflect this,
otherwise giving a warning.

>  
>  /**
>   * Gets the ID of a statistic from its name.
> 

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

Can you please send a new version addressing above issues, feel free to
keep review tag.

Thanks,
ferruh

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

[Lee Daly]

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.

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

Signed-off-by: Lee Daly <lee.daly@intel.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@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

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

[Ferruh Yigit]

On 10/12/2017 2:31 PM, Lee Daly wrote:
> 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.
> 
> Fixes: 79c913a42f0e ("ethdev: retrieve xstats by ID")
> Cc: kubax.kozak@intel.com
> 
> Signed-off-by: Lee Daly <lee.daly@intel.com>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/master, thanks.

Welcome Lee!

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

[Ivan Malov]

On 10/12/2017 2:31 PM, Lee Daly wrote:
> 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.
>
> Fixes: 79c913a42f0e ("ethdev: retrieve xstats by ID")
> Cc: kubax.kozak@intel.com
>
> Signed-off-by: Lee Daly <lee.daly@intel.com>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

I have a serious concern regarding the patch. There is a common scenario
of rte_eth_xstats_get_names_by_id() usage, and the patch breaks it.
Typically, the function is called with 'ids = NULL' and 'xstats_names = NULL'
in order to get the number of figures. Then the function is called one more
time with appropriate storage for 'xstats_names'. According to the patch,
on the former step get_xstats_count() is called to count the figures available.
The resulting number counts for PMD statistics + RTE_NB_STATS + some amount of
per-queue figures. However, on the latter step the following may take place:

> +	if (dev->dev_ops->xstats_get_names_by_id != NULL)
> +		return (*dev->dev_ops->xstats_get_names_by_id)(
> +				dev, xstats_names, ids, size);

This obviously means that in the case when 'xstats_get_names_by_id' is present,
it will be called directly, and RTE statistics will not be filled in the storage
before the PMD figures. Accordingly, the value returned by the function in this
case will count for PMD figures only (i.e. will be less than the value obtained
on the first step). This is an obvious malfunction, and it would be desirable
to provide a fix for that. I hope for your understanding.

-- 
Best regards,
Ivan

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

[Ferruh Yigit]

On 10/18/2017 4:10 AM, Ivan Malov wrote:
> On 10/12/2017 2:31 PM, Lee Daly wrote:
>> 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.
>>
>> Fixes: 79c913a42f0e ("ethdev: retrieve xstats by ID")
>> Cc: kubax.kozak@intel.com
>>
>> Signed-off-by: Lee Daly <lee.daly@intel.com>
>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> I have a serious concern regarding the patch. There is a common scenario
> of rte_eth_xstats_get_names_by_id() usage, and the patch breaks it.
> Typically, the function is called with 'ids = NULL' and 'xstats_names = NULL'
> in order to get the number of figures. Then the function is called one more
> time with appropriate storage for 'xstats_names'. According to the patch,
> on the former step get_xstats_count() is called to count the figures available.
> The resulting number counts for PMD statistics + RTE_NB_STATS + some amount of
> per-queue figures. However, on the latter step the following may take place:
> 
>> +	if (dev->dev_ops->xstats_get_names_by_id != NULL)
>> +		return (*dev->dev_ops->xstats_get_names_by_id)(
>> +				dev, xstats_names, ids, size);
> 
> This obviously means that in the case when 'xstats_get_names_by_id' is present,
> it will be called directly, and RTE statistics will not be filled in the storage
> before the PMD figures. Accordingly, the value returned by the function in this
> case will count for PMD figures only (i.e. will be less than the value obtained
> on the first step). This is an obvious malfunction, and it would be desirable
> to provide a fix for that. I hope for your understanding.

Hi Ivan,

You are right, this is broken.

But the problem is, ethdev API works with ids which are "basic + xstat" and PMD dev_ops
works with ids which are "xstat". This mismatch ends up xstats_get_names_by_id() dev_ops
being called to get all values, and filtering done in ethdev API, as done in previous
implementation.

If we use xstats_get_names_by_id() to get all values, why we have it at all, there is
already a dev_ops to get all values.

I believe we have two options,

1- Remove this by _by_id() dev_ops, but keep ethdev APIs, and APIs does the filtering of
the ids. This may be missing if any PMD has an optimal way of getting subset of ids, this
is the main reason of the devops.

2- Convert ids for PMD usage, what do you think about following fix:
(same thing for rte_eth_xstats_get_by_id() also of course)

  diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
  index 0b1e92894..1145539ee 100644
  --- a/lib/librte_ether/rte_ethdev.c
  +++ b/lib/librte_ether/rte_ethdev.c
  @@ -1631,6 +1631,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
   {
          struct rte_eth_xstat_name *xstats_names_copy;
          unsigned int expected_entries;
  +       unsigned int all_ids_from_pmd = 1;
          struct rte_eth_dev *dev;
          unsigned int i;

  @@ -1649,9 +1650,28 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
          if (ids && !xstats_names)
                  return -EINVAL;

  -       if (dev->dev_ops->xstats_get_names_by_id != NULL)
  -               return (*dev->dev_ops->xstats_get_names_by_id)(
  -                               dev, xstats_names, ids, size);
  +       if (ids && dev->dev_ops->xstats_get_names_by_id != NULL && size > 0) {
  +               uint64_t ids_copy[size];
  +               unsigned int pmd_count;
  +               unsigned int count;
  +
  +               pmd_count = (*dev->dev_ops->xstats_get_names_by_id)(dev, NULL,
  +                                       NULL, 0);
  +               count = expected_entries - pmd_count;
  +
  +               for (i = 0; i < size; i++) {
  +                       if (ids[i] < count) {
  +                               all_ids_from_pmd = 0;
  +                               break;
  +                       }
  +
  +                       ids_copy[i] = ids[i] - count;
  +               }
  +
  +               if (all_ids_from_pmd)
  +                       return (*dev->dev_ops->xstats_get_names_by_id)(dev,
  +                                       xstats_names, ids_copy, size);
  +       }

          /* Retrieve all stats */
          if (!ids) {

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

[Van Haaren, Harry]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Wednesday, October 18, 2017 10:49 PM
> To: Ivan Malov <Ivan.Malov@oktetlabs.ru>; Daly, Lee <lee.daly@intel.com>;
> thomas@monjalon.net
> Cc: dev@dpdk.org; Kozak, KubaX <kubax.kozak@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix xstats retrieve by id API
> 
> On 10/18/2017 4:10 AM, Ivan Malov wrote:
> > On 10/12/2017 2:31 PM, Lee Daly wrote:
> >> 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.
> >>
> >> Fixes: 79c913a42f0e ("ethdev: retrieve xstats by ID")
> >> Cc: kubax.kozak@intel.com
> >>
> >> Signed-off-by: Lee Daly <lee.daly@intel.com>
> >> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >
> > I have a serious concern regarding the patch. There is a common scenario
> > of rte_eth_xstats_get_names_by_id() usage, and the patch breaks it.
> > Typically, the function is called with 'ids = NULL' and 'xstats_names =
> NULL'
> > in order to get the number of figures. Then the function is called one
> more
> > time with appropriate storage for 'xstats_names'. According to the patch,
> > on the former step get_xstats_count() is called to count the figures
> available.
> > The resulting number counts for PMD statistics + RTE_NB_STATS + some
> amount of
> > per-queue figures. However, on the latter step the following may take
> place:
> >
> >> +	if (dev->dev_ops->xstats_get_names_by_id != NULL)
> >> +		return (*dev->dev_ops->xstats_get_names_by_id)(
> >> +				dev, xstats_names, ids, size);
> >
> > This obviously means that in the case when 'xstats_get_names_by_id' is
> present,
> > it will be called directly, and RTE statistics will not be filled in the
> storage
> > before the PMD figures. Accordingly, the value returned by the function in
> this
> > case will count for PMD figures only (i.e. will be less than the value
> obtained
> > on the first step). This is an obvious malfunction, and it would be
> desirable
> > to provide a fix for that. I hope for your understanding.
> 
> Hi Ivan,
> 
> You are right, this is broken.
> 
> But the problem is, ethdev API works with ids which are "basic + xstat" and
> PMD dev_ops
> works with ids which are "xstat". This mismatch ends up
> xstats_get_names_by_id() dev_ops
> being called to get all values, and filtering done in ethdev API, as done in
> previous
> implementation.
> 
> If we use xstats_get_names_by_id() to get all values, why we have it at all,
> there is
> already a dev_ops to get all values.
> 
> I believe we have two options,
> 
> 1- Remove this by _by_id() dev_ops, but keep ethdev APIs, and APIs does the
> filtering of
> the ids. This may be missing if any PMD has an optimal way of getting subset
> of ids, this
> is the main reason of the devops.


Lets not remove the _by_id() opts, because that removes the possibility for PMDs to optimize the get_by_id() case.


> 2- Convert ids for PMD usage, what do you think about following fix:
> (same thing for rte_eth_xstats_get_by_id() also of course)


A quick test of this patch allows proc-info to work again to dump xstats of a primary, eg testpmd. Run testpmd first and then proc-info:

testpmd: ./testpmd -- -i
proc-info: ./dpdk-procinfo  -- --xstats

Before patch:
Cannot get xstat names
Cannot get xstat names

After patch:
<correct xstats output>

I suggest we use Ferruh's fix, and apply ASAP unless @Ivan has a better solution/suggestion?



>   diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>   index 0b1e92894..1145539ee 100644
>   --- a/lib/librte_ether/rte_ethdev.c
>   +++ b/lib/librte_ether/rte_ethdev.c
>   @@ -1631,6 +1631,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>    {
>           struct rte_eth_xstat_name *xstats_names_copy;
>           unsigned int expected_entries;
>   +       unsigned int all_ids_from_pmd = 1;
>           struct rte_eth_dev *dev;
>           unsigned int i;
> 
>   @@ -1649,9 +1650,28 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>           if (ids && !xstats_names)
>                   return -EINVAL;
> 
>   -       if (dev->dev_ops->xstats_get_names_by_id != NULL)
>   -               return (*dev->dev_ops->xstats_get_names_by_id)(
>   -                               dev, xstats_names, ids, size);
>   +       if (ids && dev->dev_ops->xstats_get_names_by_id != NULL && size >
> 0) {
>   +               uint64_t ids_copy[size];
>   +               unsigned int pmd_count;
>   +               unsigned int count;
>   +
>   +               pmd_count = (*dev->dev_ops->xstats_get_names_by_id)(dev,
> NULL,
>   +                                       NULL, 0);
>   +               count = expected_entries - pmd_count;
>   +
>   +               for (i = 0; i < size; i++) {
>   +                       if (ids[i] < count) {
>   +                               all_ids_from_pmd = 0;
>   +                               break;
>   +                       }
>   +
>   +                       ids_copy[i] = ids[i] - count;
>   +               }
>   +
>   +               if (all_ids_from_pmd)
>   +                       return (*dev->dev_ops-
> >xstats_get_names_by_id)(dev,
>   +                                       xstats_names, ids_copy, size);
>   +       }
> 
>           /* Retrieve all stats */
>           if (!ids) {

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

[Ivan Malov]

On Thu, Oct 19, 2017 at 11:06:37AM +0000, Van Haaren, Harry wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> > Sent: Wednesday, October 18, 2017 10:49 PM
> > To: Ivan Malov <Ivan.Malov@oktetlabs.ru>; Daly, Lee <lee.daly@intel.com>;
> > thomas@monjalon.net
> > Cc: dev@dpdk.org; Kozak, KubaX <kubax.kozak@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix xstats retrieve by id API
> > 
> > On 10/18/2017 4:10 AM, Ivan Malov wrote:
> > > On 10/12/2017 2:31 PM, Lee Daly wrote:
> > >> 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.
> > >>
> > >> Fixes: 79c913a42f0e ("ethdev: retrieve xstats by ID")
> > >> Cc: kubax.kozak@intel.com
> > >>
> > >> Signed-off-by: Lee Daly <lee.daly@intel.com>
> > >> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > >
> > > I have a serious concern regarding the patch. There is a common scenario
> > > of rte_eth_xstats_get_names_by_id() usage, and the patch breaks it.
> > > Typically, the function is called with 'ids = NULL' and 'xstats_names =
> > NULL'
> > > in order to get the number of figures. Then the function is called one
> > more
> > > time with appropriate storage for 'xstats_names'. According to the patch,
> > > on the former step get_xstats_count() is called to count the figures
> > available.
> > > The resulting number counts for PMD statistics + RTE_NB_STATS + some
> > amount of
> > > per-queue figures. However, on the latter step the following may take
> > place:
> > >
> > >> +	if (dev->dev_ops->xstats_get_names_by_id != NULL)
> > >> +		return (*dev->dev_ops->xstats_get_names_by_id)(
> > >> +				dev, xstats_names, ids, size);
> > >
> > > This obviously means that in the case when 'xstats_get_names_by_id' is
> > present,
> > > it will be called directly, and RTE statistics will not be filled in the
> > storage
> > > before the PMD figures. Accordingly, the value returned by the function in
> > this
> > > case will count for PMD figures only (i.e. will be less than the value
> > obtained
> > > on the first step). This is an obvious malfunction, and it would be
> > desirable
> > > to provide a fix for that. I hope for your understanding.
> > 
> > Hi Ivan,
> > 
> > You are right, this is broken.
> > 
> > But the problem is, ethdev API works with ids which are "basic + xstat" and
> > PMD dev_ops
> > works with ids which are "xstat". This mismatch ends up
> > xstats_get_names_by_id() dev_ops
> > being called to get all values, and filtering done in ethdev API, as done in
> > previous
> > implementation.
> > 
> > If we use xstats_get_names_by_id() to get all values, why we have it at all,
> > there is
> > already a dev_ops to get all values.
> > 
> > I believe we have two options,
> > 
> > 1- Remove this by _by_id() dev_ops, but keep ethdev APIs, and APIs does the
> > filtering of
> > the ids. This may be missing if any PMD has an optimal way of getting subset
> > of ids, this
> > is the main reason of the devops.
> 
> 
> Lets not remove the _by_id() opts, because that removes the possibility for PMDs to optimize the get_by_id() case.
> 
> 
> > 2- Convert ids for PMD usage, what do you think about following fix:
> > (same thing for rte_eth_xstats_get_by_id() also of course)
> 
> 
> A quick test of this patch allows proc-info to work again to dump xstats of a primary, eg testpmd. Run testpmd first and then proc-info:
> 
> testpmd: ./testpmd -- -i
> proc-info: ./dpdk-procinfo  -- --xstats
> 
> Before patch:
> Cannot get xstat names
> Cannot get xstat names
> 
> After patch:
> <correct xstats output>
> 
> I suggest we use Ferruh's fix, and apply ASAP unless @Ivan has a better solution/suggestion?
> 
> 
> 
> >   diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> >   index 0b1e92894..1145539ee 100644
> >   --- a/lib/librte_ether/rte_ethdev.c
> >   +++ b/lib/librte_ether/rte_ethdev.c
> >   @@ -1631,6 +1631,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
> >    {
> >           struct rte_eth_xstat_name *xstats_names_copy;
> >           unsigned int expected_entries;
> >   +       unsigned int all_ids_from_pmd = 1;
> >           struct rte_eth_dev *dev;
> >           unsigned int i;
> > 
> >   @@ -1649,9 +1650,28 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
> >           if (ids && !xstats_names)
> >                   return -EINVAL;
> > 
> >   -       if (dev->dev_ops->xstats_get_names_by_id != NULL)
> >   -               return (*dev->dev_ops->xstats_get_names_by_id)(
> >   -                               dev, xstats_names, ids, size);
> >   +       if (ids && dev->dev_ops->xstats_get_names_by_id != NULL && size >
> > 0) {
> >   +               uint64_t ids_copy[size];
> >   +               unsigned int pmd_count;
> >   +               unsigned int count;
> >   +
> >   +               pmd_count = (*dev->dev_ops->xstats_get_names_by_id)(dev,
> > NULL,
> >   +                                       NULL, 0);
> >   +               count = expected_entries - pmd_count;
> >   +
> >   +               for (i = 0; i < size; i++) {
> >   +                       if (ids[i] < count) {
> >   +                               all_ids_from_pmd = 0;
> >   +                               break;
> >   +                       }
> >   +
> >   +                       ids_copy[i] = ids[i] - count;
> >   +               }
> >   +
> >   +               if (all_ids_from_pmd)
> >   +                       return (*dev->dev_ops-
> > >xstats_get_names_by_id)(dev,
> >   +                                       xstats_names, ids_copy, size);
> >   +       }
> > 
> >           /* Retrieve all stats */
> >           if (!ids) {

The point is that I've been doing a custom check since the time when the "xstats by ID" API had been added and it have always been OK.
Now the custom check fails.

I'm afraid the workaround suggested by Ferruh is not quite comprehensive because it only
affects rte_eth_xstats_get_names_by_id(). At the same time rte_eth_xstats_get_by_id() also needs a fix since it contains
the same mistake:

> +	if (dev->dev_ops->xstats_get_by_id != NULL)
> +		return (*dev->dev_ops->xstats_get_by_id)(dev, ids, values,
> +			size);

Anyhow, the overall design might be slightly poor, I presume.

First of all, calling get_xstats_count() at the beginning of either function to obtain 'expected_entries' and then
calling (*dev->dev_ops->xstats_get_names_by_id) *just* to calculate 'count = expected_entries - pmd_count' (like in the hotfix)
is so bizarre because, if I recall correctly, the aim of the API is to optimise accessing arbitrary figures. It might be expensive to call
(*dev->dev_ops->xstats_get_names_by_id) multiple times since PMD's implementation may have an internal loop counting stats
supported by a particular adapter.

To put it together, why isn't it possible to move

>        count += RTE_NB_STATS;
>        count += RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS) *
>                 RTE_NB_RXQ_STATS;
>        count += RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS) *
>                 RTE_NB_TXQ_STATS;

from get_xstats_count() and make it an individual inline function to be used in such cases (like in the hotfix) to get the "count" for basic stats?

Secondly, the for() loop (both in the hotfix and in the very original version of the API) is dubious as well.
I mean, it would be better to make an agreement (restriction) in the API that all "basic" xstat IDs (if any) must be specified *first* in 'ids' array in ascending order.
Then it would be possible to use a for() loop which will fill in basic stats or stat_names (if any), then break on the first "non-basic" ID (i.e. 'ids[i] == count'),
and then (*dev->dev_ops->xstats_get_names_by_id) could be called with &ids[i], 'size - i', '&xstat_names[i]' arguments AND an additional (new) argument 'count' (or 'basic_count')
so that the PMD would be able to fill in the rest of the array. The new 'basic_count' argument would provide an idea of the first ID owned by PMD.

I admit that my suggestion might sound like a big controversy. So if nobody likes it, the proposed hotfix will be OK for me provided that
it covers rte_eth_xstats_get_by_id() as well. Right now the hotfix is incomplete.

-- 
Best regards,
Ivan

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

[Ferruh Yigit]

On 10/19/2017 8:03 AM, Ivan Malov wrote:
> On Thu, Oct 19, 2017 at 11:06:37AM +0000, Van Haaren, Harry wrote:
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
>>> Sent: Wednesday, October 18, 2017 10:49 PM
>>> To: Ivan Malov <Ivan.Malov@oktetlabs.ru>; Daly, Lee <lee.daly@intel.com>;
>>> thomas@monjalon.net
>>> Cc: dev@dpdk.org; Kozak, KubaX <kubax.kozak@intel.com>
>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix xstats retrieve by id API
>>>
>>> On 10/18/2017 4:10 AM, Ivan Malov wrote:
>>>> On 10/12/2017 2:31 PM, Lee Daly wrote:
>>>>> 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.
>>>>>
>>>>> Fixes: 79c913a42f0e ("ethdev: retrieve xstats by ID")
>>>>> Cc: kubax.kozak@intel.com
>>>>>
>>>>> Signed-off-by: Lee Daly <lee.daly@intel.com>
>>>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>
>>>> I have a serious concern regarding the patch. There is a common scenario
>>>> of rte_eth_xstats_get_names_by_id() usage, and the patch breaks it.
>>>> Typically, the function is called with 'ids = NULL' and 'xstats_names =
>>> NULL'
>>>> in order to get the number of figures. Then the function is called one
>>> more
>>>> time with appropriate storage for 'xstats_names'. According to the patch,
>>>> on the former step get_xstats_count() is called to count the figures
>>> available.
>>>> The resulting number counts for PMD statistics + RTE_NB_STATS + some
>>> amount of
>>>> per-queue figures. However, on the latter step the following may take
>>> place:
>>>>
>>>>> +	if (dev->dev_ops->xstats_get_names_by_id != NULL)
>>>>> +		return (*dev->dev_ops->xstats_get_names_by_id)(
>>>>> +				dev, xstats_names, ids, size);
>>>>
>>>> This obviously means that in the case when 'xstats_get_names_by_id' is
>>> present,
>>>> it will be called directly, and RTE statistics will not be filled in the
>>> storage
>>>> before the PMD figures. Accordingly, the value returned by the function in
>>> this
>>>> case will count for PMD figures only (i.e. will be less than the value
>>> obtained
>>>> on the first step). This is an obvious malfunction, and it would be
>>> desirable
>>>> to provide a fix for that. I hope for your understanding.
>>>
>>> Hi Ivan,
>>>
>>> You are right, this is broken.
>>>
>>> But the problem is, ethdev API works with ids which are "basic + xstat" and
>>> PMD dev_ops
>>> works with ids which are "xstat". This mismatch ends up
>>> xstats_get_names_by_id() dev_ops
>>> being called to get all values, and filtering done in ethdev API, as done in
>>> previous
>>> implementation.
>>>
>>> If we use xstats_get_names_by_id() to get all values, why we have it at all,
>>> there is
>>> already a dev_ops to get all values.
>>>
>>> I believe we have two options,
>>>
>>> 1- Remove this by _by_id() dev_ops, but keep ethdev APIs, and APIs does the
>>> filtering of
>>> the ids. This may be missing if any PMD has an optimal way of getting subset
>>> of ids, this
>>> is the main reason of the devops.
>>
>>
>> Lets not remove the _by_id() opts, because that removes the possibility for PMDs to optimize the get_by_id() case.
>>
>>
>>> 2- Convert ids for PMD usage, what do you think about following fix:
>>> (same thing for rte_eth_xstats_get_by_id() also of course)
>>
>>
>> A quick test of this patch allows proc-info to work again to dump xstats of a primary, eg testpmd. Run testpmd first and then proc-info:
>>
>> testpmd: ./testpmd -- -i
>> proc-info: ./dpdk-procinfo  -- --xstats
>>
>> Before patch:
>> Cannot get xstat names
>> Cannot get xstat names
>>
>> After patch:
>> <correct xstats output>
>>
>> I suggest we use Ferruh's fix, and apply ASAP unless @Ivan has a better solution/suggestion?
>>
>>
>>
>>>   diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>>>   index 0b1e92894..1145539ee 100644
>>>   --- a/lib/librte_ether/rte_ethdev.c
>>>   +++ b/lib/librte_ether/rte_ethdev.c
>>>   @@ -1631,6 +1631,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>>    {
>>>           struct rte_eth_xstat_name *xstats_names_copy;
>>>           unsigned int expected_entries;
>>>   +       unsigned int all_ids_from_pmd = 1;
>>>           struct rte_eth_dev *dev;
>>>           unsigned int i;
>>>
>>>   @@ -1649,9 +1650,28 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>>           if (ids && !xstats_names)
>>>                   return -EINVAL;
>>>
>>>   -       if (dev->dev_ops->xstats_get_names_by_id != NULL)
>>>   -               return (*dev->dev_ops->xstats_get_names_by_id)(
>>>   -                               dev, xstats_names, ids, size);
>>>   +       if (ids && dev->dev_ops->xstats_get_names_by_id != NULL && size >
>>> 0) {
>>>   +               uint64_t ids_copy[size];
>>>   +               unsigned int pmd_count;
>>>   +               unsigned int count;
>>>   +
>>>   +               pmd_count = (*dev->dev_ops->xstats_get_names_by_id)(dev,
>>> NULL,
>>>   +                                       NULL, 0);
>>>   +               count = expected_entries - pmd_count;
>>>   +
>>>   +               for (i = 0; i < size; i++) {
>>>   +                       if (ids[i] < count) {
>>>   +                               all_ids_from_pmd = 0;
>>>   +                               break;
>>>   +                       }
>>>   +
>>>   +                       ids_copy[i] = ids[i] - count;
>>>   +               }
>>>   +
>>>   +               if (all_ids_from_pmd)
>>>   +                       return (*dev->dev_ops-
>>>> xstats_get_names_by_id)(dev,
>>>   +                                       xstats_names, ids_copy, size);
>>>   +       }
>>>
>>>           /* Retrieve all stats */
>>>           if (!ids) {
> 
> The point is that I've been doing a custom check since the time when the "xstats by ID" API had been added and it have always been OK.
> Now the custom check fails.
> 
> I'm afraid the workaround suggested by Ferruh is not quite comprehensive because it only
> affects rte_eth_xstats_get_names_by_id(). At the same time rte_eth_xstats_get_by_id() also needs a fix since it contains
> the same mistake:

I already put a note into email to say rte_eth_xstats_get_by_id() also needs to
be updated, the diff was just to give the idea. So that part is OK.

> 
>> +	if (dev->dev_ops->xstats_get_by_id != NULL)
>> +		return (*dev->dev_ops->xstats_get_by_id)(dev, ids, values,
>> +			size);
> 
> Anyhow, the overall design might be slightly poor, I presume.

This is to let _by_id() dev_ops to be used to get partial data, which they are
introduced for, any better idea is welcome :)

> 
> First of all, calling get_xstats_count() at the beginning of either function to obtain 'expected_entries' and then
> calling (*dev->dev_ops->xstats_get_names_by_id) *just* to calculate 'count = expected_entries - pmd_count' (like in the hotfix)
> is so bizarre because, if I recall correctly, the aim of the API is to optimise accessing arbitrary figures. It might be expensive to call
> (*dev->dev_ops->xstats_get_names_by_id) multiple times since PMD's implementation may have an internal loop counting stats
> supported by a particular adapter.
> 
> To put it together, why isn't it possible to move
> 
>>        count += RTE_NB_STATS;
>>        count += RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS) *
>>                 RTE_NB_RXQ_STATS;
>>        count += RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS) *
>>                 RTE_NB_TXQ_STATS;
> 
> from get_xstats_count() and make it an individual inline function to be used in such cases (like in the hotfix) to get the "count" for basic stats?

That was something I thought, but wanted to make _by_id() function simple.
But converting above into inline function and use make sense.

> 
> Secondly, the for() loop (both in the hotfix and in the very original version of the API) is dubious as well.
> I mean, it would be better to make an agreement (restriction) in the API that all "basic" xstat IDs (if any) must be specified *first* in 'ids' array in ascending order.
> Then it would be possible to use a for() loop which will fill in basic stats or stat_names (if any), then break on the first "non-basic" ID (i.e. 'ids[i] == count'),
> and then (*dev->dev_ops->xstats_get_names_by_id) could be called with &ids[i], 'size - i', '&xstat_names[i]' arguments AND an additional (new) argument 'count' (or 'basic_count')
> so that the PMD would be able to fill in the rest of the array. The new 'basic_count' argument would provide an idea of the first ID owned by PMD.

There is no agreement in the API to ask ids should be ordered, and at this point
we can't add this, this will be API break.

And I was about mention about same thing, as far as I can see sfc has this
assumption that ids will be sorted, just that is not guaranteed. Since these
APIs were not really functioning well and always used to get all data, you may
not recognized any problem yet.

> 
> I admit that my suggestion might sound like a big controversy. So if nobody likes it, the proposed hotfix will be OK for me provided that
> it covers rte_eth_xstats_get_by_id() as well. Right now the hotfix is incomplete.

I will send a proper patch.

Thanks,
ferruh