[dpdk-dev] [PATCH] examples/ip_pipeline: add link show to the cli

[dpdk-dev] [PATCH] examples/ip_pipeline: add link show to the cli

[Kevin Laatz]

Add the functionality to track links in the application. This enables the
user to print the name, mac address and per-port statistics for each link
in the application.

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 examples/ip_pipeline/cli.c  | 103 ++++++++++++++++++++++++++++++++++++++++++++
 examples/ip_pipeline/link.c |   6 +++
 examples/ip_pipeline/link.h |   3 ++
 3 files changed, 112 insertions(+)

diff --git a/examples/ip_pipeline/cli.c b/examples/ip_pipeline/cli.c
index c9587f5..b7fba3b 100644
--- a/examples/ip_pipeline/cli.c
+++ b/examples/ip_pipeline/cli.c
@@ -9,6 +9,7 @@
 
 #include <rte_common.h>
 #include <rte_cycles.h>
+#include <rte_ethdev.h>
 
 #include "cli.h"
 #include "kni.h"
@@ -4355,6 +4356,103 @@ cmd_thread_pipeline_disable(char **tokens,
 	}
 }
 
+/* Print the link stats and info */
+static void
+print_link_info(struct link *link, char *out, size_t out_size)
+{
+	struct rte_eth_stats stats;
+	struct ether_addr mac_addr;
+	struct rte_eth_link eth_link;
+
+	memset(&stats, 0, sizeof(stats));
+	rte_eth_stats_get(link->port_id, &stats);
+
+	rte_eth_macaddr_get(link->port_id, &mac_addr);
+	rte_eth_link_get(link->port_id, &eth_link);
+
+	snprintf(out, out_size,
+		"\n"
+		"Link name: %s\n"
+		"\tPort %u MAC: %02"PRIx8":%02"PRIx8":%02"PRIx8
+		":%02"PRIx8":%02"PRIx8":%02"PRIx8"\n"
+		"\tLink Status: %s\n"
+		"\tLink Speed: %u\n"
+		"\n"
+		"\tRX info:\n"
+		"\t\tQueues: %u\n"
+		"\t\tPackets: %" PRIu64
+		"\t\tBytes: %" PRIu64"\n"
+		"\t\tErrors: %" PRIu64
+		"\t\tMissed: %" PRIu64
+		"\t\tNo-mbuf: %" PRIu64"\n"
+		"\tTX info:\n"
+		"\t\tQueues: %u\n"
+		"\t\tPackets: %" PRIu64
+		"\t\tBytes: %" PRIu64"\n"
+		"\t\tErrors: %" PRIu64"\n",
+		link->name,
+		link->port_id,
+		mac_addr.addr_bytes[0], mac_addr.addr_bytes[1],
+		mac_addr.addr_bytes[2], mac_addr.addr_bytes[3],
+		mac_addr.addr_bytes[4], mac_addr.addr_bytes[5],
+		eth_link.link_status == 1 ? "UP" : "DOWN",
+		eth_link.link_speed,
+		link->n_rxq,
+		stats.ipackets,
+		stats.ibytes,
+		stats.ierrors,
+		stats.imissed,
+		stats.rx_nombuf,
+		link->n_txq,
+		stats.opackets,
+		stats.obytes,
+		stats.oerrors);
+}
+
+/*
+ * link show [<link_name>]
+ */
+static void
+cmd_link_show(char **tokens, uint32_t n_tokens, char *out, size_t out_size)
+{
+	struct link *link;
+	struct link_list *link_list;
+	char *link_name;
+	uint32_t i = 0;
+
+	if (n_tokens != 2 && n_tokens != 3) {
+		snprintf(out, out_size, MSG_ARG_MISMATCH, tokens[0]);
+		return;
+	}
+
+	link_list = links_get();
+
+	if (TAILQ_EMPTY(link_list)) {
+		snprintf(out, out_size, "No links - nothing to show\n");
+		return;
+	}
+
+	if (n_tokens == 2) {
+		TAILQ_FOREACH(link, link_list, node) {
+			size_t os = out_size - (i * strlen(out));
+			char *o = &out[i * strlen(out)];
+
+			if (os == 0) {
+				snprintf(out, out_size, MSG_CMD_FAIL,
+						"Output buffer too small");
+				return;
+			}
+
+			print_link_info(link, o, os);
+			i++;
+		}
+	} else {
+		link_name = tokens[2];
+		link = link_find(link_name);
+		print_link_info(link, out, out_size);
+	}
+}
+
 void
 cli_process(char *in, char *out, size_t out_size)
 {
@@ -4380,6 +4478,11 @@ cli_process(char *in, char *out, size_t out_size)
 	}
 
 	if (strcmp(tokens[0], "link") == 0) {
+		if (strcmp(tokens[1], "show") == 0) {
+			cmd_link_show(tokens, n_tokens, out, out_size);
+			return;
+		}
+
 		cmd_link(tokens, n_tokens, out, out_size);
 		return;
 	}
