diff mbox series

[v3,6/7] app/proc-info: provide way to request info on owned ports

Message ID 20200715212228.28010-7-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series app/proc-info enhancments |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Stephen Hemminger July 15, 2020, 9:22 p.m. UTC 
  There are cases where a port maybe owned by another (failsafe, netvsc,
bond); but currently proc-info has no way to look at stats of those
ports. This patch provides way for the user to explicitly ask for these
ports.

If no portmask is given the output is unchanged; it only shows the
top level ports. If portmask requests a specific port it will be
shown even if owned.

The device owner is also a useful thing to show in port info.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 app/proc-info/Makefile |  3 ++
 app/proc-info/main.c   | 74 ++++++++++++++++++++++++++++--------------
 2 files changed, 53 insertions(+), 24 deletions(-)

Comments

Thomas Monjalon July 17, 2020, 3:01 p.m. UTC | #1 
15/07/2020 23:22, Stephen Hemminger:
> --- a/app/proc-info/Makefile
> +++ b/app/proc-info/Makefile
> +CFLAGS += -DALLOW_EXPERIMENTAL_API

not needed in app/

> +CFLAGS += -O3
>  CFLAGS += $(WERROR_FLAGS)
> +CFLAGS += -Wno-deprecated-declarations

Which deprecated function is used?
We must not use deprecated functions.

> +	/* If no port mask was specified, one will be provided */

Would be nice to help the user by printing a port mask
of owned and unowned ports.

