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

[dpdk-stable] [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-stable] [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-stable] [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-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-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-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-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