diff --git a/examples/ip_pipeline/link.c b/examples/ip_pipeline/link.c
index b8a431f..b11f0c1 100644
--- a/examples/ip_pipeline/link.c
+++ b/examples/ip_pipeline/link.c
@@ -36,6 +36,12 @@ link_find(const char *name)
 	return NULL;
 }
 
+struct link_list *
+links_get(void)
+{
+	return &link_list;
+}
+
 static struct rte_eth_conf port_conf_default = {
 	.link_speeds = 0,
 	.rxmode = {
diff --git a/examples/ip_pipeline/link.h b/examples/ip_pipeline/link.h
index 37d3dc4..10f9d51 100644
--- a/examples/ip_pipeline/link.h
+++ b/examples/ip_pipeline/link.h
@@ -30,6 +30,9 @@ link_init(void);
 struct link *
 link_find(const char *name);
 
+struct link_list *
+links_get(void);
+
 struct link_params_rss {
 	uint32_t queue_id[LINK_RXQ_RSS_MAX];
 	uint32_t n_queues;
-- 
2.9.5

Re: [dpdk-dev] [PATCH] examples/ip_pipeline: add link show to the cli

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Laatz, Kevin
> Sent: Friday, June 8, 2018 5:31 PM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>; Laatz, Kevin <kevin.laatz@intel.com>
> Subject: [PATCH] examples/ip_pipeline: add link show to the cli
> 
> Add the functionality to track links in the application. This enables the
> user to print the name, mac address and per-port statistics for each link
> in the application.
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
>  examples/ip_pipeline/cli.c  | 103
> ++++++++++++++++++++++++++++++++++++++++++++
>  examples/ip_pipeline/link.c |   6 +++
>  examples/ip_pipeline/link.h |   3 ++
>  3 files changed, 112 insertions(+)
> 
> diff --git a/examples/ip_pipeline/cli.c b/examples/ip_pipeline/cli.c
> index c9587f5..b7fba3b 100644
> --- a/examples/ip_pipeline/cli.c
> +++ b/examples/ip_pipeline/cli.c
> @@ -9,6 +9,7 @@
> 
>  #include <rte_common.h>
>  #include <rte_cycles.h>
> +#include <rte_ethdev.h>
> 
>  #include "cli.h"
>  #include "kni.h"
> @@ -4355,6 +4356,103 @@ cmd_thread_pipeline_disable(char **tokens,
>  	}
>  }
> 
> +/* Print the link stats and info */
> +static void
> +print_link_info(struct link *link, char *out, size_t out_size)
> +{
> +	struct rte_eth_stats stats;
> +	struct ether_addr mac_addr;
> +	struct rte_eth_link eth_link;
> +
> +	memset(&stats, 0, sizeof(stats));
> +	rte_eth_stats_get(link->port_id, &stats);
> +
> +	rte_eth_macaddr_get(link->port_id, &mac_addr);
> +	rte_eth_link_get(link->port_id, &eth_link);
> +
> +	snprintf(out, out_size,
> +		"\n"
> +		"Link name: %s\n"
> +		"\tPort %u MAC: %02"PRIx8":%02"PRIx8":%02"PRIx8
> +		":%02"PRIx8":%02"PRIx8":%02"PRIx8"\n"
> +		"\tLink Status: %s\n"
> +		"\tLink Speed: %u\n"
> +		"\n"
> +		"\tRX info:\n"
> +		"\t\tQueues: %u\n"
> +		"\t\tPackets: %" PRIu64
> +		"\t\tBytes: %" PRIu64"\n"
> +		"\t\tErrors: %" PRIu64
> +		"\t\tMissed: %" PRIu64
> +		"\t\tNo-mbuf: %" PRIu64"\n"
> +		"\tTX info:\n"
> +		"\t\tQueues: %u\n"
> +		"\t\tPackets: %" PRIu64
> +		"\t\tBytes: %" PRIu64"\n"
> +		"\t\tErrors: %" PRIu64"\n",
> +		link->name,
> +		link->port_id,
> +		mac_addr.addr_bytes[0], mac_addr.addr_bytes[1],
> +		mac_addr.addr_bytes[2], mac_addr.addr_bytes[3],
> +		mac_addr.addr_bytes[4], mac_addr.addr_bytes[5],
> +		eth_link.link_status == 1 ? "UP" : "DOWN",
> +		eth_link.link_speed,
> +		link->n_rxq,
> +		stats.ipackets,
> +		stats.ibytes,
> +		stats.ierrors,
> +		stats.imissed,
> +		stats.rx_nombuf,
> +		link->n_txq,
> +		stats.opackets,
> +		stats.obytes,
> +		stats.oerrors);
> +}

Can we organize this as table, so for each link we print exactly one line?

If one link does not fit into a single line, we can have a print_link_info_short() that is used when all links are printed, and a print_link_info_long() that is used only when a single link is printed.

Don't forget about a function to print the table header. :)

> +
> +/*
> + * link show [<link_name>]
> + */
> +static void
> +cmd_link_show(char **tokens, uint32_t n_tokens, char *out, size_t
> out_size)
> +{
> +	struct link *link;
> +	struct link_list *link_list;
> +	char *link_name;
> +	uint32_t i = 0;
> +
> +	if (n_tokens != 2 && n_tokens != 3) {
> +		snprintf(out, out_size, MSG_ARG_MISMATCH, tokens[0]);
> +		return;
> +	}
> +
> +	link_list = links_get();

We don't need this function, the lined list of links is a global public object, please access it directly.

> +
> +	if (TAILQ_EMPTY(link_list)) {
> +		snprintf(out, out_size, "No links - nothing to show\n");
> +		return;
> +	}

No need for this, could be detected by empty table (only table header gets printed).

> +
> +	if (n_tokens == 2) {
> +		TAILQ_FOREACH(link, link_list, node) {
> +			size_t os = out_size - (i * strlen(out));
> +			char *o = &out[i * strlen(out)];
> +
> +			if (os == 0) {
> +				snprintf(out, out_size, MSG_CMD_FAIL,
> +						"Output buffer too small");
> +				return;
> +			}
> +

This check can be removed, no need to worry about output buffer overrun.

> +			print_link_info(link, o, os);
> +			i++;
> +		}
> +	} else {
> +		link_name = tokens[2];
> +		link = link_find(link_name);

What if link with this name does not exist? Checking that link is not NULL might avoid a crash as well :)

> +		print_link_info(link, out, out_size);
> +	}
> +}
> +
>  void
>  cli_process(char *in, char *out, size_t out_size)
>  {
> @@ -4380,6 +4478,11 @@ cli_process(char *in, char *out, size_t out_size)
>  	}
> 
>  	if (strcmp(tokens[0], "link") == 0) {
> +		if (strcmp(tokens[1], "show") == 0) {
> +			cmd_link_show(tokens, n_tokens, out, out_size);
> +			return;
> +		}
> +
>  		cmd_link(tokens, n_tokens, out, out_size);
>  		return;
>  	}
> diff --git a/examples/ip_pipeline/link.c b/examples/ip_pipeline/link.c
> index b8a431f..b11f0c1 100644
> --- a/examples/ip_pipeline/link.c
> +++ b/examples/ip_pipeline/link.c
> @@ -36,6 +36,12 @@ link_find(const char *name)
>  	return NULL;
>  }
> 
> +struct link_list *
> +links_get(void)
> +{
> +	return &link_list;
> +}
> +

