[dpdk-dev] [PATCH] app/testpmd: add sanity checks when retrieving xstats

[dpdk-dev] [PATCH] app/testpmd: add sanity checks when retrieving xstats

[David Marchand]

Testpmd should not expect the xstats names and values arrays to be
aligned: neither the arrays sizes, nor the order in which the values are.

This hid some bugs where pmds would either return wrong values count or
invalid statistics indexes.

Link: http://dpdk.org/browse/dpdk/commit/?id=5fd4d049692b2fde8bf49c7461b18180a8fd2545
Link: http://dpdk.org/dev/patchwork/patch/40705/

Signed-off-by: David Marchand <david.marchand@6wind.com>
---

@stable: when this goes in, I recommend backporting this to all existing
branches, as it makes it easier to show this kind of pmds bugs.

---
 app/test-pmd/config.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 97020fb..8edb80c 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -210,9 +210,11 @@ nic_stats_clear(portid_t port_id)
 void
 nic_xstats_display(portid_t port_id)
 {
-	struct rte_eth_xstat *xstats;
-	int cnt_xstats, idx_xstat;
 	struct rte_eth_xstat_name *xstats_names;
+	struct rte_eth_xstat *xstats;
+	int cnt_xnames;
+	int cnt_xstats;
+	int idx_xstat;
 
 	printf("###### NIC extended statistics for port %-2d\n", port_id);
 	if (!rte_eth_dev_is_valid_port(port_id)) {
@@ -221,33 +223,34 @@ nic_xstats_display(portid_t port_id)
 	}
 
 	/* Get count */
-	cnt_xstats = rte_eth_xstats_get_names(port_id, NULL, 0);
-	if (cnt_xstats  < 0) {
+	cnt_xnames = rte_eth_xstats_get_names(port_id, NULL, 0);
+	if (cnt_xnames  < 0) {
 		printf("Error: Cannot get count of xstats\n");
 		return;
 	}
 
 	/* Get id-name lookup table */
-	xstats_names = malloc(sizeof(struct rte_eth_xstat_name) * cnt_xstats);
+	xstats_names = malloc(sizeof(struct rte_eth_xstat_name) * cnt_xnames);
 	if (xstats_names == NULL) {
 		printf("Cannot allocate memory for xstats lookup\n");
 		return;
 	}
-	if (cnt_xstats != rte_eth_xstats_get_names(
-			port_id, xstats_names, cnt_xstats)) {
+	if (cnt_xnames != rte_eth_xstats_get_names(
+			port_id, xstats_names, cnt_xnames)) {
 		printf("Error: Cannot get xstats lookup\n");
 		free(xstats_names);
 		return;
 	}
 
 	/* Get stats themselves */
-	xstats = malloc(sizeof(struct rte_eth_xstat) * cnt_xstats);
+	xstats = malloc(sizeof(struct rte_eth_xstat) * cnt_xnames);
 	if (xstats == NULL) {
 		printf("Cannot allocate memory for xstats\n");
 		free(xstats_names);
 		return;
 	}
-	if (cnt_xstats != rte_eth_xstats_get(port_id, xstats, cnt_xstats)) {
+	cnt_xstats = rte_eth_xstats_get(port_id, xstats, cnt_xnames);
+	if (cnt_xstats > cnt_xnames) {
 		printf("Error: Unable to get xstats\n");
 		free(xstats_names);
 		free(xstats);
@@ -256,10 +259,15 @@ nic_xstats_display(portid_t port_id)
 
 	/* Display xstats */
 	for (idx_xstat = 0; idx_xstat < cnt_xstats; idx_xstat++) {
+		if (xstats[idx_xstat].id > (uint64_t)cnt_xnames) {
+			printf("Error: Invalid statistic index: %"PRId64
+			       ", max %d\n", xstats[idx_xstat].id, cnt_xnames);
+			continue;
+		}
 		if (xstats_hide_zero && !xstats[idx_xstat].value)
 			continue;
 		printf("%s: %"PRIu64"\n",
-			xstats_names[idx_xstat].name,
+			xstats_names[xstats[idx_xstat].id].name,
 			xstats[idx_xstat].value);
 	}
 	free(xstats_names);
-- 
2.7.4

Re: [dpdk-dev] [PATCH] app/testpmd: add sanity checks when retrieving xstats

[David Marchand]

Hey guys,

On Thu, Jun 7, 2018 at 10:15 AM, David Marchand
<david.marchand@6wind.com> wrote:
> Testpmd should not expect the xstats names and values arrays to be
> aligned: neither the arrays sizes, nor the order in which the values are.
>
> This hid some bugs where pmds would either return wrong values count or
> invalid statistics indexes.
>
> Link: http://dpdk.org/browse/dpdk/commit/?id=5fd4d049692b2fde8bf49c7461b18180a8fd2545
> Link: http://dpdk.org/dev/patchwork/patch/40705/
>
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---
>
> @stable: when this goes in, I recommend backporting this to all existing
> branches, as it makes it easier to show this kind of pmds bugs.

Can someone have a look please ?
Thanks.


-- 
David Marchand

Re: [dpdk-dev] [PATCH] app/testpmd: add sanity checks when retrieving xstats

[Remy Horton]

Was out of office, so only saw the patchset this morning.

Missing fixlines (watch out for subject line wrap):

Fixes: e2aae1c1ced9 ("ethdev: remove name from extended statistic fetch")
Fixes: 0a5beecf466a ("ethdev: revert xstats by ID")

Otherwise looks good to me. A case of implementation simplification 
turning into API assumption :(

..Remy


On 07/06/2018 09:15, David Marchand wrote:
[..]
> Signed-off-by: David Marchand <david.marchand@6wind.com>

Acked-by: Remy Horton <remy.horton@intel.com>

Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: add sanity checks when retrieving xstats

[Ferruh Yigit]

On 6/7/2018 9:15 AM, David Marchand wrote:
> Testpmd should not expect the xstats names and values arrays to be
> aligned: neither the arrays sizes, nor the order in which the values are.

As far as I can see this assumption is everywhere in API implementation:
xstats names and values are aligned with same order.
The basic stat part of the xstats, implemented in ethdev layer, seems relying on
same assumption. Also looks like "xstat size" and "xstat_names size" used
interchangeably.

And I don't see any case that mentions xstats.id is xstats_name index.
cc'ed Harry, to get more information about initial intention.

the id value in xstats struct looks like duplication, but other than that, is
there any downside of using array index to mach name, value pair?
And do we really need another layer of indirection (and complexity) to mach
simple name,value key pair in xstats?

> 
> This hid some bugs where pmds would either return wrong values count or
> invalid statistics indexes.
> 
> Link: http://dpdk.org/browse/dpdk/commit/?id=5fd4d049692b2fde8bf49c7461b18180a8fd2545
> Link: http://dpdk.org/dev/patchwork/patch/40705/
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---
> 
> @stable: when this goes in, I recommend backporting this to all existing
> branches, as it makes it easier to show this kind of pmds bugs.

<...>

Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: add sanity checks when retrieving xstats

[Remy Horton]

On 13/06/2018 16:39, Ferruh Yigit wrote:
> On 6/7/2018 9:15 AM, David Marchand wrote:
>> Testpmd should not expect the xstats names and values arrays to be
>> aligned: neither the arrays sizes, nor the order in which the values are.
>
> As far as I can see this assumption is everywhere in API implementation:
> xstats names and values are aligned with same order.
> The basic stat part of the xstats, implemented in ethdev layer, seems relying on
> same assumption. Also looks like "xstat size" and "xstat_names size" used
> interchangeably.
>
> And I don't see any case that mentions xstats.id is xstats_name index.
> cc'ed Harry, to get more information about initial intention.
>
> the id value in xstats struct looks like duplication, but other than that, is
> there any downside of using array index to mach name, value pair?
> And do we really need another layer of indirection (and complexity) to mach
> simple name,value key pair in xstats?

When I was working on xstats one of my intentions was to allow PMDs to 
only return a subset of values for all the keys they declare, with 
xstats[idx].id==idx just being a coincidence that was not to be relied 
on. Since then there appears to have been several instances of rework, 
so no idea if this coincidence becoming an assumption was intentional or 
an oversight.

..Remy

Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: add sanity checks when retrieving xstats

[Ferruh Yigit]

On 6/14/2018 7:39 AM, Remy Horton wrote:
> 
> On 13/06/2018 16:39, Ferruh Yigit wrote:
>> On 6/7/2018 9:15 AM, David Marchand wrote:
>>> Testpmd should not expect the xstats names and values arrays to be
>>> aligned: neither the arrays sizes, nor the order in which the values are.
>>
>> As far as I can see this assumption is everywhere in API implementation:
>> xstats names and values are aligned with same order.
>> The basic stat part of the xstats, implemented in ethdev layer, seems relying on
>> same assumption. Also looks like "xstat size" and "xstat_names size" used
>> interchangeably.
>>
>> And I don't see any case that mentions xstats.id is xstats_name index.
>> cc'ed Harry, to get more information about initial intention.
>>
>> the id value in xstats struct looks like duplication, but other than that, is
>> there any downside of using array index to mach name, value pair?
>> And do we really need another layer of indirection (and complexity) to mach
>> simple name,value key pair in xstats?
> 
> When I was working on xstats one of my intentions was to allow PMDs to 
> only return a subset of values for all the keys they declare, with 
> xstats[idx].id==idx just being a coincidence that was not to be relied 
> on.

APIs exist for getting subset of values (.._by_id) but they both assume
requested ids are array index.
As you said this works fine because of xstats[idx].id==idx

struct rte_eth_xstat_name { char name[]; }
struct rte_eth_xstat { uint64_t id; uint64_t value; }

These two structs are for basic key-value match.
But one has the "id" field, but other doesn't. If we use "id" as match, this
will be the index of xstat_name[]. This is extra complexity, and xstats is
already unnecessarily complex.

I am for documenting that "xstat_name" and "xstat" are aligned, both in size and
order, and array indexes are ids, clearly in API doc and continue with existing
implementation. What do you think?

> Since then there appears to have been several instances of rework, 
> so no idea if this coincidence becoming an assumption was intentional or 
> an oversight.
> 
> ..Remy
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: add sanity checks when retrieving xstats

[Remy Horton]

On 14/06/2018 11:55, Ferruh Yigit wrote:
> On 6/14/2018 7:39 AM, Remy Horton wrote:
>>
>> On 13/06/2018 16:39, Ferruh Yigit wrote:
>>> On 6/7/2018 9:15 AM, David Marchand wrote:
>>>> Testpmd should not expect the xstats names and values arrays to be
>>>> aligned: neither the arrays sizes, nor the order in which the values are.
[..]

> APIs exist for getting subset of values (.._by_id) but they both assume
> requested ids are array index.
> As you said this works fine because of xstats[idx].id==idx

Changing that coincidence into an assumption looks like a bug to me.


> struct rte_eth_xstat_name { char name[]; }
> struct rte_eth_xstat { uint64_t id; uint64_t value; }
>
> These two structs are for basic key-value match.
> But one has the "id" field, but other doesn't. If we use "id" as match, this
> will be the index of xstat_name[]. This is extra complexity, and xstats is
> already unnecessarily complex.
>
> I am for documenting that "xstat_name" and "xstat" are aligned, both in size and
> order, and array indexes are ids, clearly in API doc and continue with existing
> implementation. What do you think?

As long as none of the PMD vendors intend to take advantage of 
xstats[idx].id!=idx (e.g. to allow omitted values) then I'm OK with it.


..Remy

[dpdk-dev] [PATCH] ethdev: document xstats API assumption

[Ferruh Yigit]

Documents the assumption that 'xstats[i].id == i' and
key=xstats_names[i].name, value=xstats[i].value

xstats[i].id is still used for xstats _by_id() APIs.

This patch reverts some part of the commit 6d52d1d4afb4 ("ethdev:
clarify extended statistics documentation")

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: David Marchand <david.marchand@6wind.com>
Cc: Olivier Matz <olivier.matz@6wind.com>
Cc: Remy Horton <remy.horton@intel.com>
---
 lib/librte_ethdev/rte_ethdev.h | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 5760f45d3..261dabafb 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -2008,6 +2008,15 @@ int rte_eth_stats_reset(uint16_t port_id);
 /**
  * Retrieve names of extended statistics of an Ethernet device.
  *
+ * There is an assumption that 'xstat_names' and 'xstats' arrays are matched
+ * by array index:
+ *  xstats_names[i].name => xstats[i].value
+ *
+ * And the array index is same with id field of 'struct rte_eth_xstat':
+ *  xstats[i].id == i
+ *
+ * This assumption makes key-value pair matching less flexible but simpler.
+ *
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param xstats_names
@@ -2032,13 +2041,20 @@ int rte_eth_xstats_get_names(uint16_t port_id,
 /**
  * Retrieve extended statistics of an Ethernet device.
  *
+ * There is an assumption that 'xstat_names' and 'xstats' arrays are matched
+ * by array index:
+ *  xstats_names[i].name => xstats[i].value
+ *
+ * And the array index is same with id field of 'struct rte_eth_xstat':
+ *  xstats[i].id == i
+ *
+ * This assumption makes key-value pair matching less flexible but simpler.
+ *
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param xstats
  *   A pointer to a table of structure of type *rte_eth_xstat*
- *   to be filled with device statistics ids and values: id is the
- *   index of the name string in xstats_names (see rte_eth_xstats_get_names()),
- *   and value is the statistic counter.
+ *   to be filled with device statistics ids and values.
  *   This parameter can be set to NULL if n is 0.
  * @param n
  *   The size of the xstats array (number of elements).
-- 
2.17.1

Re: [dpdk-dev] [PATCH] ethdev: document xstats API assumption

[David Marchand]

On Thu, Jun 28, 2018 at 4:16 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> Documents the assumption that 'xstats[i].id == i' and
> key=xstats_names[i].name, value=xstats[i].value
>
> xstats[i].id is still used for xstats _by_id() APIs.
>
> This patch reverts some part of the commit 6d52d1d4afb4 ("ethdev:
> clarify extended statistics documentation")
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Reviewed-by: David Marchand <david.marchand@6wind.com>

So my testpmd patch can be dropped.


-- 
David Marchand

Re: [dpdk-dev] [PATCH] ethdev: document xstats API assumption

[Ferruh Yigit]

On 6/29/2018 8:15 AM, David Marchand wrote:
> On Thu, Jun 28, 2018 at 4:16 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>> Documents the assumption that 'xstats[i].id == i' and
>> key=xstats_names[i].name, value=xstats[i].value
>>
>> xstats[i].id is still used for xstats _by_id() APIs.
>>
>> This patch reverts some part of the commit 6d52d1d4afb4 ("ethdev:
>> clarify extended statistics documentation")
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Reviewed-by: David Marchand <david.marchand@6wind.com>

Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

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

Re: [dpdk-dev] [PATCH] ethdev: document xstats API assumption

[Van Haaren, Harry]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Thursday, June 28, 2018 3:16 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; David Marchand
> <david.marchand@6wind.com>; Olivier Matz <olivier.matz@6wind.com>; Horton,
> Remy <remy.horton@intel.com>
> Subject: [dpdk-dev] [PATCH] ethdev: document xstats API assumption
> 
> Documents the assumption that 'xstats[i].id == i' and
> key=xstats_names[i].name, value=xstats[i].value
> 
> xstats[i].id is still used for xstats _by_id() APIs.
> 
> This patch reverts some part of the commit 6d52d1d4afb4 ("ethdev:
> clarify extended statistics documentation")
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

I'm also in favor of simplifying xstats APIs if possible, given we have a number of PMDs using it, and they all adhere to this simplification;

Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Re: [dpdk-dev] [PATCH] ethdev: document xstats API assumption

[Andrew Rybchenko]

On 28.06.2018 17:16, Ferruh Yigit wrote:
> Documents the assumption that 'xstats[i].id == i' and
> key=xstats_names[i].name, value=xstats[i].value
>
> xstats[i].id is still used for xstats _by_id() APIs.
>
> This patch reverts some part of the commit 6d52d1d4afb4 ("ethdev:
> clarify extended statistics documentation")
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>