[dpdk-dev] [PATCH] ethdev: expose link status and speed using xstats

[dpdk-dev] [PATCH] ethdev: expose link status and speed using xstats

[Harry van Haaren]

This patch exposes link duplex, speed, and status via the
existing xstats API.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 doc/guides/rel_notes/release_2_3.rst |  1 +
 lib/librte_ether/rte_ethdev.c        | 29 ++++++++++++++++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/doc/guides/rel_notes/release_2_3.rst b/doc/guides/rel_notes/release_2_3.rst
index 99de186..c3449dc 100644
--- a/doc/guides/rel_notes/release_2_3.rst
+++ b/doc/guides/rel_notes/release_2_3.rst
@@ -19,6 +19,7 @@ Drivers
 Libraries
 ~~~~~~~~~
 
+* **Link Status added to extended statistics in ethdev**
 
 Examples
 ~~~~~~~~
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index ed971b4..3c35e1b 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -83,6 +83,15 @@ struct rte_eth_xstats_name_off {
 	unsigned offset;
 };
 
+/* Link Status display in xstats */
+static const char * const rte_eth_duplex_strings[] = {
+	"link_duplex_autonegotiate",
+	"link_duplex_half",
+	"link_duplex_full"
+};
+
+#define RTE_NB_LINK_STATUS_STATS 3
+
 static const struct rte_eth_xstats_name_off rte_stats_strings[] = {
 	{"rx_good_packets", offsetof(struct rte_eth_stats, ipackets)},
 	{"tx_good_packets", offsetof(struct rte_eth_stats, opackets)},
@@ -94,7 +103,10 @@ static const struct rte_eth_xstats_name_off rte_stats_strings[] = {
 		rx_nombuf)},
 };
 