Function to be removed, used linked list of links directly (see comment above).

>  static struct rte_eth_conf port_conf_default = {
>  	.link_speeds = 0,
>  	.rxmode = {
> diff --git a/examples/ip_pipeline/link.h b/examples/ip_pipeline/link.h
> index 37d3dc4..10f9d51 100644
> --- a/examples/ip_pipeline/link.h
> +++ b/examples/ip_pipeline/link.h
> @@ -30,6 +30,9 @@ link_init(void);
>  struct link *
>  link_find(const char *name);
> 
> +struct link_list *
> +links_get(void);
> +
>  struct link_params_rss {
>  	uint32_t queue_id[LINK_RXQ_RSS_MAX];
>  	uint32_t n_queues;
> --
> 2.9.5

Re: [dpdk-dev] [PATCH] examples/ip_pipeline: add link show to the cli

[Laatz, Kevin]

Hi Cristian,

> > +
> > +	link_list = links_get();
> 
> We don't need this function, the lined list of links is a global public object,
> please access it directly.

link_list is declared as a static struct in link.c so it is not accessible from cli.c

I can either leave this function in to pass the pointer to the cli.c or move the declaration to link.h. 
Do you have a preference?

> >
> > +struct link_list *
> > +links_get(void)
> > +{
> > +	return &link_list;
> > +}
> > +
> 
> Function to be removed, used linked list of links directly (see comment
> above).

See above comment 

Thanks,
Kevin

Re: [dpdk-dev] [PATCH] examples/ip_pipeline: add link show to the cli

[Dumitrescu, Cristian]

Hi Kevin,

If you only need access to this linked list for iterating through it, you can add a new function: struct link *link_next(struct link *) that gets you the next element in the list (for first element, invoke with link = NULL; at and of list, it returns NULL). Makes sense?

Are there other usages of this linked list that you need to cover?

Regards,
Cristian

> -----Original Message-----
> From: Laatz, Kevin
> Sent: Friday, June 15, 2018 3:57 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org
> Cc: Singh, Jasvinder <jasvinder.singh@intel.com>
> Subject: RE: [PATCH] examples/ip_pipeline: add link show to the cli
> 
> Hi Cristian,
> 
> > > +
> > > +	link_list = links_get();
> >
> > We don't need this function, the lined list of links is a global public object,
> > please access it directly.
> 
> link_list is declared as a static struct in link.c so it is not accessible from cli.c
> 
> I can either leave this function in to pass the pointer to the cli.c or move the
> declaration to link.h.
> Do you have a preference?
> 
> > >
> > > +struct link_list *
> > > +links_get(void)
> > > +{
> > > +	return &link_list;
> > > +}
> > > +
> >
> > Function to be removed, used linked list of links directly (see comment
> > above).
> 
> See above comment
> 
> Thanks,
> Kevin

[dpdk-dev] [PATCH v2] examples/ip_pipeline: add link show to the cli

[Kevin Laatz]

Add the functionality to track links in the application. This enables the
user to print the name, mac address and statistics for each link
in the application.

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

---
v2:
  - Implemeted API to iterate through the linked list
  - Printing tidy up
  - Removed unnecessary checks
---
 examples/ip_pipeline/cli.c  | 91 +++++++++++++++++++++++++++++++++++++++++++++
 examples/ip_pipeline/link.c |  6 +++
 examples/ip_pipeline/link.h |  3 ++
 3 files changed, 100 insertions(+)

diff --git a/examples/ip_pipeline/cli.c b/examples/ip_pipeline/cli.c
index c9587f5..27c9844 100644
--- a/examples/ip_pipeline/cli.c
+++ b/examples/ip_pipeline/cli.c
@@ -9,6 +9,7 @@
 
 #include <rte_common.h>
 #include <rte_cycles.h>
+#include <rte_ethdev.h>
 
 #include "cli.h"
 #include "kni.h"
@@ -237,6 +238,91 @@ cmd_link(char **tokens,
 	}
 }
 
+/* Print the link stats and info */
+static void
+print_link_info(struct link *link, char *out, size_t out_size)
+{
+	struct rte_eth_stats stats;
+	struct ether_addr mac_addr;
+	struct rte_eth_link eth_link;
+	uint16_t mtu;
+
+	memset(&stats, 0, sizeof(stats));
+	rte_eth_stats_get(link->port_id, &stats);
+
+	rte_eth_macaddr_get(link->port_id, &mac_addr);
+	rte_eth_link_get(link->port_id, &eth_link);
+	rte_eth_dev_get_mtu(link->port_id, &mtu);
+
+	snprintf(out, out_size,
+		"\n"
+		"%s: flags=<%s> mtu %u\n"
+		"\tether %02X:%02X:%02X:%02X:%02X:%02X rxqueues %u txqueues %u\n"
+		"\tport# %u  speed %u Mbps\n"
+		"\tRX packets %" PRIu64"  bytes %" PRIu64"\n"
+		"\tRX errors %" PRIu64"  missed %" PRIu64"  no-mbuf %" PRIu64"\n"
+		"\tTX packets %" PRIu64"  bytes %" PRIu64"\n"
+		"\tTX errors %" PRIu64"\n",
+		link->name,
+		eth_link.link_status == 0 ? "DOWN" : "UP",
+		mtu,
+		mac_addr.addr_bytes[0], mac_addr.addr_bytes[1],
+		mac_addr.addr_bytes[2], mac_addr.addr_bytes[3],
+		mac_addr.addr_bytes[4], mac_addr.addr_bytes[5],
+		link->n_rxq,
+		link->n_txq,
+		link->port_id,
+		eth_link.link_speed,
+		stats.ipackets,
+		stats.ibytes,
+		stats.ierrors,
+		stats.imissed,
+		stats.rx_nombuf,
+		stats.opackets,
+		stats.obytes,
+		stats.oerrors);
+}
+
+/*
+ * link show [<link_name>]
+ */
+static void
+cmd_link_show(char **tokens, uint32_t n_tokens, char *out, size_t out_size)
+{
+	struct link *link;
+	char *link_name;
+
+	if (n_tokens != 2 && n_tokens != 3) {
+		snprintf(out, out_size, MSG_ARG_MISMATCH, tokens[0]);
+		return;
+	}
+
+	if (n_tokens == 2) {
+		link = link_next(NULL);
+
+		while (link != NULL) {
+			out_size = out_size - strlen(out);
+			out = &out[strlen(out)];
+
+			print_link_info(link, out, out_size);
+			link = link_next(link);
+		}
+	} else {
+		out_size = out_size - strlen(out);
+		out = &out[strlen(out)];
+
+		link_name = tokens[2];
+		link = link_find(link_name);
+
+		if (link == NULL) {
+			snprintf(out, out_size, MSG_ARG_INVALID,
+					"Link does not exist");
+			return;
+		}
+		print_link_info(link, out, out_size);
+	}
+}
+
 /**
  * swq <swq_name>
  *  size <size>
@@ -4380,6 +4466,11 @@ cli_process(char *in, char *out, size_t out_size)
 	}
 
 	if (strcmp(tokens[0], "link") == 0) {
+		if (strcmp(tokens[1], "show") == 0) {
+			cmd_link_show(tokens, n_tokens, out, out_size);
+			return;
+		}
+
 		cmd_link(tokens, n_tokens, out, out_size);
 		return;
 	}
diff --git a/examples/ip_pipeline/link.c b/examples/ip_pipeline/link.c
index b8a431f..28a46d2 100644
--- a/examples/ip_pipeline/link.c
+++ b/examples/ip_pipeline/link.c
@@ -36,6 +36,12 @@ link_find(const char *name)
 	return NULL;
 }
 
+struct link *
+link_next(struct link *link)
+{
+	return (link == NULL) ? TAILQ_FIRST(&link_list) : TAILQ_NEXT(link, node);
+}
+
 static struct rte_eth_conf port_conf_default = {
 	.link_speeds = 0,
 	.rxmode = {
diff --git a/examples/ip_pipeline/link.h b/examples/ip_pipeline/link.h
index 37d3dc4..34ff114 100644
--- a/examples/ip_pipeline/link.h
+++ b/examples/ip_pipeline/link.h
@@ -30,6 +30,9 @@ link_init(void);
 struct link *
 link_find(const char *name);
 
+struct link *
+link_next(struct link *link);
+
 struct link_params_rss {
 	uint32_t queue_id[LINK_RXQ_RSS_MAX];
 	uint32_t n_queues;
-- 
2.9.5