> +	if (enabled_port_mask == 0) {
> +		RTE_ETH_FOREACH_DEV(i) {
> +			enabled_port_mask |= 1u << i;
>  		}
>  	}
>  
> +	for (port_mask = enabled_port_mask; port_mask != 0;
> +	     port_mask &= ~(1u << i)) {

Please would be good to help drunk or sleepy readers with few comments.

> +		/* ffs() first bit is 1 not 0 */
> +		i = ffs(port_mask) - 1;
> +
> +		if (i >= RTE_MAX_ETHPORTS)
> +			break;

This check is already done in rte_eth_dev_is_valid_port().

> +
> +		if (!rte_eth_dev_is_valid_port(i))
> +			continue;
Stephen Hemminger July 21, 2020, 5:05 p.m. UTC | #2 
On Fri, 17 Jul 2020 17:01:00 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 15/07/2020 23:22, Stephen Hemminger:
> > --- a/app/proc-info/Makefile
> > +++ b/app/proc-info/Makefile
> > +CFLAGS += -DALLOW_EXPERIMENTAL_API  
> 
> not needed in app/
> 
> > +CFLAGS += -O3
> >  CFLAGS += $(WERROR_FLAGS)
> > +CFLAGS += -Wno-deprecated-declarations  
> 
> Which deprecated function is used?
> We must not use deprecated functions.

Fixing.

> 
> > +	/* If no port mask was specified, one will be provided */  
> 
> Would be nice to help the user by printing a port mask
> of owned and unowned ports.

Not needed, since each display already has the port #

> 
> > +	if (enabled_port_mask == 0) {
> > +		RTE_ETH_FOREACH_DEV(i) {
> > +			enabled_port_mask |= 1u << i;
> >  		}
> >  	}
> >  
> > +	for (port_mask = enabled_port_mask; port_mask != 0;
> > +	     port_mask &= ~(1u << i)) {  
> 
> Please would be good to help drunk or sleepy readers with few comments.
ok?

> 
> > +		/* ffs() first bit is 1 not 0 */
> > +		i = ffs(port_mask) - 1;
> > +
> > +		if (i >= RTE_MAX_ETHPORTS)
> > +			break;  
> 
> This check is already done in rte_eth_dev_is_valid_port().

Want to stop early if port is out of range,
but continue if hits a port that is owned.


> 
> > +
> > +		if (!rte_eth_dev_is_valid_port(i))
> > +			continue;  
> 
> 
>
Thomas Monjalon July 21, 2020, 5:08 p.m. UTC | #3 
21/07/2020 19:05, Stephen Hemminger:
> On Fri, 17 Jul 2020 17:01:00 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> > 15/07/2020 23:22, Stephen Hemminger:
> > > +	/* If no port mask was specified, one will be provided */  
> > 
> > Would be nice to help the user by printing a port mask
> > of owned and unowned ports.
> 
> Not needed, since each display already has the port #

By default, owned ports are not displayed at all.
How are we supposed to find them?

> > > +	if (enabled_port_mask == 0) {
> > > +		RTE_ETH_FOREACH_DEV(i) {
> > > +			enabled_port_mask |= 1u << i;
> > >  		}
> > >  	}
> > >  
> > > +	for (port_mask = enabled_port_mask; port_mask != 0;
> > > +	     port_mask &= ~(1u << i)) {  
> > 
> > Please would be good to help drunk or sleepy readers with few comments.
> ok?

Comments in the code I mean.

> > > +		/* ffs() first bit is 1 not 0 */
> > > +		i = ffs(port_mask) - 1;
> > > +
> > > +		if (i >= RTE_MAX_ETHPORTS)
> > > +			break;  
> > 
> > This check is already done in rte_eth_dev_is_valid_port().
> 
> Want to stop early if port is out of range,
> but continue if hits a port that is owned.

OK

> > > +
> > > +		if (!rte_eth_dev_is_valid_port(i))
> > > +			continue;
Stephen Hemminger July 21, 2020, 5:37 p.m. UTC | #4 
On Tue, 21 Jul 2020 19:08:57 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 21/07/2020 19:05, Stephen Hemminger:
> > On Fri, 17 Jul 2020 17:01:00 +0200
> > Thomas Monjalon <thomas@monjalon.net> wrote:  
> > > 15/07/2020 23:22, Stephen Hemminger:  
> > > > +	/* If no port mask was specified, one will be provided */    
> > > 
> > > Would be nice to help the user by printing a port mask
> > > of owned and unowned ports.  
> > 
> > Not needed, since each display already has the port #  
> 
> By default, owned ports are not displayed at all.
> How are we supposed to find them?

You have to pass it in portmask.
The alternative would be to show all ports and mark those that are owned.

The ownership model API does not support a way to ask for either
"tell me which other port owns port X" or "iterate over all the ports owned by Y".
The problem is that the owner id is generic and stored inside the PMD.
The root cause is that the original design of port ownership was targeting
a more general use case that was never used. I would have been happier with
a simpler parent port id index in ethdev instead.
diff mbox series

Patch

diff --git a/app/proc-info/Makefile b/app/proc-info/Makefile
index 214f3f54a1e9..f777e037861f 100644
--- a/app/proc-info/Makefile
+++ b/app/proc-info/Makefile
@@ -5,7 +5,10 @@  include $(RTE_SDK)/mk/rte.vars.mk
 
 APP = dpdk-procinfo
 
+CFLAGS += -DALLOW_EXPERIMENTAL_API
+CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
+CFLAGS += -Wno-deprecated-declarations
 
 # all source are stored in SRCS-y
 
diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 5000724fcdd1..14c04f8367f1 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -12,6 +12,7 @@ 
 #include <stdlib.h>
 #include <getopt.h>
 #include <unistd.h>
+#include <strings.h>
 
 #include <rte_eal.h>
 #include <rte_common.h>
@@ -676,19 +677,26 @@  eth_tx_queue_available(uint16_t port_id, uint16_t queue_id, uint16_t n)
 static void
 show_port(void)
 {
-	uint16_t i = 0;
-	int ret = 0, j, k;
+	uint32_t port_mask = enabled_port_mask;
+	int i, ret, j, k;
 
 	snprintf(bdr_str, MAX_STRING_LEN, " show - Port PMD ");
 	STATS_BDR_STR(10, bdr_str);
 
-	RTE_ETH_FOREACH_DEV(i) {
+	for (port_mask = enabled_port_mask; port_mask != 0;
+	     port_mask &= ~(1u << i)) {
 		uint16_t mtu = 0;
 		struct rte_eth_link link;
 		struct rte_eth_dev_info dev_info;
 		struct rte_eth_rss_conf rss_conf;
 		struct rte_eth_fc_conf fc_conf;
 		struct rte_ether_addr mac;
+		struct rte_eth_dev_owner owner;
+
+		i = ffs(port_mask) - 1;
+
+		if (!rte_eth_dev_is_valid_port(i))
+			continue;
 
 		memset(&rss_conf, 0, sizeof(rss_conf));
 
@@ -707,6 +715,11 @@  show_port(void)
 		       dev_info.driver_name, dev_info.device->name,
 		       rte_eth_dev_socket_id(i));
 
+		ret = rte_eth_dev_owner_get(i, &owner);
+		if (ret == 0 && owner.id != RTE_ETH_DEV_NO_OWNER)
+			printf("\t --  owner %#"PRIx64":%s\n",
+			       owner.id, owner.name);
+
 		ret = rte_eth_link_get(i, &link);
 		if (ret < 0) {
 			printf("Link get failed (port %u): %s\n",
@@ -1351,6 +1364,7 @@  main(int argc, char **argv)
 	char log_flag[] = "--log-level=6";
 	char *argp[argc + 4];
 	uint16_t nb_ports;
+	uint32_t port_mask;
 
 	/* preparse app arguments */
 	ret = proc_info_preparse_args(argc, argv);
@@ -1394,30 +1408,42 @@  main(int argc, char **argv)
 	if (nb_ports == 0)
 		rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
 
-	/* If no port mask was specified*/
-	if (enabled_port_mask == 0)
-		enabled_port_mask = 0xffff;
-
-	RTE_ETH_FOREACH_DEV(i) {
-		if (enabled_port_mask & (1 << i)) {
-			if (enable_stats)
-				nic_stats_display(i);
-			else if (enable_xstats)
-				nic_xstats_display(i);
-			else if (reset_stats)
-				nic_stats_clear(i);
-			else if (reset_xstats)
-				nic_xstats_clear(i);
-			else if (enable_xstats_name)
-				nic_xstats_by_name_display(i, xstats_name);
-			else if (nb_xstats_ids > 0)
-				nic_xstats_by_ids_display(i, xstats_ids,
-						nb_xstats_ids);
-			else if (enable_metrics)
-				metrics_display(i);
+	/* If no port mask was specified, one will be provided */
+	if (enabled_port_mask == 0) {
+		RTE_ETH_FOREACH_DEV(i) {
+			enabled_port_mask |= 1u << i;
 		}
 	}
 
+	for (port_mask = enabled_port_mask; port_mask != 0;
+	     port_mask &= ~(1u << i)) {
+		/* ffs() first bit is 1 not 0 */
+		i = ffs(port_mask) - 1;
+
+		if (i >= RTE_MAX_ETHPORTS)
+			break;
+
+		if (!rte_eth_dev_is_valid_port(i))
+			continue;
+
+		if (enable_stats)
+			nic_stats_display(i);
+		else if (enable_xstats)
+			nic_xstats_display(i);
+		else if (reset_stats)
+			nic_stats_clear(i);
+		else if (reset_xstats)
+			nic_xstats_clear(i);
+		else if (enable_xstats_name)
+			nic_xstats_by_name_display(i, xstats_name);
+		else if (nb_xstats_ids > 0)
+			nic_xstats_by_ids_display(i, xstats_ids,
+						  nb_xstats_ids);
+		else if (enable_metrics)
+			metrics_display(i);
+
+	}
+
 	/* print port independent stats */
 	if (enable_metrics)
 		metrics_display(RTE_METRICS_GLOBAL);