-#define RTE_NB_STATS (sizeof(rte_stats_strings) / sizeof(rte_stats_strings[0]))
+#define RTE_GENERIC_STATS (sizeof(rte_stats_strings) / \
+		sizeof(rte_stats_strings[0]))
+
+#define RTE_NB_STATS (RTE_NB_LINK_STATUS_STATS + RTE_GENERIC_STATS)
 
 static const struct rte_eth_xstats_name_off rte_rxq_stats_strings[] = {
 	{"packets", offsetof(struct rte_eth_stats, q_ipackets)},
@@ -1466,6 +1478,7 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 {
 	struct rte_eth_stats eth_stats;
 	struct rte_eth_dev *dev;
+	struct rte_eth_link link;
 	unsigned count = 0, i, q;
 	signed xcount = 0;
 	uint64_t val, *stats_ptr;
@@ -1497,8 +1510,18 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 	count = 0;
 	rte_eth_stats_get(port_id, &eth_stats);
 
+	/* link status */
+	rte_eth_link_get_nowait(port_id, &link);
+	snprintf(xstats[count].name, sizeof(xstats[count].name), "link_status");
+	xstats[count++].value = link.link_status;
+	snprintf(xstats[count].name, sizeof(xstats[count].name), "link_speed");
+	xstats[count++].value = link.link_speed;
+	snprintf(xstats[count].name, sizeof(xstats[count].name),
+		 "%s", rte_eth_duplex_strings[link.link_duplex]);
+	xstats[count++].value = 1;
+
 	/* global stats */
-	for (i = 0; i < RTE_NB_STATS; i++) {
+	for (i = 0; i < RTE_GENERIC_STATS; i++) {
 		stats_ptr = RTE_PTR_ADD(&eth_stats,
 					rte_stats_strings[i].offset);
 		val = *stats_ptr;
-- 
2.5.0

Re: [dpdk-dev] [PATCH] ethdev: expose link status and speed using xstats

[Kyle Larose]

On Wed, Jan 20, 2016 at 9:28 AM, Harry van Haaren
<harry.van.haaren@intel.com> wrote:
> This patch exposes link duplex, speed, and status via the
> existing xstats API.
>

I'm slightly confused by this. Why are we exposing operational
properties of the chip through an API which I thought was primarily
targeting statistics? When I think of statistics and a NIC, I think of
values which are monotonically increasing. I think of values that are
derived primary from the packets flowing through the system. I do not
think of link state, link speed and duplex, which have nothing to do
with packets, and are not monotonic.

Should we not have a separate API to get this type information? I
mean, just because we have a generic "string to uint64_t" map doesn't
mean we should toss in anything that can fit into a uin64_t. Would you
want to see the MAC address in here as well? If we put in link
speed/etc. it seems like we may as well!

Thanks,

Kyle

Re: [dpdk-dev] [PATCH] ethdev: expose link status and speed using xstats

[Van Haaren, Harry]

Hi Kyle,

> From: Kyle Larose [mailto:eomereadig@gmail.com]
> On Wed, Jan 20, 2016 at 9:28 AM, Harry van Haaren
> <harry.van.haaren@intel.com> wrote:
> > This patch exposes link duplex, speed, and status via the
> > existing xstats API.
> 
> I'm slightly confused by this. Why are we exposing operational
> properties of the chip through an API which I thought was primarily
> targeting statistics?


In a fault-detection situation, link state is a good item to monitor - just like
the rest of the statistics on the NIC.


> When I think of statistics and a NIC, I think of
> values which are monotonically increasing. I think of values that are
> derived primary from the packets flowing through the system. I do not
> think of link state, link speed and duplex, which have nothing to do
> with packets, and are not monotonic.


Link state, and speed seem a good fit to me. I'll admit I'm not sure about duplex, and would be happy to respin the patch without duplex if the community would prefer that.


> Should we not have a separate API to get this type information? I
> mean, just because we have a generic "string to uint64_t" map doesn't
> mean we should toss in anything that can fit into a uin64_t.


In theory we could create a new API for this, but I think the current xstats API is a good fit for exposing this info, so why create extra APIs? As a client of the DPDK API, I would prefer more statistics in a single API than have to research and implement two or more APIs to retrieve the information to monitor.

I'm working on exposing keep-alive statistics using an xstats style API, I'll the patches later today so we can discuss them too.


Regards, -Harry

Re: [dpdk-dev] [PATCH] ethdev: expose link status and speed using xstats

[Kyle Larose]

Hi Harry,

On Wed, Jan 20, 2016 at 9:45 AM, Van Haaren, Harry
<harry.van.haaren@intel.com> wrote:
> Hi Kyle,

>
> In theory we could create a new API for this, but I think the current xstats API is a good fit for exposing this info, so why create extra APIs? As a client of the DPDK API, I would prefer more statistics in a single API than have to research and implement two or more APIs to retrieve the information to monitor.
>

You create new APIs for many reasons: modularity, simplicitly within
the API, consistency, etc. My main concern with this proposed change
relates to consistency. Previously, each stat had similar semantics.
It was a number, representing the amount of times something had
occurred on a chip. This fact allows you to perform operations like
addition, subtraction/etc and expect that the result will be
meaningful for every value in the array.

For example, suppose I wrote a tool to give the "rate" for each of the
stats. We could sample these stats periodically, then output the
difference between the two samples divided by the time between samples
for each stat. A naive implementation, but quite simple.

However, if we start adding values like link speed and state, which
are not really numerical, or not monotonic, you can no longer apply
the same mathematical operations on them and expect them to be
meaningful. For example, suppose a link went down. The "rate" for that
stat would be -1. Does that really make sense? Anyone using this API
would need to explicitly filter out the non-stats, or risk nonsensical
output.

Let's also consider how to interpret the value. When I look at a stat,
there's usually one of two meanings: it's either a number of packets,
or it's a number of bytes. We're now adding exceptions to that rule.
Link state is a boolean. Link speed is a value in mbps. Duplex is
pretty much an enum.

We already have the rte_eth_link_get function. Why not let users
continue to use that? It's well defined, it is simple, and it is
consistent.

Thanks,

Kyle

Re: [dpdk-dev] [PATCH] ethdev: expose link status and speed using xstats

[Thomas Monjalon]

2016-01-20 10:03, Kyle Larose:
> Hi Harry,
> 
> On Wed, Jan 20, 2016 at 9:45 AM, Van Haaren, Harry
> <harry.van.haaren@intel.com> wrote:
> > Hi Kyle,
> 
> >
> > In theory we could create a new API for this, but I think the current xstats API is a good fit for exposing this info, so why create extra APIs? As a client of the DPDK API, I would prefer more statistics in a single API than have to research and implement two or more APIs to retrieve the information to monitor.
> >
> 
> You create new APIs for many reasons: modularity, simplicitly within
> the API, consistency, etc. My main concern with this proposed change
> relates to consistency. Previously, each stat had similar semantics.
> It was a number, representing the amount of times something had
> occurred on a chip. This fact allows you to perform operations like
> addition, subtraction/etc and expect that the result will be
> meaningful for every value in the array.
> 
> For example, suppose I wrote a tool to give the "rate" for each of the
> stats. We could sample these stats periodically, then output the
> difference between the two samples divided by the time between samples
> for each stat. A naive implementation, but quite simple.
> 
> However, if we start adding values like link speed and state, which
> are not really numerical, or not monotonic, you can no longer apply
> the same mathematical operations on them and expect them to be
> meaningful. For example, suppose a link went down. The "rate" for that
> stat would be -1. Does that really make sense? Anyone using this API
> would need to explicitly filter out the non-stats, or risk nonsensical
> output.
> 
> Let's also consider how to interpret the value. When I look at a stat,
> there's usually one of two meanings: it's either a number of packets,
> or it's a number of bytes. We're now adding exceptions to that rule.
> Link state is a boolean. Link speed is a value in mbps. Duplex is
> pretty much an enum.
> 
> We already have the rte_eth_link_get function. Why not let users
> continue to use that? It's well defined, it is simple, and it is
> consistent.

+1

Please also consider this work in progress
about link speed information:
http://dpdk.org/dev/patchwork/patch/7995/

Re: [dpdk-dev] [PATCH] ethdev: expose link status and speed using xstats

[Van Haaren, Harry]

> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> 2016-01-20 10:03, Kyle Larose:
> > We already have the rte_eth_link_get function. Why not let users
> > continue to use that? It's well defined, it is simple, and it is
> > consistent.
> 
> +1


Ok, no problem. I'll mark the link-status patch rejected in Patchwork.


I've just sent the Keepalive patchset, patch #3 is of interest regarding this discussion:
http://dpdk.org/dev/patchwork/patch/10003/

It adds a function to the API for collecting xstats, meaning it doesn't pollute the rte_eth_xstats_get() output. I'm interested to hear the communities view of this approach.


Regards, -Harry

Re: [dpdk-dev] [PATCH] ethdev: expose link status and speed using xstats

[Stephen Hemminger]

On Wed, 20 Jan 2016 16:13:34 +0100
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> > We already have the rte_eth_link_get function. Why not let users
> > continue to use that? It's well defined, it is simple, and it is
> > consistent.  

+1

API's should not duplicate results (DRY)

That said, it would be useful to have some way to get statistics
on the number of link transitions and time since last change.
But this ideally should be in rte_eth_link_get() but that wouldn't
be ABI compatiable.