[dpdk-dev] [PATCH v3 0/2] examples/client_server_mp: fix port issues

[dpdk-dev] [PATCH v3 0/2] examples/client_server_mp: fix port issues

[Stephen Hemminger]

The client_server_mp application does not work correctly
Azure/Hyper-V because it does not handle the concept of some
ports being hidden and unavailable.

Stephen Hemminger (2):
  examples/multi_process/client_server_mp: check port validity
  examples/multi_process - fix crash in mp_client with sparse ports

v3 - merge both patches in one series
     use alternative algorithm to check port ownership (N^2)
     because reviewer didn't like direct check.

 .../client_server_mp/mp_client/client.c       | 18 ++++----
 .../client_server_mp/mp_server/args.c         | 46 +++++++++++++------
 .../client_server_mp/mp_server/args.h         |  2 +-
 .../client_server_mp/mp_server/init.c         |  7 +--
 4 files changed, 44 insertions(+), 29 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v3 1/2] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

From: Stephen Hemminger <sthemmin@microsoft.com>

The mp_server incorrectly allows a port mask that included hidden
ports and which later caused either lost packets or failed initialization.

This fixes explicitly checking that each bit in portmask is a
valid port before using it.

The max_ports parameter is no longer necessary, so remove it
from the caller.

Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 .../client_server_mp/mp_server/args.c         | 46 +++++++++++++------
 .../client_server_mp/mp_server/args.h         |  2 +-
 .../client_server_mp/mp_server/init.c         |  7 +--
 3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_server/args.c b/examples/multi_process/client_server_mp/mp_server/args.c
index b0d8d7665c85..1d52489347df 100644
--- a/examples/multi_process/client_server_mp/mp_server/args.c
+++ b/examples/multi_process/client_server_mp/mp_server/args.c
@@ -10,6 +10,7 @@
 #include <errno.h>
 
 #include <rte_memory.h>
+#include <rte_ethdev.h>
 #include <rte_string_fns.h>
 
 #include "common.h"
@@ -34,6 +35,22 @@ usage(void)
 	    , progname);
 }
 
+/**
+ * Check if port is present in the system
+ * It maybe owned by a device and should not be used.
+ */
+static int
+port_is_present(uint16_t portid)
+{
+	uint16_t id;
+
+	RTE_ETH_FOREACH_DEV(id) {
+		if (id == portid)
+			return 1;
+	}
+	return 0;
+}
+
 /**
  * The ports to be used by the application are passed in
  * the form of a bitmask. This function parses the bitmask
@@ -41,31 +58,32 @@ usage(void)
  * array variable
  */
 static int
-parse_portmask(uint8_t max_ports, const char *portmask)
+parse_portmask(const char *portmask)
 {
 	char *end = NULL;
 	unsigned long pm;
-	uint16_t count = 0;
+	uint16_t count;
 
 	if (portmask == NULL || *portmask == '\0')
 		return -1;
 
 	/* convert parameter to a number and verify */
 	pm = strtoul(portmask, &end, 16);
-	if (end == NULL || *end != '\0' || pm == 0)
+	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
 		return -1;
 
 	/* loop through bits of the mask and mark ports */
-	while (pm != 0){
-		if (pm & 0x01){ /* bit is set in mask, use port */
-			if (count >= max_ports)
-				printf("WARNING: requested port %u not present"
-				" - ignoring\n", (unsigned)count);
-			else
-			    ports->id[ports->num_ports++] = count;
+	for (count = 0; pm != 0; pm >>= 1, ++count) {
+		if ((pm & 0x1) == 0)
+			continue;
+
+		if (!port_is_present(count)) {
+			printf("WARNING: requested port %u not present - ignoring\n",
+				count);
+			continue;
 		}
-		pm = (pm >> 1);
-		count++;
+
+		ports->id[ports->num_ports++] = count;
 	}
 
 	return 0;
@@ -99,7 +117,7 @@ parse_num_clients(const char *clients)
  * on error.
  */
 int
-parse_app_args(uint16_t max_ports, int argc, char *argv[])
+parse_app_args(int argc, char *argv[])
 {
 	int option_index, opt;
 	char **argvopt = argv;
@@ -112,7 +130,7 @@ parse_app_args(uint16_t max_ports, int argc, char *argv[])
 		&option_index)) != EOF){
 		switch (opt){
 			case 'p':
-				if (parse_portmask(max_ports, optarg) != 0){
+				if (parse_portmask(optarg) != 0){
 					usage();
 					return -1;
 				}
diff --git a/examples/multi_process/client_server_mp/mp_server/args.h b/examples/multi_process/client_server_mp/mp_server/args.h
index 79c190a33a37..52c8cc86e6f0 100644
--- a/examples/multi_process/client_server_mp/mp_server/args.h
+++ b/examples/multi_process/client_server_mp/mp_server/args.h
@@ -5,6 +5,6 @@
 #ifndef _ARGS_H_
 #define _ARGS_H_
 
-int parse_app_args(uint16_t max_ports, int argc, char *argv[]);
+int parse_app_args(int argc, char *argv[]);
 
 #endif /* ifndef _ARGS_H_ */
diff --git a/examples/multi_process/client_server_mp/mp_server/init.c b/examples/multi_process/client_server_mp/mp_server/init.c
index 3af5dc6994bf..1b0569937b51 100644
--- a/examples/multi_process/client_server_mp/mp_server/init.c
+++ b/examples/multi_process/client_server_mp/mp_server/init.c
@@ -238,7 +238,7 @@ init(int argc, char *argv[])
 {
 	int retval;
 	const struct rte_memzone *mz;
-	uint16_t i, total_ports;
+	uint16_t i;
 
 	/* init EAL, parsing EAL args */
 	retval = rte_eal_init(argc, argv);
@@ -247,9 +247,6 @@ init(int argc, char *argv[])
 	argc -= retval;
 	argv += retval;
 
-	/* get total number of ports */
-	total_ports = rte_eth_dev_count_total();
-
 	/* set up array for port data */
 	mz = rte_memzone_reserve(MZ_PORT_INFO, sizeof(*ports),
 				rte_socket_id(), NO_FLAGS);
@@ -259,7 +256,7 @@ init(int argc, char *argv[])
 	ports = mz->addr;
 
 	/* parse additional, application arguments */
-	retval = parse_app_args(total_ports, argc, argv);
+	retval = parse_app_args(argc, argv);
 	if (retval != 0)
 		return -1;
 
-- 
2.20.1

[dpdk-dev] [PATCH v3 2/2] examples/multi_process - fix crash in mp_client with sparse ports

[Stephen Hemminger]

From: Stephen Hemminger <sthemmin@microsoft.com>

The mp_client crashes if run on Azure or any system where ethdev
ports are owned. In that case, the tx_buffer and tx_stats for the
real port were initialized correctly, but the wrong port was used.

For example if the server has Ports 3 and 5. Then calling
rte_eth_tx_buffer_flush on any other buffer will dereference null
because the tx buffer for that port was not allocated.

Also:
   - the flush code is common enough that it should not be marked
     unlikely
   - combine conditions to reduce indentation
   - avoid unnecessary if() if sent is zero.

Fixes: e2366e74e029 ("examples: use buffered Tx")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 .../client_server_mp/mp_client/client.c        | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_client/client.c b/examples/multi_process/client_server_mp/mp_client/client.c
index c23dd3f378f7..361d90b54b2d 100644
--- a/examples/multi_process/client_server_mp/mp_client/client.c
+++ b/examples/multi_process/client_server_mp/mp_client/client.c
@@ -246,19 +246,19 @@ main(int argc, char *argv[])
 
 	for (;;) {
 		uint16_t i, rx_pkts;
-		uint16_t port;
 
 		rx_pkts = rte_ring_dequeue_burst(rx_ring, pkts,
 				PKT_READ_SIZE, NULL);
 
-		if (unlikely(rx_pkts == 0)){
-			if (need_flush)
-				for (port = 0; port < ports->num_ports; port++) {
-					sent = rte_eth_tx_buffer_flush(ports->id[port], client_id,
-							tx_buffer[port]);
-					if (unlikely(sent))
-						tx_stats->tx[port] += sent;
-				}
+		if (rx_pkts == 0 && need_flush) {
+			for (i = 0; i < ports->num_ports; i++) {
+				uint16_t port = ports->id[i];
+
+				sent = rte_eth_tx_buffer_flush(port,
+							       client_id,
+							       tx_buffer[port]);
+				tx_stats->tx[port] += sent;
+			}
 			need_flush = 0;
 			continue;
 		}
-- 
2.20.1

[dpdk-dev] [PATCH v4 0/4] examples/client_server_mp: fix port issues

[Stephen Hemminger]

The client_server_mp application does not work correctly
with failsafe or other devices using port ownership.

v4 - fix checkpatch warning
     add patches to fix style issues and use ether format addr

v3 - merge both patches in one series
     use alternative algorithm to check port ownership (N^2)
     because reviewer didn't like direct check.

Stephen Hemminger (4):
  examples/multi_process/client_server_mp: check port validity
  examples/multi_process/client_server_mp - fix crash in mp_client with
    sparse ports
  examples/multi_process/client_server_mp/mp_server: fix style
  examples/multi_process/client_server_mp/mp_server: use ether format
    address

 .../client_server_mp/mp_client/client.c       | 18 ++---
 .../client_server_mp/mp_server/args.c         | 81 ++++++++++++-------
 .../client_server_mp/mp_server/args.h         |  2 +-
 .../client_server_mp/mp_server/init.c         | 65 ++++++++-------
 .../client_server_mp/mp_server/main.c         | 68 +++++++---------
 5 files changed, 124 insertions(+), 110 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v4 1/4] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

From: Stephen Hemminger <sthemmin@microsoft.com>

The mp_server incorrectly allows a port mask that included hidden
ports and which later caused either lost packets or failed initialization.

This fixes explicitly checking that each bit in portmask is a
valid port before using it.

Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 .../client_server_mp/mp_server/args.c         | 46 +++++++++++++------
 .../client_server_mp/mp_server/args.h         |  2 +-
 .../client_server_mp/mp_server/init.c         |  7 +--
 3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_server/args.c b/examples/multi_process/client_server_mp/mp_server/args.c
index b0d8d7665c85..c1ab12ad00d1 100644
--- a/examples/multi_process/client_server_mp/mp_server/args.c
+++ b/examples/multi_process/client_server_mp/mp_server/args.c
@@ -10,6 +10,7 @@
 #include <errno.h>
 
 #include <rte_memory.h>
+#include <rte_ethdev.h>
 #include <rte_string_fns.h>
 
 #include "common.h"
@@ -34,6 +35,22 @@ usage(void)
 	    , progname);
 }
 
+/**
+ * Check if port is present in the system
+ * It maybe owned by a device and should not be used.
+ */
+static int
+port_is_present(uint16_t portid)
+{
+	uint16_t id;
+
+	RTE_ETH_FOREACH_DEV(id) {
+		if (id == portid)
+			return 1;
+	}
+	return 0;
+}
+
 /**
  * The ports to be used by the application are passed in
  * the form of a bitmask. This function parses the bitmask
@@ -41,31 +58,32 @@ usage(void)
  * array variable
  */
 static int
-parse_portmask(uint8_t max_ports, const char *portmask)
+parse_portmask(const char *portmask)
 {
 	char *end = NULL;
 	unsigned long pm;
-	uint16_t count = 0;
+	uint16_t count;
 
 	if (portmask == NULL || *portmask == '\0')
 		return -1;
 
 	/* convert parameter to a number and verify */
 	pm = strtoul(portmask, &end, 16);
-	if (end == NULL || *end != '\0' || pm == 0)
+	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
 		return -1;
 
 	/* loop through bits of the mask and mark ports */
-	while (pm != 0){
-		if (pm & 0x01){ /* bit is set in mask, use port */
-			if (count >= max_ports)
-				printf("WARNING: requested port %u not present"
-				" - ignoring\n", (unsigned)count);
-			else
-			    ports->id[ports->num_ports++] = count;
+	for (count = 0; pm != 0; pm >>= 1, ++count) {
+		if ((pm & 0x1) == 0)
+			continue;
+
+		if (!port_is_present(count)) {
+			printf("WARNING: requested port %u not present - ignoring\n",
+				count);
+			continue;
 		}
-		pm = (pm >> 1);
-		count++;
+
+		ports->id[ports->num_ports++] = count;
 	}
 
 	return 0;
@@ -99,7 +117,7 @@ parse_num_clients(const char *clients)
  * on error.
  */
 int
-parse_app_args(uint16_t max_ports, int argc, char *argv[])
+parse_app_args(int argc, char *argv[])
 {
 	int option_index, opt;
 	char **argvopt = argv;
@@ -112,7 +130,7 @@ parse_app_args(uint16_t max_ports, int argc, char *argv[])
 		&option_index)) != EOF){
 		switch (opt){
 			case 'p':
-				if (parse_portmask(max_ports, optarg) != 0){
+				if (parse_portmask(optarg) != 0) {
 					usage();
 					return -1;
 				}
diff --git a/examples/multi_process/client_server_mp/mp_server/args.h b/examples/multi_process/client_server_mp/mp_server/args.h
index 79c190a33a37..52c8cc86e6f0 100644
--- a/examples/multi_process/client_server_mp/mp_server/args.h
+++ b/examples/multi_process/client_server_mp/mp_server/args.h
@@ -5,6 +5,6 @@
 #ifndef _ARGS_H_
 #define _ARGS_H_
 
-int parse_app_args(uint16_t max_ports, int argc, char *argv[]);
+int parse_app_args(int argc, char *argv[]);
 
 #endif /* ifndef _ARGS_H_ */
diff --git a/examples/multi_process/client_server_mp/mp_server/init.c b/examples/multi_process/client_server_mp/mp_server/init.c
index 3af5dc6994bf..1b0569937b51 100644
--- a/examples/multi_process/client_server_mp/mp_server/init.c
+++ b/examples/multi_process/client_server_mp/mp_server/init.c
@@ -238,7 +238,7 @@ init(int argc, char *argv[])
 {
 	int retval;
 	const struct rte_memzone *mz;
-	uint16_t i, total_ports;
+	uint16_t i;
 
 	/* init EAL, parsing EAL args */
 	retval = rte_eal_init(argc, argv);
@@ -247,9 +247,6 @@ init(int argc, char *argv[])
 	argc -= retval;
 	argv += retval;
 
-	/* get total number of ports */
-	total_ports = rte_eth_dev_count_total();
-
 	/* set up array for port data */
 	mz = rte_memzone_reserve(MZ_PORT_INFO, sizeof(*ports),
 				rte_socket_id(), NO_FLAGS);
@@ -259,7 +256,7 @@ init(int argc, char *argv[])
 	ports = mz->addr;
 
 	/* parse additional, application arguments */
-	retval = parse_app_args(total_ports, argc, argv);
+	retval = parse_app_args(argc, argv);
 	if (retval != 0)
 		return -1;
 
-- 
2.20.1

[dpdk-dev] [PATCH v4 2/4] examples/multi_process/client_server_mp - fix crash in mp_client with sparse ports

[Stephen Hemminger]

From: Stephen Hemminger <sthemmin@microsoft.com>

The mp_client crashes if run on Azure or any system where ethdev
ports are owned. In that case, the tx_buffer and tx_stats for the
real port were initialized correctly, but the wrong port was used.

For example if the server has Ports 3 and 5. Then calling
rte_eth_tx_buffer_flush on any other buffer will dereference null
because the tx buffer for that port was not allocated.

Also:
   - the flush code is common enough that it should not be marked
     unlikely
   - combine conditions to reduce indentation
   - avoid unnecessary if() if sent is zero.

Fixes: e2366e74e029 ("examples: use buffered Tx")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 .../client_server_mp/mp_client/client.c        | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_client/client.c b/examples/multi_process/client_server_mp/mp_client/client.c
index c23dd3f378f7..361d90b54b2d 100644
--- a/examples/multi_process/client_server_mp/mp_client/client.c
+++ b/examples/multi_process/client_server_mp/mp_client/client.c
@@ -246,19 +246,19 @@ main(int argc, char *argv[])
 
 	for (;;) {
 		uint16_t i, rx_pkts;
-		uint16_t port;
 
 		rx_pkts = rte_ring_dequeue_burst(rx_ring, pkts,
 				PKT_READ_SIZE, NULL);
 
-		if (unlikely(rx_pkts == 0)){
-			if (need_flush)
-				for (port = 0; port < ports->num_ports; port++) {
-					sent = rte_eth_tx_buffer_flush(ports->id[port], client_id,
-							tx_buffer[port]);
-					if (unlikely(sent))
-						tx_stats->tx[port] += sent;
-				}
+		if (rx_pkts == 0 && need_flush) {
+			for (i = 0; i < ports->num_ports; i++) {
+				uint16_t port = ports->id[i];
+
+				sent = rte_eth_tx_buffer_flush(port,
+							       client_id,
+							       tx_buffer[port]);
+				tx_stats->tx[port] += sent;
+			}
 			need_flush = 0;
 			continue;
 		}
-- 
2.20.1

[dpdk-dev] [PATCH v4 3/4] examples/multi_process/client_server_mp/mp_server: fix style

[Stephen Hemminger]

Lots of little style complaints from checkpatch.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 .../client_server_mp/mp_server/args.c         | 37 ++++++------
 .../client_server_mp/mp_server/init.c         | 58 +++++++++++--------
 .../client_server_mp/mp_server/main.c         | 44 +++++++-------
 3 files changed, 76 insertions(+), 63 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_server/args.c b/examples/multi_process/client_server_mp/mp_server/args.c
index c1ab12ad00d1..f8c501a13d5e 100644
--- a/examples/multi_process/client_server_mp/mp_server/args.c
+++ b/examples/multi_process/client_server_mp/mp_server/args.c
@@ -127,33 +127,34 @@ parse_app_args(int argc, char *argv[])
 	progname = argv[0];
 
 	while ((opt = getopt_long(argc, argvopt, "n:p:", lgopts,
-		&option_index)) != EOF){
-		switch (opt){
-			case 'p':
-				if (parse_portmask(optarg) != 0) {
-					usage();
-					return -1;
-				}
-				break;
-			case 'n':
-				if (parse_num_clients(optarg) != 0){
-					usage();
-					return -1;
-				}
-				break;
-			default:
-				printf("ERROR: Unknown option '%c'\n", opt);
+				  &option_index)) != EOF) {
+
+		switch (opt) {
+		case 'p':
+			if (parse_portmask(optarg) != 0) {
+				usage();
+				return -1;
+			}
+			break;
+		case 'n':
+			if (parse_num_clients(optarg) != 0) {
 				usage();
 				return -1;
+			}
+			break;
+		default:
+			printf("ERROR: Unknown option '%c'\n", opt);
+			usage();
+			return -1;
 		}
 	}
 
-	if (ports->num_ports == 0 || num_clients == 0){
+	if (ports->num_ports == 0 || num_clients == 0) {
 		usage();
 		return -1;
 	}
 
-	if (ports->num_ports % 2 != 0){
+	if (ports->num_ports % 2 != 0) {
 		printf("ERROR: application requires an even number of ports to use\n");
 		return -1;
 	}
diff --git a/examples/multi_process/client_server_mp/mp_server/init.c b/examples/multi_process/client_server_mp/mp_server/init.c
index 1b0569937b51..26416f42cf03 100644
--- a/examples/multi_process/client_server_mp/mp_server/init.c
+++ b/examples/multi_process/client_server_mp/mp_server/init.c
@@ -49,7 +49,7 @@
 struct rte_mempool *pktmbuf_pool;
 
 /* array of info/queues for clients */
-struct client *clients = NULL;
+struct client *clients;
 
 /* the port details */
 struct port_info *ports;
@@ -72,7 +72,8 @@ init_mbuf_pools(void)
 		num_mbufs_server + num_mbufs_client + num_mbufs_mp_cache;
 
 	/* don't pass single-producer/single-consumer flags to mbuf create as it
-	 * seems faster to use a cache instead */
+	 * seems faster to use a cache instead
+	 */
 	printf("Creating mbuf pool '%s' [%u mbufs] ...\n",
 			PKTMBUF_POOL_NAME, num_mbufs);
 	pktmbuf_pool = rte_pktmbuf_pool_create(PKTMBUF_POOL_NAME, num_mbufs,
@@ -108,9 +109,11 @@ init_port(uint16_t port_num)
 	fflush(stdout);
 
 	/* Standard DPDK port initialisation - config port, then set up
-	 * rx and tx rings */
-	if ((retval = rte_eth_dev_configure(port_num, rx_rings, tx_rings,
-		&port_conf)) != 0)
+	 * rx and tx rings
+	 */
+	retval = rte_eth_dev_configure(port_num, rx_rings, tx_rings,
+				       &port_conf);
+	if (retval != 0)
 		return retval;
 
 	retval = rte_eth_dev_adjust_nb_rx_tx_desc(port_num, &rx_ring_size,
@@ -122,22 +125,25 @@ init_port(uint16_t port_num)
 		retval = rte_eth_rx_queue_setup(port_num, q, rx_ring_size,
 				rte_eth_dev_socket_id(port_num),
 				NULL, pktmbuf_pool);
-		if (retval < 0) return retval;
+		if (retval < 0)
+			return retval;
 	}
 
-	for ( q = 0; q < tx_rings; q ++ ) {
+	for (q = 0; q < tx_rings; q++) {
 		retval = rte_eth_tx_queue_setup(port_num, q, tx_ring_size,
 				rte_eth_dev_socket_id(port_num),
 				NULL);
-		if (retval < 0) return retval;
+		if (retval < 0)
+			return retval;
 	}
 
 	rte_eth_promiscuous_enable(port_num);
 
 	retval  = rte_eth_dev_start(port_num);
-	if (retval < 0) return retval;
+	if (retval < 0)
+		return retval;
 
-	printf( "done: \n");
+	printf("done:\n");
 
 	return 0;
 }
@@ -150,15 +156,15 @@ init_port(uint16_t port_num)
 static int
 init_shm_rings(void)
 {
-	unsigned i;
-	unsigned socket_id;
-	const char * q_name;
-	const unsigned ringsize = CLIENT_QUEUE_RINGSIZE;
+	unsigned int i, socket_id;
+	const char *q_name;
+	const unsigned int ringsize = CLIENT_QUEUE_RINGSIZE;
 
 	clients = rte_malloc("client details",
 		sizeof(*clients) * num_clients, 0);
 	if (clients == NULL)
-		rte_exit(EXIT_FAILURE, "Cannot allocate memory for client program details\n");
+		rte_exit(EXIT_FAILURE,
+			 "Cannot allocate memory for client program details\n");
 
 	for (i = 0; i < num_clients; i++) {
 		/* Create an RX queue for each client */
@@ -166,9 +172,10 @@ init_shm_rings(void)
 		q_name = get_rx_queue_name(i);
 		clients[i].rx_q = rte_ring_create(q_name,
 				ringsize, socket_id,
-				RING_F_SP_ENQ | RING_F_SC_DEQ ); /* single prod, single cons */
+				RING_F_SP_ENQ | RING_F_SC_DEQ);
 		if (clients[i].rx_q == NULL)
-			rte_exit(EXIT_FAILURE, "Cannot create rx ring queue for client %u\n", i);
+			rte_exit(EXIT_FAILURE,
+				 "Cannot create rx ring queue for client %u\n", i);
 	}
 	return 0;
 }
@@ -195,11 +202,11 @@ check_all_ports_link_status(uint16_t port_num, uint32_t port_mask)
 			/* print link status if flag set */
 			if (print_flag == 1) {
 				if (link.link_status)
-					printf("Port %d Link Up - speed %u "
-						"Mbps - %s\n", ports->id[portid],
-						(unsigned)link.link_speed,
-				(link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
-					("full-duplex") : ("half-duplex\n"));
+					printf("Port %d Link Up - speed %u Mbps - %s-duplex\n",
+					       ports->id[portid],
+					       link.link_speed,
+					       (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
+					       "full" : "half");
 				else
 					printf("Port %d Link Down\n",
 						(uint8_t)ports->id[portid]);
@@ -251,7 +258,8 @@ init(int argc, char *argv[])
 	mz = rte_memzone_reserve(MZ_PORT_INFO, sizeof(*ports),
 				rte_socket_id(), NO_FLAGS);
 	if (mz == NULL)
-		rte_exit(EXIT_FAILURE, "Cannot reserve memory zone for port information\n");
+		rte_exit(EXIT_FAILURE,
+			 "Cannot reserve memory zone for port information\n");
 	memset(mz->addr, 0, sizeof(*ports));
 	ports = mz->addr;
 
@@ -269,8 +277,8 @@ init(int argc, char *argv[])
 	for (i = 0; i < ports->num_ports; i++) {
 		retval = init_port(ports->id[i]);
 		if (retval != 0)
-			rte_exit(EXIT_FAILURE, "Cannot initialise port %u\n",
-					(unsigned)i);
+			rte_exit(EXIT_FAILURE,
+				 "Cannot initialise port %u\n", i);
 	}
 
 	check_all_ports_link_status(ports->num_ports, (~0x0));
diff --git a/examples/multi_process/client_server_mp/mp_server/main.c b/examples/multi_process/client_server_mp/mp_server/main.c
index 0150533700f0..bfec0bef3a71 100644
--- a/examples/multi_process/client_server_mp/mp_server/main.c
+++ b/examples/multi_process/client_server_mp/mp_server/main.c
@@ -64,8 +64,9 @@ get_printable_mac_addr(uint16_t port)
 
 	if (unlikely(port >= RTE_MAX_ETHPORTS))
 		return err_address;
-	if (unlikely(addresses[port][0]=='\0')){
+	if (unlikely(addresses[port][0] == '\0')) {
 		struct rte_ether_addr mac;
+
 		rte_eth_macaddr_get(port, &mac);
 		snprintf(addresses[port], sizeof(addresses[port]),
 				"%02x:%02x:%02x:%02x:%02x:%02x\n",
@@ -85,9 +86,9 @@ get_printable_mac_addr(uint16_t port)
 static void
 do_stats_display(void)
 {
-	unsigned i, j;
+	unsigned int i, j;
 	const char clr[] = { 27, '[', '2', 'J', '\0' };
-	const char topLeft[] = { 27, '[', '1', ';', '1', 'H','\0' };
+	const char topLeft[] = { 27, '[', '1', ';', '1', 'H', '\0' };
 	uint64_t port_tx[RTE_MAX_ETHPORTS], port_tx_drop[RTE_MAX_ETHPORTS];
 	uint64_t client_tx[MAX_CLIENTS], client_tx_drop[MAX_CLIENTS];
 
@@ -97,12 +98,14 @@ do_stats_display(void)
 	memset(client_tx, 0, sizeof(client_tx));
 	memset(client_tx_drop, 0, sizeof(client_tx_drop));
 
-	for (i = 0; i < num_clients; i++){
+	for (i = 0; i < num_clients; i++) {
 		const volatile struct tx_stats *tx = &ports->tx_stats[i];
-		for (j = 0; j < ports->num_ports; j++){
+
+		for (j = 0; j < ports->num_ports; j++) {
 			/* assign to local variables here, save re-reading volatile vars */
 			const uint64_t tx_val = tx->tx[ports->id[j]];
 			const uint64_t drop_val = tx->tx_drop[ports->id[j]];
+
 			port_tx[j] += tx_val;
 			port_tx_drop[j] += drop_val;
 			client_tx[i] += tx_val;
@@ -116,21 +119,21 @@ do_stats_display(void)
 	printf("PORTS\n");
 	printf("-----\n");
 	for (i = 0; i < ports->num_ports; i++)
-		printf("Port %u: '%s'\t", (unsigned)ports->id[i],
-				get_printable_mac_addr(ports->id[i]));
+		printf("Port %u: '%s'\t", ports->id[i],
+		       get_printable_mac_addr(ports->id[i]));
 	printf("\n\n");
-	for (i = 0; i < ports->num_ports; i++){
-		printf("Port %u - rx: %9"PRIu64"\t"
-				"tx: %9"PRIu64"\n",
-				(unsigned)ports->id[i], ports->rx_stats.rx[i],
+	for (i = 0; i < ports->num_ports; i++) {
+		printf("Port %u - rx: %9"PRIu64"\ttx: %9"PRIu64"\n",
+				ports->id[i], ports->rx_stats.rx[i],
 				port_tx[i]);
 	}
 
 	printf("\nCLIENTS\n");
 	printf("-------\n");
-	for (i = 0; i < num_clients; i++){
+	for (i = 0; i < num_clients; i++) {
 		const unsigned long long rx = clients[i].stats.rx;
 		const unsigned long long rx_drop = clients[i].stats.rx_drop;
+
 		printf("Client %2u - rx: %9llu, rx_drop: %9llu\n"
 				"            tx: %9"PRIu64", tx_drop: %9"PRIu64"\n",
 				i, rx, rx_drop, client_tx[i], client_tx_drop[i]);
@@ -153,7 +156,8 @@ sleep_lcore(__attribute__((unused)) void *dummy)
 
 	/* Only one core should display stats */
 	if (rte_atomic32_test_and_set(&display_stats)) {
-		const unsigned sleeptime = 1;
+		const unsigned int sleeptime = 1;
+
 		printf("Core %u displaying statistics\n", rte_lcore_id());
 
 		/* Longer initial pause so above printf is seen */
@@ -173,7 +177,7 @@ sleep_lcore(__attribute__((unused)) void *dummy)
 static void
 clear_stats(void)
 {
-	unsigned i;
+	unsigned int i;
 
 	for (i = 0; i < num_clients; i++)
 		clients[i].stats.rx = clients[i].stats.rx_drop = 0;
@@ -194,12 +198,11 @@ flush_rx_queue(uint16_t client)
 
 	cl = &clients[client];
 	if (rte_ring_enqueue_bulk(cl->rx_q, (void **)cl_rx_buf[client].buffer,
-			cl_rx_buf[client].count, NULL) == 0){
+			cl_rx_buf[client].count, NULL) == 0) {
 		for (j = 0; j < cl_rx_buf[client].count; j++)
 			rte_pktmbuf_free(cl_rx_buf[client].buffer[j]);
 		cl->stats.rx_drop += cl_rx_buf[client].count;
-	}
-	else
+	} else
 		cl->stats.rx += cl_rx_buf[client].count;
 
 	cl_rx_buf[client].count = 0;
@@ -243,14 +246,14 @@ process_packets(uint32_t port_num __rte_unused,
 static void
 do_packet_forwarding(void)
 {
-	unsigned port_num = 0; /* indexes the port[] array */
+	unsigned int port_num = 0; /* indexes the port[] array */
 
 	for (;;) {
 		struct rte_mbuf *buf[PACKET_READ_SIZE];
 		uint16_t rx_count;
 
 		/* read a port */
-		rx_count = rte_eth_rx_burst(ports->id[port_num], 0, \
+		rx_count = rte_eth_rx_burst(ports->id[port_num], 0,
 				buf, PACKET_READ_SIZE);
 		ports->rx_stats.rx[port_num] += rx_count;
 
@@ -281,8 +284,9 @@ int
 main(int argc, char *argv[])
 {
 	signal(SIGINT, signal_handler);
+
 	/* initialise the system */
-	if (init(argc, argv) < 0 )
+	if (init(argc, argv) < 0)
 		return -1;
 	RTE_LOG(INFO, APP, "Finished Process Init.\n");
 
-- 
2.20.1

[dpdk-dev] [PATCH v4 4/4] examples/multi_process/client_server_mp/mp_server: use ether format address

[Stephen Hemminger]

No need to use snprintf to print ethernet address.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 .../client_server_mp/mp_server/main.c         | 32 ++++++-------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_server/main.c b/examples/multi_process/client_server_mp/mp_server/main.c
index bfec0bef3a71..d1339d4be64c 100644
--- a/examples/multi_process/client_server_mp/mp_server/main.c
+++ b/examples/multi_process/client_server_mp/mp_server/main.c
@@ -56,26 +56,6 @@ struct client_rx_buf {
 /* One buffer per client rx queue - dynamically allocate array */
 static struct client_rx_buf *cl_rx_buf;
 
-static const char *
-get_printable_mac_addr(uint16_t port)
-{
-	static const char err_address[] = "00:00:00:00:00:00";
-	static char addresses[RTE_MAX_ETHPORTS][sizeof(err_address)];
-
-	if (unlikely(port >= RTE_MAX_ETHPORTS))
-		return err_address;
-	if (unlikely(addresses[port][0] == '\0')) {
-		struct rte_ether_addr mac;
-
-		rte_eth_macaddr_get(port, &mac);
-		snprintf(addresses[port], sizeof(addresses[port]),
-				"%02x:%02x:%02x:%02x:%02x:%02x\n",
-				mac.addr_bytes[0], mac.addr_bytes[1], mac.addr_bytes[2],
-				mac.addr_bytes[3], mac.addr_bytes[4], mac.addr_bytes[5]);
-	}
-	return addresses[port];
-}
-
 /*
  * This function displays the recorded statistics for each port
  * and for each client. It uses ANSI terminal codes to clear
@@ -118,9 +98,15 @@ do_stats_display(void)
 
 	printf("PORTS\n");
 	printf("-----\n");
-	for (i = 0; i < ports->num_ports; i++)
-		printf("Port %u: '%s'\t", ports->id[i],
-		       get_printable_mac_addr(ports->id[i]));
+	for (i = 0; i < ports->num_ports; i++) {
+		struct rte_ether_addr mac = { };
+		char buf[32];
+
+		rte_eth_macaddr_get(ports->id[i], &mac);
+		rte_ether_format_addr(buf, sizeof(buf), &mac);
+		printf("Port %u: '%s'\t", ports->id[i], buf);
+	}
+
 	printf("\n\n");
 	for (i = 0; i < ports->num_ports; i++) {
 		printf("Port %u - rx: %9"PRIu64"\ttx: %9"PRIu64"\n",
-- 
2.20.1

Re: [dpdk-dev] [PATCH v4 1/4] examples/multi_process/client_server_mp: check port validity

[Matan Azrad]

Hi Stephen

From:  Stephen Hemminger
> From: Stephen Hemminger <sthemmin@microsoft.com>
> 
> The mp_server incorrectly allows a port mask that included hidden ports and
> which later caused either lost packets or failed initialization.
> 
> This fixes explicitly checking that each bit in portmask is a valid port before
> using it.
> 
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>  .../client_server_mp/mp_server/args.c         | 46 +++++++++++++------
>  .../client_server_mp/mp_server/args.h         |  2 +-
>  .../client_server_mp/mp_server/init.c         |  7 +--
>  3 files changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/examples/multi_process/client_server_mp/mp_server/args.c
> b/examples/multi_process/client_server_mp/mp_server/args.c
> index b0d8d7665c85..c1ab12ad00d1 100644
> --- a/examples/multi_process/client_server_mp/mp_server/args.c
> +++ b/examples/multi_process/client_server_mp/mp_server/args.c
> @@ -10,6 +10,7 @@
>  #include <errno.h>
> 
>  #include <rte_memory.h>
> +#include <rte_ethdev.h>
>  #include <rte_string_fns.h>
> 
>  #include "common.h"
> @@ -34,6 +35,22 @@ usage(void)
>  	    , progname);
>  }
> 
> +/**
> + * Check if port is present in the system
> + * It maybe owned by a device and should not be used.
> + */
> +static int
> +port_is_present(uint16_t portid)
> +{
> +	uint16_t id;
> +
> +	RTE_ETH_FOREACH_DEV(id) {
> +		if (id == portid)
> +			return 1;
> +	}
> +	return 0;
> +}
> +
>  /**
>   * The ports to be used by the application are passed in
>   * the form of a bitmask. This function parses the bitmask @@ -41,31 +58,32
> @@ usage(void)
>   * array variable
>   */
>  static int
> -parse_portmask(uint8_t max_ports, const char *portmask)
> +parse_portmask(const char *portmask)
>  {
>  	char *end = NULL;
>  	unsigned long pm;
> -	uint16_t count = 0;
> +	uint16_t count;
> 
>  	if (portmask == NULL || *portmask == '\0')
>  		return -1;
> 
>  	/* convert parameter to a number and verify */
>  	pm = strtoul(portmask, &end, 16);
> -	if (end == NULL || *end != '\0' || pm == 0)
> +	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
>  		return -1;
> 
>  	/* loop through bits of the mask and mark ports */
> -	while (pm != 0){
> -		if (pm & 0x01){ /* bit is set in mask, use port */
> -			if (count >= max_ports)
> -				printf("WARNING: requested port %u not
> present"
> -				" - ignoring\n", (unsigned)count);
> -			else
> -			    ports->id[ports->num_ports++] = count;
> +	for (count = 0; pm != 0; pm >>= 1, ++count) {
> +		if ((pm & 0x1) == 0)
> +			continue;
> +
> +		if (!port_is_present(count)) {
> +			printf("WARNING: requested port %u not present -
> ignoring\n",
> +				count);
> +			continue;
>  		}
> -		pm = (pm >> 1);
> -		count++;
> +
> +		ports->id[ports->num_ports++] = count;
>  	}

Why not just to do something like:

RTE_ETH_FOREACH_DEV(id) {
	If (pm & (1 << id)) {
		pm &= ~(1 << id) 
		put in list
	}
}
	If (pm) 
		Warning



>  	return 0;
> @@ -99,7 +117,7 @@ parse_num_clients(const char *clients)
>   * on error.
>   */
>  int
> -parse_app_args(uint16_t max_ports, int argc, char *argv[])
> +parse_app_args(int argc, char *argv[])
>  {
>  	int option_index, opt;
>  	char **argvopt = argv;
> @@ -112,7 +130,7 @@ parse_app_args(uint16_t max_ports, int argc, char
> *argv[])
>  		&option_index)) != EOF){
>  		switch (opt){
>  			case 'p':
> -				if (parse_portmask(max_ports, optarg) != 0){
> +				if (parse_portmask(optarg) != 0) {
>  					usage();
>  					return -1;
>  				}
> diff --git a/examples/multi_process/client_server_mp/mp_server/args.h
> b/examples/multi_process/client_server_mp/mp_server/args.h
> index 79c190a33a37..52c8cc86e6f0 100644
> --- a/examples/multi_process/client_server_mp/mp_server/args.h
> +++ b/examples/multi_process/client_server_mp/mp_server/args.h
> @@ -5,6 +5,6 @@
>  #ifndef _ARGS_H_
>  #define _ARGS_H_
> 
> -int parse_app_args(uint16_t max_ports, int argc, char *argv[]);
> +int parse_app_args(int argc, char *argv[]);
> 
>  #endif /* ifndef _ARGS_H_ */
> diff --git a/examples/multi_process/client_server_mp/mp_server/init.c
> b/examples/multi_process/client_server_mp/mp_server/init.c
> index 3af5dc6994bf..1b0569937b51 100644
> --- a/examples/multi_process/client_server_mp/mp_server/init.c
> +++ b/examples/multi_process/client_server_mp/mp_server/init.c
> @@ -238,7 +238,7 @@ init(int argc, char *argv[])  {
>  	int retval;
>  	const struct rte_memzone *mz;
> -	uint16_t i, total_ports;
> +	uint16_t i;
> 
>  	/* init EAL, parsing EAL args */
>  	retval = rte_eal_init(argc, argv);
> @@ -247,9 +247,6 @@ init(int argc, char *argv[])
>  	argc -= retval;
>  	argv += retval;
> 
> -	/* get total number of ports */
> -	total_ports = rte_eth_dev_count_total();
> -
>  	/* set up array for port data */
>  	mz = rte_memzone_reserve(MZ_PORT_INFO, sizeof(*ports),
>  				rte_socket_id(), NO_FLAGS);
> @@ -259,7 +256,7 @@ init(int argc, char *argv[])
>  	ports = mz->addr;
> 
>  	/* parse additional, application arguments */
> -	retval = parse_app_args(total_ports, argc, argv);
> +	retval = parse_app_args(argc, argv);
>  	if (retval != 0)
>  		return -1;
> 
> --
> 2.20.1

Re: [dpdk-dev] [PATCH v4 1/4] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

On Tue, 30 Jul 2019 09:21:02 +0000
Matan Azrad <matan@mellanox.com> wrote:

> Hi Stephen
> 
> From:  Stephen Hemminger
> > From: Stephen Hemminger <sthemmin@microsoft.com>
> > 
> > The mp_server incorrectly allows a port mask that included hidden ports and
> > which later caused either lost packets or failed initialization.
> > 
> > This fixes explicitly checking that each bit in portmask is a valid port before
> > using it.
> > 
> > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > ---
> >  .../client_server_mp/mp_server/args.c         | 46 +++++++++++++------
> >  .../client_server_mp/mp_server/args.h         |  2 +-
> >  .../client_server_mp/mp_server/init.c         |  7 +--
> >  3 files changed, 35 insertions(+), 20 deletions(-)
> > 
> > diff --git a/examples/multi_process/client_server_mp/mp_server/args.c
> > b/examples/multi_process/client_server_mp/mp_server/args.c
> > index b0d8d7665c85..c1ab12ad00d1 100644
> > --- a/examples/multi_process/client_server_mp/mp_server/args.c
> > +++ b/examples/multi_process/client_server_mp/mp_server/args.c
> > @@ -10,6 +10,7 @@
> >  #include <errno.h>
> > 
> >  #include <rte_memory.h>
> > +#include <rte_ethdev.h>
> >  #include <rte_string_fns.h>
> > 
> >  #include "common.h"
> > @@ -34,6 +35,22 @@ usage(void)
> >  	    , progname);
> >  }
> > 
> > +/**
> > + * Check if port is present in the system
> > + * It maybe owned by a device and should not be used.
> > + */
> > +static int
> > +port_is_present(uint16_t portid)
> > +{
> > +	uint16_t id;
> > +
> > +	RTE_ETH_FOREACH_DEV(id) {
> > +		if (id == portid)
> > +			return 1;
> > +	}
> > +	return 0;
> > +}
> > +
> >  /**
> >   * The ports to be used by the application are passed in
> >   * the form of a bitmask. This function parses the bitmask @@ -41,31 +58,32
> > @@ usage(void)
> >   * array variable
> >   */
> >  static int
> > -parse_portmask(uint8_t max_ports, const char *portmask)
> > +parse_portmask(const char *portmask)
> >  {
> >  	char *end = NULL;
> >  	unsigned long pm;
> > -	uint16_t count = 0;
> > +	uint16_t count;
> > 
> >  	if (portmask == NULL || *portmask == '\0')
> >  		return -1;
> > 
> >  	/* convert parameter to a number and verify */
> >  	pm = strtoul(portmask, &end, 16);
> > -	if (end == NULL || *end != '\0' || pm == 0)
> > +	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
> >  		return -1;
> > 
> >  	/* loop through bits of the mask and mark ports */
> > -	while (pm != 0){
> > -		if (pm & 0x01){ /* bit is set in mask, use port */
> > -			if (count >= max_ports)
> > -				printf("WARNING: requested port %u not
> > present"
> > -				" - ignoring\n", (unsigned)count);
> > -			else
> > -			    ports->id[ports->num_ports++] = count;
> > +	for (count = 0; pm != 0; pm >>= 1, ++count) {
> > +		if ((pm & 0x1) == 0)
> > +			continue;
> > +
> > +		if (!port_is_present(count)) {
> > +			printf("WARNING: requested port %u not present -
> > ignoring\n",
> > +				count);
> > +			continue;
> >  		}
> > -		pm = (pm >> 1);
> > -		count++;
> > +
> > +		ports->id[ports->num_ports++] = count;
> >  	}  
> 
> Why not just to do something like:
> 
> RTE_ETH_FOREACH_DEV(id) {
> 	If (pm & (1 << id)) {
> 		pm &= ~(1 << id) 
> 		put in list
> 	}
> }
> 	If (pm) 
> 		Warning
> 

No real reason, was just trying to keep existing logic flow

Re: [dpdk-dev] [PATCH v4 1/4] examples/multi_process/client_server_mp: check port validity

[Matan Azrad]

Hi

From: Stephen Hemminger
> On Tue, 30 Jul 2019 09:21:02 +0000
> Matan Azrad <matan@mellanox.com> wrote:
> 
> > Hi Stephen
> >
> > From:  Stephen Hemminger
> > > From: Stephen Hemminger <sthemmin@microsoft.com>
> > >
> > > The mp_server incorrectly allows a port mask that included hidden
> > > ports and which later caused either lost packets or failed initialization.
> > >
> > > This fixes explicitly checking that each bit in portmask is a valid
> > > port before using it.
> > >
> > > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > > ---
> > >  .../client_server_mp/mp_server/args.c         | 46 +++++++++++++------
> > >  .../client_server_mp/mp_server/args.h         |  2 +-
> > >  .../client_server_mp/mp_server/init.c         |  7 +--
> > >  3 files changed, 35 insertions(+), 20 deletions(-)
> > >
> > > diff --git
> > > a/examples/multi_process/client_server_mp/mp_server/args.c
> > > b/examples/multi_process/client_server_mp/mp_server/args.c
> > > index b0d8d7665c85..c1ab12ad00d1 100644
> > > --- a/examples/multi_process/client_server_mp/mp_server/args.c
> > > +++ b/examples/multi_process/client_server_mp/mp_server/args.c
> > > @@ -10,6 +10,7 @@
> > >  #include <errno.h>
> > >
> > >  #include <rte_memory.h>
> > > +#include <rte_ethdev.h>
> > >  #include <rte_string_fns.h>
> > >
> > >  #include "common.h"
> > > @@ -34,6 +35,22 @@ usage(void)
> > >  	    , progname);
> > >  }
> > >
> > > +/**
> > > + * Check if port is present in the system
> > > + * It maybe owned by a device and should not be used.
> > > + */
> > > +static int
> > > +port_is_present(uint16_t portid)
> > > +{
> > > +	uint16_t id;
> > > +
> > > +	RTE_ETH_FOREACH_DEV(id) {
> > > +		if (id == portid)
> > > +			return 1;
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * The ports to be used by the application are passed in
> > >   * the form of a bitmask. This function parses the bitmask @@
> > > -41,31 +58,32 @@ usage(void)
> > >   * array variable
> > >   */
> > >  static int
> > > -parse_portmask(uint8_t max_ports, const char *portmask)
> > > +parse_portmask(const char *portmask)
> > >  {
> > >  	char *end = NULL;
> > >  	unsigned long pm;
> > > -	uint16_t count = 0;
> > > +	uint16_t count;
> > >
> > >  	if (portmask == NULL || *portmask == '\0')
> > >  		return -1;
> > >
> > >  	/* convert parameter to a number and verify */
> > >  	pm = strtoul(portmask, &end, 16);
> > > -	if (end == NULL || *end != '\0' || pm == 0)
> > > +	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
> > >  		return -1;
> > >
> > >  	/* loop through bits of the mask and mark ports */
> > > -	while (pm != 0){
> > > -		if (pm & 0x01){ /* bit is set in mask, use port */
> > > -			if (count >= max_ports)
> > > -				printf("WARNING: requested port %u not
> > > present"
> > > -				" - ignoring\n", (unsigned)count);
> > > -			else
> > > -			    ports->id[ports->num_ports++] = count;
> > > +	for (count = 0; pm != 0; pm >>= 1, ++count) {
> > > +		if ((pm & 0x1) == 0)
> > > +			continue;
> > > +
> > > +		if (!port_is_present(count)) {
> > > +			printf("WARNING: requested port %u not present -
> > > ignoring\n",
> > > +				count);
> > > +			continue;
> > >  		}
> > > -		pm = (pm >> 1);
> > > -		count++;
> > > +
> > > +		ports->id[ports->num_ports++] = count;
> > >  	}
> >
> > Why not just to do something like:
> >
> > RTE_ETH_FOREACH_DEV(id) {
> > 	If (pm & (1 << id)) {
> > 		pm &= ~(1 << id)
> > 		put in list
> > 	}
> > }
> > 	If (pm)
> > 		Warning
> >
> 
> No real reason, was just trying to keep existing logic flow

This is an example for the users\developers, and you on it,
I think this is a good chance to show how to do it in the correct way - O(n) instead O(n^2).

Don't you think so?
 
I suggest to adjust. 

Re: [dpdk-dev] [PATCH v4 1/4] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

On Tue, 30 Jul 2019 16:39:52 +0000
Matan Azrad <matan@mellanox.com> wrote:

> Hi
> 
> From: Stephen Hemminger
> > On Tue, 30 Jul 2019 09:21:02 +0000
> > Matan Azrad <matan@mellanox.com> wrote:
> >   
> > > Hi Stephen
> > >
> > > From:  Stephen Hemminger  
> > > > From: Stephen Hemminger <sthemmin@microsoft.com>
> > > >
> > > > The mp_server incorrectly allows a port mask that included hidden
> > > > ports and which later caused either lost packets or failed initialization.
> > > >
> > > > This fixes explicitly checking that each bit in portmask is a valid
> > > > port before using it.
> > > >
> > > > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > > > ---
> > > >  .../client_server_mp/mp_server/args.c         | 46 +++++++++++++------
> > > >  .../client_server_mp/mp_server/args.h         |  2 +-
> > > >  .../client_server_mp/mp_server/init.c         |  7 +--
> > > >  3 files changed, 35 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git
> > > > a/examples/multi_process/client_server_mp/mp_server/args.c
> > > > b/examples/multi_process/client_server_mp/mp_server/args.c
> > > > index b0d8d7665c85..c1ab12ad00d1 100644
> > > > --- a/examples/multi_process/client_server_mp/mp_server/args.c
> > > > +++ b/examples/multi_process/client_server_mp/mp_server/args.c
> > > > @@ -10,6 +10,7 @@
> > > >  #include <errno.h>
> > > >
> > > >  #include <rte_memory.h>
> > > > +#include <rte_ethdev.h>
> > > >  #include <rte_string_fns.h>
> > > >
> > > >  #include "common.h"
> > > > @@ -34,6 +35,22 @@ usage(void)
> > > >  	    , progname);
> > > >  }
> > > >
> > > > +/**
> > > > + * Check if port is present in the system
> > > > + * It maybe owned by a device and should not be used.
> > > > + */
> > > > +static int
> > > > +port_is_present(uint16_t portid)
> > > > +{
> > > > +	uint16_t id;
> > > > +
> > > > +	RTE_ETH_FOREACH_DEV(id) {
> > > > +		if (id == portid)
> > > > +			return 1;
> > > > +	}
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  /**
> > > >   * The ports to be used by the application are passed in
> > > >   * the form of a bitmask. This function parses the bitmask @@
> > > > -41,31 +58,32 @@ usage(void)
> > > >   * array variable
> > > >   */
> > > >  static int
> > > > -parse_portmask(uint8_t max_ports, const char *portmask)
> > > > +parse_portmask(const char *portmask)
> > > >  {
> > > >  	char *end = NULL;
> > > >  	unsigned long pm;
> > > > -	uint16_t count = 0;
> > > > +	uint16_t count;
> > > >
> > > >  	if (portmask == NULL || *portmask == '\0')
> > > >  		return -1;
> > > >
> > > >  	/* convert parameter to a number and verify */
> > > >  	pm = strtoul(portmask, &end, 16);
> > > > -	if (end == NULL || *end != '\0' || pm == 0)
> > > > +	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
> > > >  		return -1;
> > > >
> > > >  	/* loop through bits of the mask and mark ports */
> > > > -	while (pm != 0){
> > > > -		if (pm & 0x01){ /* bit is set in mask, use port */
> > > > -			if (count >= max_ports)
> > > > -				printf("WARNING: requested port %u not
> > > > present"
> > > > -				" - ignoring\n", (unsigned)count);
> > > > -			else
> > > > -			    ports->id[ports->num_ports++] = count;
> > > > +	for (count = 0; pm != 0; pm >>= 1, ++count) {
> > > > +		if ((pm & 0x1) == 0)
> > > > +			continue;
> > > > +
> > > > +		if (!port_is_present(count)) {
> > > > +			printf("WARNING: requested port %u not present -
> > > > ignoring\n",
> > > > +				count);
> > > > +			continue;
> > > >  		}
> > > > -		pm = (pm >> 1);
> > > > -		count++;
> > > > +
> > > > +		ports->id[ports->num_ports++] = count;
> > > >  	}  
> > >
> > > Why not just to do something like:
> > >
> > > RTE_ETH_FOREACH_DEV(id) {
> > > 	If (pm & (1 << id)) {
> > > 		pm &= ~(1 << id)
> > > 		put in list
> > > 	}
> > > }
> > > 	If (pm)
> > > 		Warning
> > >  
> > 
> > No real reason, was just trying to keep existing logic flow  
> 
> This is an example for the users\developers, and you on it,
> I think this is a good chance to show how to do it in the correct way - O(n) instead O(n^2).
> 
> Don't you think so?
>  
> I suggest to adjust. 
> 

This is a toy example short loops, it doesn't matter.
It is important to catch case where portmask contains hidden ports.

[dpdk-dev] [PATCH v5 0/4] examples/client_server_mp: port id fixes

[Stephen Hemminger]

The client_server_mp application does not work correctly
with failsafe or other devices using port ownership.

v5 - change logic in server_mp for evaluating port mask

v4 - fix checkpatch warning
     add patches to fix style issues and use ether format addr

v3 - merge both patches in one series
     use alternative algorithm to check port ownership (N^2)
     because reviewer didn't like direct check.

Stephen Hemminger (4):
  examples/multi_process/client_server_mp: check port validity
  examples/multi_process/client_server_mp - fix crash in mp_client with
    sparse ports
  examples/multi_process/client_server_mp/mp_server: fix style
  examples/multi_process/client_server_mp/mp_server: use ether format
    address

 .../client_server_mp/mp_client/client.c       | 18 ++--
 .../client_server_mp/mp_server/args.c         | 70 ++++++++-------
 .../client_server_mp/mp_server/args.h         |  2 +-
 .../client_server_mp/mp_server/init.c         | 86 ++++++++++---------
 .../client_server_mp/mp_server/main.c         | 68 +++++++--------
 5 files changed, 123 insertions(+), 121 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

From: Stephen Hemminger <sthemmin@microsoft.com>

The mp_server incorrectly allows a port mask that included hidden
ports and which later caused either lost packets or failed initialization.

This fixes explicitly checking that each bit in portmask is a
valid port before using it.

Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 .../client_server_mp/mp_server/args.c         | 35 ++++++++++---------
 .../client_server_mp/mp_server/args.h         |  2 +-
 .../client_server_mp/mp_server/init.c         |  7 ++--
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_server/args.c b/examples/multi_process/client_server_mp/mp_server/args.c
index b0d8d7665c85..fdc008b3d677 100644
--- a/examples/multi_process/client_server_mp/mp_server/args.c
+++ b/examples/multi_process/client_server_mp/mp_server/args.c
@@ -10,6 +10,7 @@
 #include <errno.h>
 
 #include <rte_memory.h>
+#include <rte_ethdev.h>
 #include <rte_string_fns.h>
 
 #include "common.h"
@@ -41,31 +42,33 @@ usage(void)
  * array variable
  */
 static int
-parse_portmask(uint8_t max_ports, const char *portmask)
+parse_portmask(const char *portmask)
 {
 	char *end = NULL;
 	unsigned long pm;
-	uint16_t count = 0;
+	uint16_t id;
 
 	if (portmask == NULL || *portmask == '\0')
 		return -1;
 
 	/* convert parameter to a number and verify */
 	pm = strtoul(portmask, &end, 16);
-	if (end == NULL || *end != '\0' || pm == 0)
+	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
 		return -1;
 
-	/* loop through bits of the mask and mark ports */
-	while (pm != 0){
-		if (pm & 0x01){ /* bit is set in mask, use port */
-			if (count >= max_ports)
-				printf("WARNING: requested port %u not present"
-				" - ignoring\n", (unsigned)count);
-			else
-			    ports->id[ports->num_ports++] = count;
-		}
-		pm = (pm >> 1);
-		count++;
+	RTE_ETH_FOREACH_DEV(id) {
+		unsigned long msk = 1u << id;
+
+		if ((pm & msk) == 0)
+			continue;
+
+		pm &= ~msk;
+		ports->id[ports->num_ports++] = id;
+	}
+
+	if (pm != 0) {
+		printf("WARNING: leftover ports in mask %#lx - ignoring\n",
+		       pm);
 	}
 
 	return 0;
@@ -99,7 +102,7 @@ parse_num_clients(const char *clients)
  * on error.
  */
 int
-parse_app_args(uint16_t max_ports, int argc, char *argv[])
+parse_app_args(int argc, char *argv[])
 {
 	int option_index, opt;
 	char **argvopt = argv;
@@ -112,7 +115,7 @@ parse_app_args(uint16_t max_ports, int argc, char *argv[])
 		&option_index)) != EOF){
 		switch (opt){
 			case 'p':
-				if (parse_portmask(max_ports, optarg) != 0){
+				if (parse_portmask(optarg) != 0) {
 					usage();
 					return -1;
 				}
diff --git a/examples/multi_process/client_server_mp/mp_server/args.h b/examples/multi_process/client_server_mp/mp_server/args.h
index 79c190a33a37..52c8cc86e6f0 100644
--- a/examples/multi_process/client_server_mp/mp_server/args.h
+++ b/examples/multi_process/client_server_mp/mp_server/args.h
@@ -5,6 +5,6 @@
 #ifndef _ARGS_H_
 #define _ARGS_H_
 
-int parse_app_args(uint16_t max_ports, int argc, char *argv[]);
+int parse_app_args(int argc, char *argv[]);
 
 #endif /* ifndef _ARGS_H_ */
diff --git a/examples/multi_process/client_server_mp/mp_server/init.c b/examples/multi_process/client_server_mp/mp_server/init.c
index 3af5dc6994bf..1b0569937b51 100644
--- a/examples/multi_process/client_server_mp/mp_server/init.c
+++ b/examples/multi_process/client_server_mp/mp_server/init.c
@@ -238,7 +238,7 @@ init(int argc, char *argv[])
 {
 	int retval;
 	const struct rte_memzone *mz;
-	uint16_t i, total_ports;
+	uint16_t i;
 
 	/* init EAL, parsing EAL args */
 	retval = rte_eal_init(argc, argv);
@@ -247,9 +247,6 @@ init(int argc, char *argv[])
 	argc -= retval;
 	argv += retval;
 
-	/* get total number of ports */
-	total_ports = rte_eth_dev_count_total();
-
 	/* set up array for port data */
 	mz = rte_memzone_reserve(MZ_PORT_INFO, sizeof(*ports),
 				rte_socket_id(), NO_FLAGS);
@@ -259,7 +256,7 @@ init(int argc, char *argv[])
 	ports = mz->addr;
 
 	/* parse additional, application arguments */
-	retval = parse_app_args(total_ports, argc, argv);
+	retval = parse_app_args(argc, argv);
 	if (retval != 0)
 		return -1;
 
-- 
2.20.1

[dpdk-dev] [PATCH v5 2/4] examples/multi_process/client_server_mp - fix crash in mp_client with sparse ports

[Stephen Hemminger]

From: Stephen Hemminger <sthemmin@microsoft.com>

The mp_client crashes if run on Azure or any system where ethdev
ports are owned. In that case, the tx_buffer and tx_stats for the
real port were initialized correctly, but the wrong port was used.

For example if the server has Ports 3 and 5. Then calling
rte_eth_tx_buffer_flush on any other buffer will dereference null
because the tx buffer for that port was not allocated.

Also:
   - the flush code is common enough that it should not be marked
     unlikely
   - combine conditions to reduce indentation
   - avoid unnecessary if() if sent is zero.

Fixes: e2366e74e029 ("examples: use buffered Tx")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 .../client_server_mp/mp_client/client.c        | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_client/client.c b/examples/multi_process/client_server_mp/mp_client/client.c
index c23dd3f378f7..361d90b54b2d 100644
--- a/examples/multi_process/client_server_mp/mp_client/client.c
+++ b/examples/multi_process/client_server_mp/mp_client/client.c
@@ -246,19 +246,19 @@ main(int argc, char *argv[])
 
 	for (;;) {
 		uint16_t i, rx_pkts;
-		uint16_t port;
 
 		rx_pkts = rte_ring_dequeue_burst(rx_ring, pkts,
 				PKT_READ_SIZE, NULL);
 
-		if (unlikely(rx_pkts == 0)){
-			if (need_flush)
-				for (port = 0; port < ports->num_ports; port++) {
-					sent = rte_eth_tx_buffer_flush(ports->id[port], client_id,
-							tx_buffer[port]);
-					if (unlikely(sent))
-						tx_stats->tx[port] += sent;
-				}
+		if (rx_pkts == 0 && need_flush) {
+			for (i = 0; i < ports->num_ports; i++) {
+				uint16_t port = ports->id[i];
+
+				sent = rte_eth_tx_buffer_flush(port,
+							       client_id,
+							       tx_buffer[port]);
+				tx_stats->tx[port] += sent;
+			}
 			need_flush = 0;
 			continue;
 		}
-- 
2.20.1

[dpdk-dev] [PATCH v5 3/4] examples/multi_process/client_server_mp/mp_server: fix style

[Stephen Hemminger]

Lots of little style complaints from checkpatch.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 .../client_server_mp/mp_server/args.c         | 37 ++++-----
 .../client_server_mp/mp_server/init.c         | 79 +++++++++++--------
 .../client_server_mp/mp_server/main.c         | 44 ++++++-----
 3 files changed, 88 insertions(+), 72 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_server/args.c b/examples/multi_process/client_server_mp/mp_server/args.c
index fdc008b3d677..91c823856a72 100644
--- a/examples/multi_process/client_server_mp/mp_server/args.c
+++ b/examples/multi_process/client_server_mp/mp_server/args.c
@@ -112,33 +112,34 @@ parse_app_args(int argc, char *argv[])
 	progname = argv[0];
 
 	while ((opt = getopt_long(argc, argvopt, "n:p:", lgopts,
-		&option_index)) != EOF){
-		switch (opt){
-			case 'p':
-				if (parse_portmask(optarg) != 0) {
-					usage();
-					return -1;
-				}
-				break;
-			case 'n':
-				if (parse_num_clients(optarg) != 0){
-					usage();
-					return -1;
-				}
-				break;
-			default:
-				printf("ERROR: Unknown option '%c'\n", opt);
+				  &option_index)) != EOF) {
+
+		switch (opt) {
+		case 'p':
+			if (parse_portmask(optarg) != 0) {
+				usage();
+				return -1;
+			}
+			break;
+		case 'n':
+			if (parse_num_clients(optarg) != 0) {
 				usage();
 				return -1;
+			}
+			break;
+		default:
+			printf("ERROR: Unknown option '%c'\n", opt);
+			usage();
+			return -1;
 		}
 	}
 
-	if (ports->num_ports == 0 || num_clients == 0){
+	if (ports->num_ports == 0 || num_clients == 0) {
 		usage();
 		return -1;
 	}
 
-	if (ports->num_ports % 2 != 0){
+	if (ports->num_ports % 2 != 0) {
 		printf("ERROR: application requires an even number of ports to use\n");
 		return -1;
 	}
diff --git a/examples/multi_process/client_server_mp/mp_server/init.c b/examples/multi_process/client_server_mp/mp_server/init.c
index 1b0569937b51..96c35f220a7d 100644
--- a/examples/multi_process/client_server_mp/mp_server/init.c
+++ b/examples/multi_process/client_server_mp/mp_server/init.c
@@ -49,7 +49,7 @@
 struct rte_mempool *pktmbuf_pool;
 
 /* array of info/queues for clients */
-struct client *clients = NULL;
+struct client *clients;
 
 /* the port details */
 struct port_info *ports;
@@ -72,7 +72,8 @@ init_mbuf_pools(void)
 		num_mbufs_server + num_mbufs_client + num_mbufs_mp_cache;
 
 	/* don't pass single-producer/single-consumer flags to mbuf create as it
-	 * seems faster to use a cache instead */
+	 * seems faster to use a cache instead
+	 */
 	printf("Creating mbuf pool '%s' [%u mbufs] ...\n",
 			PKTMBUF_POOL_NAME, num_mbufs);
 	pktmbuf_pool = rte_pktmbuf_pool_create(PKTMBUF_POOL_NAME, num_mbufs,
@@ -108,9 +109,11 @@ init_port(uint16_t port_num)
 	fflush(stdout);
 
 	/* Standard DPDK port initialisation - config port, then set up
-	 * rx and tx rings */
-	if ((retval = rte_eth_dev_configure(port_num, rx_rings, tx_rings,
-		&port_conf)) != 0)
+	 * rx and tx rings
+	 */
+	retval = rte_eth_dev_configure(port_num, rx_rings, tx_rings,
+				       &port_conf);
+	if (retval != 0)
 		return retval;
 
 	retval = rte_eth_dev_adjust_nb_rx_tx_desc(port_num, &rx_ring_size,
@@ -122,22 +125,25 @@ init_port(uint16_t port_num)
 		retval = rte_eth_rx_queue_setup(port_num, q, rx_ring_size,
 				rte_eth_dev_socket_id(port_num),
 				NULL, pktmbuf_pool);
-		if (retval < 0) return retval;
+		if (retval < 0)
+			return retval;
 	}
 
-	for ( q = 0; q < tx_rings; q ++ ) {
+	for (q = 0; q < tx_rings; q++) {
 		retval = rte_eth_tx_queue_setup(port_num, q, tx_ring_size,
 				rte_eth_dev_socket_id(port_num),
 				NULL);
-		if (retval < 0) return retval;
+		if (retval < 0)
+			return retval;
 	}
 
 	rte_eth_promiscuous_enable(port_num);
 
 	retval  = rte_eth_dev_start(port_num);
-	if (retval < 0) return retval;
+	if (retval < 0)
+		return retval;
 
-	printf( "done: \n");
+	printf("done:\n");
 
 	return 0;
 }
@@ -150,15 +156,15 @@ init_port(uint16_t port_num)
 static int
 init_shm_rings(void)
 {
-	unsigned i;
-	unsigned socket_id;
-	const char * q_name;
-	const unsigned ringsize = CLIENT_QUEUE_RINGSIZE;
+	unsigned int i, socket_id;
+	const char *q_name;
+	const unsigned int ringsize = CLIENT_QUEUE_RINGSIZE;
 
 	clients = rte_malloc("client details",
 		sizeof(*clients) * num_clients, 0);
 	if (clients == NULL)
-		rte_exit(EXIT_FAILURE, "Cannot allocate memory for client program details\n");
+		rte_exit(EXIT_FAILURE,
+			 "Cannot allocate memory for client program details\n");
 
 	for (i = 0; i < num_clients; i++) {
 		/* Create an RX queue for each client */
@@ -166,13 +172,27 @@ init_shm_rings(void)
 		q_name = get_rx_queue_name(i);
 		clients[i].rx_q = rte_ring_create(q_name,
 				ringsize, socket_id,
-				RING_F_SP_ENQ | RING_F_SC_DEQ ); /* single prod, single cons */
+				RING_F_SP_ENQ | RING_F_SC_DEQ);
 		if (clients[i].rx_q == NULL)
-			rte_exit(EXIT_FAILURE, "Cannot create rx ring queue for client %u\n", i);
+			rte_exit(EXIT_FAILURE,
+				 "Cannot create rx ring queue for client %u\n",
+				 i);
 	}
 	return 0;
 }
 
+static void
+print_link_status(uint16 id, const struct rte_eth_link *link)
+{
+	if (link->link_status)
+		printf("Port %d Link Up - speed %u Mbps - %s-duplex\n",
+		       id, link->link_speed,
+		       (link->link_duplex == ETH_LINK_FULL_DUPLEX) ?
+		       "full" : "half");
+	else
+		printf("Port %d Link Down\n", id);
+}
+
 /* Check the link status of all ports in up to 9s, and print them finally */
 static void
 check_all_ports_link_status(uint16_t port_num, uint32_t port_mask)
@@ -192,21 +212,11 @@ check_all_ports_link_status(uint16_t port_num, uint32_t port_mask)
 				continue;
 			memset(&link, 0, sizeof(link));
 			rte_eth_link_get_nowait(ports->id[portid], &link);
+
 			/* print link status if flag set */
-			if (print_flag == 1) {
-				if (link.link_status)
-					printf("Port %d Link Up - speed %u "
-						"Mbps - %s\n", ports->id[portid],
-						(unsigned)link.link_speed,
-				(link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
-					("full-duplex") : ("half-duplex\n"));
-				else
-					printf("Port %d Link Down\n",
-						(uint8_t)ports->id[portid]);
-				continue;
-			}
-			/* clear all_ports_up flag if any link down */
-			if (link.link_status == ETH_LINK_DOWN) {
+			if (print_flag == 1)
+				print_link_status(&link);
+			else if (link.link_status == ETH_LINK_DOWN) {
 				all_ports_up = 0;
 				break;
 			}
@@ -251,7 +261,8 @@ init(int argc, char *argv[])
 	mz = rte_memzone_reserve(MZ_PORT_INFO, sizeof(*ports),
 				rte_socket_id(), NO_FLAGS);
 	if (mz == NULL)
-		rte_exit(EXIT_FAILURE, "Cannot reserve memory zone for port information\n");
+		rte_exit(EXIT_FAILURE,
+			 "Cannot reserve memory zone for port information\n");
 	memset(mz->addr, 0, sizeof(*ports));
 	ports = mz->addr;
 
@@ -269,8 +280,8 @@ init(int argc, char *argv[])
 	for (i = 0; i < ports->num_ports; i++) {
 		retval = init_port(ports->id[i]);
 		if (retval != 0)
-			rte_exit(EXIT_FAILURE, "Cannot initialise port %u\n",
-					(unsigned)i);
+			rte_exit(EXIT_FAILURE,
+				 "Cannot initialise port %u\n", i);
 	}
 
 	check_all_ports_link_status(ports->num_ports, (~0x0));
diff --git a/examples/multi_process/client_server_mp/mp_server/main.c b/examples/multi_process/client_server_mp/mp_server/main.c
index 0150533700f0..bfec0bef3a71 100644
--- a/examples/multi_process/client_server_mp/mp_server/main.c
+++ b/examples/multi_process/client_server_mp/mp_server/main.c
@@ -64,8 +64,9 @@ get_printable_mac_addr(uint16_t port)
 
 	if (unlikely(port >= RTE_MAX_ETHPORTS))
 		return err_address;
-	if (unlikely(addresses[port][0]=='\0')){
+	if (unlikely(addresses[port][0] == '\0')) {
 		struct rte_ether_addr mac;
+
 		rte_eth_macaddr_get(port, &mac);
 		snprintf(addresses[port], sizeof(addresses[port]),
 				"%02x:%02x:%02x:%02x:%02x:%02x\n",
@@ -85,9 +86,9 @@ get_printable_mac_addr(uint16_t port)
 static void
 do_stats_display(void)
 {
-	unsigned i, j;
+	unsigned int i, j;
 	const char clr[] = { 27, '[', '2', 'J', '\0' };
-	const char topLeft[] = { 27, '[', '1', ';', '1', 'H','\0' };
+	const char topLeft[] = { 27, '[', '1', ';', '1', 'H', '\0' };
 	uint64_t port_tx[RTE_MAX_ETHPORTS], port_tx_drop[RTE_MAX_ETHPORTS];
 	uint64_t client_tx[MAX_CLIENTS], client_tx_drop[MAX_CLIENTS];
 
@@ -97,12 +98,14 @@ do_stats_display(void)
 	memset(client_tx, 0, sizeof(client_tx));
 	memset(client_tx_drop, 0, sizeof(client_tx_drop));
 
-	for (i = 0; i < num_clients; i++){
+	for (i = 0; i < num_clients; i++) {
 		const volatile struct tx_stats *tx = &ports->tx_stats[i];
-		for (j = 0; j < ports->num_ports; j++){
+
+		for (j = 0; j < ports->num_ports; j++) {
 			/* assign to local variables here, save re-reading volatile vars */
 			const uint64_t tx_val = tx->tx[ports->id[j]];
 			const uint64_t drop_val = tx->tx_drop[ports->id[j]];
+
 			port_tx[j] += tx_val;
 			port_tx_drop[j] += drop_val;
 			client_tx[i] += tx_val;
@@ -116,21 +119,21 @@ do_stats_display(void)
 	printf("PORTS\n");
 	printf("-----\n");
 	for (i = 0; i < ports->num_ports; i++)
-		printf("Port %u: '%s'\t", (unsigned)ports->id[i],
-				get_printable_mac_addr(ports->id[i]));
+		printf("Port %u: '%s'\t", ports->id[i],
+		       get_printable_mac_addr(ports->id[i]));
 	printf("\n\n");
-	for (i = 0; i < ports->num_ports; i++){
-		printf("Port %u - rx: %9"PRIu64"\t"
-				"tx: %9"PRIu64"\n",
-				(unsigned)ports->id[i], ports->rx_stats.rx[i],
+	for (i = 0; i < ports->num_ports; i++) {
+		printf("Port %u - rx: %9"PRIu64"\ttx: %9"PRIu64"\n",
+				ports->id[i], ports->rx_stats.rx[i],
 				port_tx[i]);
 	}
 
 	printf("\nCLIENTS\n");
 	printf("-------\n");
-	for (i = 0; i < num_clients; i++){
+	for (i = 0; i < num_clients; i++) {
 		const unsigned long long rx = clients[i].stats.rx;
 		const unsigned long long rx_drop = clients[i].stats.rx_drop;
+
 		printf("Client %2u - rx: %9llu, rx_drop: %9llu\n"
 				"            tx: %9"PRIu64", tx_drop: %9"PRIu64"\n",
 				i, rx, rx_drop, client_tx[i], client_tx_drop[i]);
@@ -153,7 +156,8 @@ sleep_lcore(__attribute__((unused)) void *dummy)
 
 	/* Only one core should display stats */
 	if (rte_atomic32_test_and_set(&display_stats)) {
-		const unsigned sleeptime = 1;
+		const unsigned int sleeptime = 1;
+
 		printf("Core %u displaying statistics\n", rte_lcore_id());
 
 		/* Longer initial pause so above printf is seen */
@@ -173,7 +177,7 @@ sleep_lcore(__attribute__((unused)) void *dummy)
 static void
 clear_stats(void)
 {
-	unsigned i;
+	unsigned int i;
 
 	for (i = 0; i < num_clients; i++)
 		clients[i].stats.rx = clients[i].stats.rx_drop = 0;
@@ -194,12 +198,11 @@ flush_rx_queue(uint16_t client)
 
 	cl = &clients[client];
 	if (rte_ring_enqueue_bulk(cl->rx_q, (void **)cl_rx_buf[client].buffer,
-			cl_rx_buf[client].count, NULL) == 0){
+			cl_rx_buf[client].count, NULL) == 0) {
 		for (j = 0; j < cl_rx_buf[client].count; j++)
 			rte_pktmbuf_free(cl_rx_buf[client].buffer[j]);
 		cl->stats.rx_drop += cl_rx_buf[client].count;
-	}
-	else
+	} else
 		cl->stats.rx += cl_rx_buf[client].count;
 
 	cl_rx_buf[client].count = 0;
@@ -243,14 +246,14 @@ process_packets(uint32_t port_num __rte_unused,
 static void
 do_packet_forwarding(void)
 {
-	unsigned port_num = 0; /* indexes the port[] array */
+	unsigned int port_num = 0; /* indexes the port[] array */
 
 	for (;;) {
 		struct rte_mbuf *buf[PACKET_READ_SIZE];
 		uint16_t rx_count;
 
 		/* read a port */
-		rx_count = rte_eth_rx_burst(ports->id[port_num], 0, \
+		rx_count = rte_eth_rx_burst(ports->id[port_num], 0,
 				buf, PACKET_READ_SIZE);
 		ports->rx_stats.rx[port_num] += rx_count;
 
@@ -281,8 +284,9 @@ int
 main(int argc, char *argv[])
 {
 	signal(SIGINT, signal_handler);
+
 	/* initialise the system */
-	if (init(argc, argv) < 0 )
+	if (init(argc, argv) < 0)
 		return -1;
 	RTE_LOG(INFO, APP, "Finished Process Init.\n");
 
-- 
2.20.1

[dpdk-dev] [PATCH v5 4/4] examples/multi_process/client_server_mp/mp_server: use ether format address

[Stephen Hemminger]

No need to use snprintf to print ethernet address.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 .../client_server_mp/mp_server/main.c         | 32 ++++++-------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_server/main.c b/examples/multi_process/client_server_mp/mp_server/main.c
index bfec0bef3a71..d1339d4be64c 100644
--- a/examples/multi_process/client_server_mp/mp_server/main.c
+++ b/examples/multi_process/client_server_mp/mp_server/main.c
@@ -56,26 +56,6 @@ struct client_rx_buf {
 /* One buffer per client rx queue - dynamically allocate array */
 static struct client_rx_buf *cl_rx_buf;
 
-static const char *
-get_printable_mac_addr(uint16_t port)
-{
-	static const char err_address[] = "00:00:00:00:00:00";
-	static char addresses[RTE_MAX_ETHPORTS][sizeof(err_address)];
-
-	if (unlikely(port >= RTE_MAX_ETHPORTS))
-		return err_address;
-	if (unlikely(addresses[port][0] == '\0')) {
-		struct rte_ether_addr mac;
-
-		rte_eth_macaddr_get(port, &mac);
-		snprintf(addresses[port], sizeof(addresses[port]),
-				"%02x:%02x:%02x:%02x:%02x:%02x\n",
-				mac.addr_bytes[0], mac.addr_bytes[1], mac.addr_bytes[2],
-				mac.addr_bytes[3], mac.addr_bytes[4], mac.addr_bytes[5]);
-	}
-	return addresses[port];
-}
-
 /*
  * This function displays the recorded statistics for each port
  * and for each client. It uses ANSI terminal codes to clear
@@ -118,9 +98,15 @@ do_stats_display(void)
 
 	printf("PORTS\n");
 	printf("-----\n");
-	for (i = 0; i < ports->num_ports; i++)
-		printf("Port %u: '%s'\t", ports->id[i],
-		       get_printable_mac_addr(ports->id[i]));
+	for (i = 0; i < ports->num_ports; i++) {
+		struct rte_ether_addr mac = { };
+		char buf[32];
+
+		rte_eth_macaddr_get(ports->id[i], &mac);
+		rte_ether_format_addr(buf, sizeof(buf), &mac);
+		printf("Port %u: '%s'\t", ports->id[i], buf);
+	}
+
 	printf("\n\n");
 	for (i = 0; i < ports->num_ports; i++) {
 		printf("Port %u - rx: %9"PRIu64"\ttx: %9"PRIu64"\n",
-- 
2.20.1

Re: [dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Matan Azrad]

Hi Stephen

One more small  comment inline

From:  Stephen Hemminger
> Sent: Friday, August 2, 2019 5:58 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <sthemmin@microsoft.com>
> Subject: [dpdk-dev] [PATCH v5 1/4]
> examples/multi_process/client_server_mp: check port validity
> 
> From: Stephen Hemminger <sthemmin@microsoft.com>
> 
> The mp_server incorrectly allows a port mask that included hidden ports and
> which later caused either lost packets or failed initialization.
> 
> This fixes explicitly checking that each bit in portmask is a valid port before
> using it.
> 
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>  .../client_server_mp/mp_server/args.c         | 35 ++++++++++---------
>  .../client_server_mp/mp_server/args.h         |  2 +-
>  .../client_server_mp/mp_server/init.c         |  7 ++--
>  3 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/examples/multi_process/client_server_mp/mp_server/args.c
> b/examples/multi_process/client_server_mp/mp_server/args.c
> index b0d8d7665c85..fdc008b3d677 100644
> --- a/examples/multi_process/client_server_mp/mp_server/args.c
> +++ b/examples/multi_process/client_server_mp/mp_server/args.c
> @@ -10,6 +10,7 @@
>  #include <errno.h>
> 
>  #include <rte_memory.h>
> +#include <rte_ethdev.h>
>  #include <rte_string_fns.h>
> 
>  #include "common.h"
> @@ -41,31 +42,33 @@ usage(void)
>   * array variable
>   */
>  static int
> -parse_portmask(uint8_t max_ports, const char *portmask)
> +parse_portmask(const char *portmask)
>  {
>  	char *end = NULL;
>  	unsigned long pm;
> -	uint16_t count = 0;
> +	uint16_t id;
> 
>  	if (portmask == NULL || *portmask == '\0')
>  		return -1;
> 
>  	/* convert parameter to a number and verify */
>  	pm = strtoul(portmask, &end, 16);
> -	if (end == NULL || *end != '\0' || pm == 0)
> +	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)

Why pm > UINT16_MAX ? should be something like > (1 << RTE_MAX_ETHPORTS) - 1.
And need to be sure pm type can hold RTE_MAX_ETHPORTS bits, otherwise port 0 may unlikely be all the time visible in the loop below.

>  		return -1;
> 
> -	/* loop through bits of the mask and mark ports */
> -	while (pm != 0){
> -		if (pm & 0x01){ /* bit is set in mask, use port */
> -			if (count >= max_ports)
> -				printf("WARNING: requested port %u not
> present"
> -				" - ignoring\n", (unsigned)count);
> -			else
> -			    ports->id[ports->num_ports++] = count;
> -		}
> -		pm = (pm >> 1);
> -		count++;
> +	RTE_ETH_FOREACH_DEV(id) {
> +		unsigned long msk = 1u << id;
> +
> +		if ((pm & msk) == 0)
> +			continue;
> +
> +		pm &= ~msk;
> +		ports->id[ports->num_ports++] = id;
> +	}
> +
> +	if (pm != 0) {
> +		printf("WARNING: leftover ports in mask %#lx - ignoring\n",
> +		       pm);
>  	}
> 
>  	return 0;
> @@ -99,7 +102,7 @@ parse_num_clients(const char *clients)
>   * on error.
>   */
>  int
> -parse_app_args(uint16_t max_ports, int argc, char *argv[])
> +parse_app_args(int argc, char *argv[])
>  {
>  	int option_index, opt;
>  	char **argvopt = argv;
> @@ -112,7 +115,7 @@ parse_app_args(uint16_t max_ports, int argc, char
> *argv[])
>  		&option_index)) != EOF){
>  		switch (opt){
>  			case 'p':
> -				if (parse_portmask(max_ports, optarg) != 0){
> +				if (parse_portmask(optarg) != 0) {
>  					usage();
>  					return -1;
>  				}
> diff --git a/examples/multi_process/client_server_mp/mp_server/args.h
> b/examples/multi_process/client_server_mp/mp_server/args.h
> index 79c190a33a37..52c8cc86e6f0 100644
> --- a/examples/multi_process/client_server_mp/mp_server/args.h
> +++ b/examples/multi_process/client_server_mp/mp_server/args.h
> @@ -5,6 +5,6 @@
>  #ifndef _ARGS_H_
>  #define _ARGS_H_
> 
> -int parse_app_args(uint16_t max_ports, int argc, char *argv[]);
> +int parse_app_args(int argc, char *argv[]);
> 
>  #endif /* ifndef _ARGS_H_ */
> diff --git a/examples/multi_process/client_server_mp/mp_server/init.c
> b/examples/multi_process/client_server_mp/mp_server/init.c
> index 3af5dc6994bf..1b0569937b51 100644
> --- a/examples/multi_process/client_server_mp/mp_server/init.c
> +++ b/examples/multi_process/client_server_mp/mp_server/init.c
> @@ -238,7 +238,7 @@ init(int argc, char *argv[])  {
>  	int retval;
>  	const struct rte_memzone *mz;
> -	uint16_t i, total_ports;
> +	uint16_t i;
> 
>  	/* init EAL, parsing EAL args */
>  	retval = rte_eal_init(argc, argv);
> @@ -247,9 +247,6 @@ init(int argc, char *argv[])
>  	argc -= retval;
>  	argv += retval;
> 
> -	/* get total number of ports */
> -	total_ports = rte_eth_dev_count_total();
> -
>  	/* set up array for port data */
>  	mz = rte_memzone_reserve(MZ_PORT_INFO, sizeof(*ports),
>  				rte_socket_id(), NO_FLAGS);
> @@ -259,7 +256,7 @@ init(int argc, char *argv[])
>  	ports = mz->addr;
> 
>  	/* parse additional, application arguments */
> -	retval = parse_app_args(total_ports, argc, argv);
> +	retval = parse_app_args(argc, argv);
>  	if (retval != 0)
>  		return -1;
> 
> --
> 2.20.1

Re: [dpdk-dev] [PATCH v5 3/4] examples/multi_process/client_server_mp/mp_server: fix style

[David Marchand]

On Fri, Aug 2, 2019 at 4:59 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> Lots of little style complaints from checkpatch.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  .../client_server_mp/mp_server/args.c         | 37 ++++-----
>  .../client_server_mp/mp_server/init.c         | 79 +++++++++++--------
>  .../client_server_mp/mp_server/main.c         | 44 ++++++-----
>  3 files changed, 88 insertions(+), 72 deletions(-)
>
> diff --git a/examples/multi_process/client_server_mp/mp_server/args.c b/examples/multi_process/client_server_mp/mp_server/args.c
> index fdc008b3d677..91c823856a72 100644
> --- a/examples/multi_process/client_server_mp/mp_server/args.c
> +++ b/examples/multi_process/client_server_mp/mp_server/args.c
> @@ -112,33 +112,34 @@ parse_app_args(int argc, char *argv[])
>         progname = argv[0];
>
>         while ((opt = getopt_long(argc, argvopt, "n:p:", lgopts,
> -               &option_index)) != EOF){
> -               switch (opt){
> -                       case 'p':
> -                               if (parse_portmask(optarg) != 0) {
> -                                       usage();
> -                                       return -1;
> -                               }
> -                               break;
> -                       case 'n':
> -                               if (parse_num_clients(optarg) != 0){
> -                                       usage();
> -                                       return -1;
> -                               }
> -                               break;
> -                       default:
> -                               printf("ERROR: Unknown option '%c'\n", opt);
> +                                 &option_index)) != EOF) {
> +
> +               switch (opt) {
> +               case 'p':
> +                       if (parse_portmask(optarg) != 0) {
> +                               usage();
> +                               return -1;
> +                       }
> +                       break;
> +               case 'n':
> +                       if (parse_num_clients(optarg) != 0) {
>                                 usage();
>                                 return -1;
> +                       }
> +                       break;
> +               default:
> +                       printf("ERROR: Unknown option '%c'\n", opt);
> +                       usage();
> +                       return -1;
>                 }
>         }
>
> -       if (ports->num_ports == 0 || num_clients == 0){
> +       if (ports->num_ports == 0 || num_clients == 0) {
>                 usage();
>                 return -1;
>         }
>
> -       if (ports->num_ports % 2 != 0){
> +       if (ports->num_ports % 2 != 0) {
>                 printf("ERROR: application requires an even number of ports to use\n");
>                 return -1;
>         }
> diff --git a/examples/multi_process/client_server_mp/mp_server/init.c b/examples/multi_process/client_server_mp/mp_server/init.c
> index 1b0569937b51..96c35f220a7d 100644
> --- a/examples/multi_process/client_server_mp/mp_server/init.c
> +++ b/examples/multi_process/client_server_mp/mp_server/init.c
> @@ -49,7 +49,7 @@
>  struct rte_mempool *pktmbuf_pool;
>
>  /* array of info/queues for clients */
> -struct client *clients = NULL;
> +struct client *clients;
>
>  /* the port details */
>  struct port_info *ports;
> @@ -72,7 +72,8 @@ init_mbuf_pools(void)
>                 num_mbufs_server + num_mbufs_client + num_mbufs_mp_cache;
>
>         /* don't pass single-producer/single-consumer flags to mbuf create as it
> -        * seems faster to use a cache instead */
> +        * seems faster to use a cache instead
> +        */
>         printf("Creating mbuf pool '%s' [%u mbufs] ...\n",
>                         PKTMBUF_POOL_NAME, num_mbufs);
>         pktmbuf_pool = rte_pktmbuf_pool_create(PKTMBUF_POOL_NAME, num_mbufs,
> @@ -108,9 +109,11 @@ init_port(uint16_t port_num)
>         fflush(stdout);
>
>         /* Standard DPDK port initialisation - config port, then set up
> -        * rx and tx rings */
> -       if ((retval = rte_eth_dev_configure(port_num, rx_rings, tx_rings,
> -               &port_conf)) != 0)
> +        * rx and tx rings
> +        */
> +       retval = rte_eth_dev_configure(port_num, rx_rings, tx_rings,
> +                                      &port_conf);
> +       if (retval != 0)
>                 return retval;
>
>         retval = rte_eth_dev_adjust_nb_rx_tx_desc(port_num, &rx_ring_size,
> @@ -122,22 +125,25 @@ init_port(uint16_t port_num)
>                 retval = rte_eth_rx_queue_setup(port_num, q, rx_ring_size,
>                                 rte_eth_dev_socket_id(port_num),
>                                 NULL, pktmbuf_pool);
> -               if (retval < 0) return retval;
> +               if (retval < 0)
> +                       return retval;
>         }
>
> -       for ( q = 0; q < tx_rings; q ++ ) {
> +       for (q = 0; q < tx_rings; q++) {
>                 retval = rte_eth_tx_queue_setup(port_num, q, tx_ring_size,
>                                 rte_eth_dev_socket_id(port_num),
>                                 NULL);
> -               if (retval < 0) return retval;
> +               if (retval < 0)
> +                       return retval;
>         }
>
>         rte_eth_promiscuous_enable(port_num);
>
>         retval  = rte_eth_dev_start(port_num);
> -       if (retval < 0) return retval;
> +       if (retval < 0)
> +               return retval;
>
> -       printf( "done: \n");
> +       printf("done:\n");
>
>         return 0;
>  }
> @@ -150,15 +156,15 @@ init_port(uint16_t port_num)
>  static int
>  init_shm_rings(void)
>  {
> -       unsigned i;
> -       unsigned socket_id;
> -       const char * q_name;
> -       const unsigned ringsize = CLIENT_QUEUE_RINGSIZE;
> +       unsigned int i, socket_id;
> +       const char *q_name;
> +       const unsigned int ringsize = CLIENT_QUEUE_RINGSIZE;
>
>         clients = rte_malloc("client details",
>                 sizeof(*clients) * num_clients, 0);
>         if (clients == NULL)
> -               rte_exit(EXIT_FAILURE, "Cannot allocate memory for client program details\n");
> +               rte_exit(EXIT_FAILURE,
> +                        "Cannot allocate memory for client program details\n");
>
>         for (i = 0; i < num_clients; i++) {
>                 /* Create an RX queue for each client */
> @@ -166,13 +172,27 @@ init_shm_rings(void)
>                 q_name = get_rx_queue_name(i);
>                 clients[i].rx_q = rte_ring_create(q_name,
>                                 ringsize, socket_id,
> -                               RING_F_SP_ENQ | RING_F_SC_DEQ ); /* single prod, single cons */
> +                               RING_F_SP_ENQ | RING_F_SC_DEQ);
>                 if (clients[i].rx_q == NULL)
> -                       rte_exit(EXIT_FAILURE, "Cannot create rx ring queue for client %u\n", i);
> +                       rte_exit(EXIT_FAILURE,
> +                                "Cannot create rx ring queue for client %u\n",
> +                                i);
>         }
>         return 0;
>  }
>
> +static void
> +print_link_status(uint16 id, const struct rte_eth_link *link)


uint16_t

See build error at
http://mails.dpdk.org/archives/test-report/2019-August/092150.html


-- 
David Marchand

Re: [dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

On Fri, 2 Aug 2019 05:33:20 +0000
Matan Azrad <matan@mellanox.com> wrote:

> Hi Stephen
> 
> One more small  comment inline
> 
> From:  Stephen Hemminger
> > Sent: Friday, August 2, 2019 5:58 AM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > Subject: [dpdk-dev] [PATCH v5 1/4]
> > examples/multi_process/client_server_mp: check port validity
> > 
> > From: Stephen Hemminger <sthemmin@microsoft.com>
> > 
> > The mp_server incorrectly allows a port mask that included hidden ports and
> > which later caused either lost packets or failed initialization.
> > 
> > This fixes explicitly checking that each bit in portmask is a valid port before
> > using it.
> > 
> > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > ---
> >  .../client_server_mp/mp_server/args.c         | 35 ++++++++++---------
> >  .../client_server_mp/mp_server/args.h         |  2 +-
> >  .../client_server_mp/mp_server/init.c         |  7 ++--
> >  3 files changed, 22 insertions(+), 22 deletions(-)
> > 
> > diff --git a/examples/multi_process/client_server_mp/mp_server/args.c
> > b/examples/multi_process/client_server_mp/mp_server/args.c
> > index b0d8d7665c85..fdc008b3d677 100644
> > --- a/examples/multi_process/client_server_mp/mp_server/args.c
> > +++ b/examples/multi_process/client_server_mp/mp_server/args.c
> > @@ -10,6 +10,7 @@
> >  #include <errno.h>
> > 
> >  #include <rte_memory.h>
> > +#include <rte_ethdev.h>
> >  #include <rte_string_fns.h>
> > 
> >  #include "common.h"
> > @@ -41,31 +42,33 @@ usage(void)
> >   * array variable
> >   */
> >  static int
> > -parse_portmask(uint8_t max_ports, const char *portmask)
> > +parse_portmask(const char *portmask)
> >  {
> >  	char *end = NULL;
> >  	unsigned long pm;
> > -	uint16_t count = 0;
> > +	uint16_t id;
> > 
> >  	if (portmask == NULL || *portmask == '\0')
> >  		return -1;
> > 
> >  	/* convert parameter to a number and verify */
> >  	pm = strtoul(portmask, &end, 16);
> > -	if (end == NULL || *end != '\0' || pm == 0)
> > +	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)  
> 
> Why pm > UINT16_MAX ? should be something like > (1 << RTE_MAX_ETHPORTS) - 1.
> And need to be sure pm type can hold RTE_MAX_ETHPORTS bits, otherwise port 0 may unlikely be all the time visible in the loop below.
>

The DPDK assumes a lot of places that unsigned long will hold a port mask.
If some extra bits are set, the error is visible later when the bits are leftover
after finding ports.

The original code had worse problems, it would not catch invalid pm values at all
and truncate silently.

[dpdk-dev] [PATCH v6 0/2] examples/client_server_mp: port id (fixes only)

[Stephen Hemminger]

v6 - just include fixes; cleanups can wait until 19.11

v5 - change logic in server_mp for evaluating port mask

v4 - fix checkpatch warning
     add patches to fix style issues and use ether format addr

v3 - merge both patches in one series
     use alternative algorithm to check port ownership (N^2)
     because reviewer didn't like direct check.

Stephen Hemminger (2):
  examples/multi_process/client_server_mp: check port validity
  examples/multi_process/client_server_mp - fix crash in mp_client with
    sparse ports

 .../client_server_mp/mp_client/client.c       | 18 +++++-----
 .../client_server_mp/mp_server/args.c         | 35 ++++++++++---------
 .../client_server_mp/mp_server/args.h         |  2 +-
 .../client_server_mp/mp_server/init.c         |  7 ++--
 4 files changed, 31 insertions(+), 31 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v6 1/2] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

From: Stephen Hemminger <sthemmin@microsoft.com>

The mp_server incorrectly allows a port mask that included hidden
ports and which later caused either lost packets or failed initialization.

This fixes explicitly checking that each bit in portmask is a
valid port before using it.

Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 .../client_server_mp/mp_server/args.c         | 35 ++++++++++---------
 .../client_server_mp/mp_server/args.h         |  2 +-
 .../client_server_mp/mp_server/init.c         |  7 ++--
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_server/args.c b/examples/multi_process/client_server_mp/mp_server/args.c
index b0d8d7665c85..fdc008b3d677 100644
--- a/examples/multi_process/client_server_mp/mp_server/args.c
+++ b/examples/multi_process/client_server_mp/mp_server/args.c
@@ -10,6 +10,7 @@
 #include <errno.h>
 
 #include <rte_memory.h>
+#include <rte_ethdev.h>
 #include <rte_string_fns.h>
 
 #include "common.h"
@@ -41,31 +42,33 @@ usage(void)
  * array variable
  */
 static int
-parse_portmask(uint8_t max_ports, const char *portmask)
+parse_portmask(const char *portmask)
 {
 	char *end = NULL;
 	unsigned long pm;
-	uint16_t count = 0;
+	uint16_t id;
 
 	if (portmask == NULL || *portmask == '\0')
 		return -1;
 
 	/* convert parameter to a number and verify */
 	pm = strtoul(portmask, &end, 16);
-	if (end == NULL || *end != '\0' || pm == 0)
+	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
 		return -1;
 
-	/* loop through bits of the mask and mark ports */
-	while (pm != 0){
-		if (pm & 0x01){ /* bit is set in mask, use port */
-			if (count >= max_ports)
-				printf("WARNING: requested port %u not present"
-				" - ignoring\n", (unsigned)count);
-			else
-			    ports->id[ports->num_ports++] = count;
-		}
-		pm = (pm >> 1);
-		count++;
+	RTE_ETH_FOREACH_DEV(id) {
+		unsigned long msk = 1u << id;
+
+		if ((pm & msk) == 0)
+			continue;
+
+		pm &= ~msk;
+		ports->id[ports->num_ports++] = id;
+	}
+
+	if (pm != 0) {
+		printf("WARNING: leftover ports in mask %#lx - ignoring\n",
+		       pm);
 	}
 
 	return 0;
@@ -99,7 +102,7 @@ parse_num_clients(const char *clients)
  * on error.
  */
 int
-parse_app_args(uint16_t max_ports, int argc, char *argv[])
+parse_app_args(int argc, char *argv[])
 {
 	int option_index, opt;
 	char **argvopt = argv;
@@ -112,7 +115,7 @@ parse_app_args(uint16_t max_ports, int argc, char *argv[])
 		&option_index)) != EOF){
 		switch (opt){
 			case 'p':
-				if (parse_portmask(max_ports, optarg) != 0){
+				if (parse_portmask(optarg) != 0) {
 					usage();
 					return -1;
 				}
diff --git a/examples/multi_process/client_server_mp/mp_server/args.h b/examples/multi_process/client_server_mp/mp_server/args.h
index 79c190a33a37..52c8cc86e6f0 100644
--- a/examples/multi_process/client_server_mp/mp_server/args.h
+++ b/examples/multi_process/client_server_mp/mp_server/args.h
@@ -5,6 +5,6 @@
 #ifndef _ARGS_H_
 #define _ARGS_H_
 
-int parse_app_args(uint16_t max_ports, int argc, char *argv[]);
+int parse_app_args(int argc, char *argv[]);
 
 #endif /* ifndef _ARGS_H_ */
diff --git a/examples/multi_process/client_server_mp/mp_server/init.c b/examples/multi_process/client_server_mp/mp_server/init.c
index 3af5dc6994bf..1b0569937b51 100644
--- a/examples/multi_process/client_server_mp/mp_server/init.c
+++ b/examples/multi_process/client_server_mp/mp_server/init.c
@@ -238,7 +238,7 @@ init(int argc, char *argv[])
 {
 	int retval;
 	const struct rte_memzone *mz;
-	uint16_t i, total_ports;
+	uint16_t i;
 
 	/* init EAL, parsing EAL args */
 	retval = rte_eal_init(argc, argv);
@@ -247,9 +247,6 @@ init(int argc, char *argv[])
 	argc -= retval;
 	argv += retval;
 
-	/* get total number of ports */
-	total_ports = rte_eth_dev_count_total();
-
 	/* set up array for port data */
 	mz = rte_memzone_reserve(MZ_PORT_INFO, sizeof(*ports),
 				rte_socket_id(), NO_FLAGS);
@@ -259,7 +256,7 @@ init(int argc, char *argv[])
 	ports = mz->addr;
 
 	/* parse additional, application arguments */
-	retval = parse_app_args(total_ports, argc, argv);
+	retval = parse_app_args(argc, argv);
 	if (retval != 0)
 		return -1;
 
-- 
2.20.1

[dpdk-dev] [PATCH v6 2/2] examples/multi_process/client_server_mp - fix crash in mp_client with sparse ports

[Stephen Hemminger]

From: Stephen Hemminger <sthemmin@microsoft.com>

The mp_client crashes if run on Azure or any system where ethdev
ports are owned. In that case, the tx_buffer and tx_stats for the
real port were initialized correctly, but the wrong port was used.

For example if the server has Ports 3 and 5. Then calling
rte_eth_tx_buffer_flush on any other buffer will dereference null
because the tx buffer for that port was not allocated.

Also:
   - the flush code is common enough that it should not be marked
     unlikely
   - combine conditions to reduce indentation
   - avoid unnecessary if() if sent is zero.

Fixes: e2366e74e029 ("examples: use buffered Tx")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 .../client_server_mp/mp_client/client.c        | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_client/client.c b/examples/multi_process/client_server_mp/mp_client/client.c
index c23dd3f378f7..361d90b54b2d 100644
--- a/examples/multi_process/client_server_mp/mp_client/client.c
+++ b/examples/multi_process/client_server_mp/mp_client/client.c
@@ -246,19 +246,19 @@ main(int argc, char *argv[])
 
 	for (;;) {
 		uint16_t i, rx_pkts;
-		uint16_t port;
 
 		rx_pkts = rte_ring_dequeue_burst(rx_ring, pkts,
 				PKT_READ_SIZE, NULL);
 
-		if (unlikely(rx_pkts == 0)){
-			if (need_flush)
-				for (port = 0; port < ports->num_ports; port++) {
-					sent = rte_eth_tx_buffer_flush(ports->id[port], client_id,
-							tx_buffer[port]);
-					if (unlikely(sent))
-						tx_stats->tx[port] += sent;
-				}
+		if (rx_pkts == 0 && need_flush) {
+			for (i = 0; i < ports->num_ports; i++) {
+				uint16_t port = ports->id[i];
+
+				sent = rte_eth_tx_buffer_flush(port,
+							       client_id,
+							       tx_buffer[port]);
+				tx_stats->tx[port] += sent;
+			}
 			need_flush = 0;
 			continue;
 		}
-- 
2.20.1

Re: [dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Matan Azrad]

Hi Stephen

From: Stephen Hemminger
> On Fri, 2 Aug 2019 05:33:20 +0000
> Matan Azrad <matan@mellanox.com> wrote:
> 
> > Hi Stephen
> >
> > One more small  comment inline
> >
> > From:  Stephen Hemminger
> > > Sent: Friday, August 2, 2019 5:58 AM
> > > To: dev@dpdk.org
> > > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > > Subject: [dpdk-dev] [PATCH v5 1/4]
> > > examples/multi_process/client_server_mp: check port validity
> > >
> > > From: Stephen Hemminger <sthemmin@microsoft.com>
> > >
> > > The mp_server incorrectly allows a port mask that included hidden
> > > ports and which later caused either lost packets or failed initialization.
> > >
> > > This fixes explicitly checking that each bit in portmask is a valid
> > > port before using it.
> > >
> > > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > > ---
> > >  .../client_server_mp/mp_server/args.c         | 35 ++++++++++---------
> > >  .../client_server_mp/mp_server/args.h         |  2 +-
> > >  .../client_server_mp/mp_server/init.c         |  7 ++--
> > >  3 files changed, 22 insertions(+), 22 deletions(-)
> > >
> > > diff --git
> > > a/examples/multi_process/client_server_mp/mp_server/args.c
> > > b/examples/multi_process/client_server_mp/mp_server/args.c
> > > index b0d8d7665c85..fdc008b3d677 100644
> > > --- a/examples/multi_process/client_server_mp/mp_server/args.c
> > > +++ b/examples/multi_process/client_server_mp/mp_server/args.c
> > > @@ -10,6 +10,7 @@
> > >  #include <errno.h>
> > >
> > >  #include <rte_memory.h>
> > > +#include <rte_ethdev.h>
> > >  #include <rte_string_fns.h>
> > >
> > >  #include "common.h"
> > > @@ -41,31 +42,33 @@ usage(void)
> > >   * array variable
> > >   */
> > >  static int
> > > -parse_portmask(uint8_t max_ports, const char *portmask)
> > > +parse_portmask(const char *portmask)
> > >  {
> > >  	char *end = NULL;
> > >  	unsigned long pm;
> > > -	uint16_t count = 0;
> > > +	uint16_t id;
> > >
> > >  	if (portmask == NULL || *portmask == '\0')
> > >  		return -1;
> > >
> > >  	/* convert parameter to a number and verify */
> > >  	pm = strtoul(portmask, &end, 16);
> > > -	if (end == NULL || *end != '\0' || pm == 0)
> > > +	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
> >
> > Why pm > UINT16_MAX ? should be something like > (1 <<
> RTE_MAX_ETHPORTS) - 1.
> > And need to be sure pm type can hold RTE_MAX_ETHPORTS bits,
> otherwise port 0 may unlikely be all the time visible in the loop below.
> >
> 
> The DPDK assumes a lot of places that unsigned long will hold a port mask.

So, all are bugs, no?

> If some extra bits are set, the error is visible later when the bits are leftover
> after finding ports.

Yes, but if there is a valid port which its port id is bigger than the portmask bits number - port 0 will be all the time visible in the check -> bug.

> The original code had worse problems, it would not catch invalid pm values at
> all and truncate silently.

Yes, maybe, but I really don't understand why you chose to limit for 16 ports, where this number come from?
So, my approach here, 2 options:

1. Remove this line change at all.
2. Do the portmask check bug-free.

Matan
 

Re: [dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

On Sun, 4 Aug 2019 08:31:54 +0000
Matan Azrad <matan@mellanox.com> wrote:

> > > >  	/* convert parameter to a number and verify */
> > > >  	pm = strtoul(portmask, &end, 16);
> > > > -	if (end == NULL || *end != '\0' || pm == 0)
> > > > +	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)  
> > >
> > > Why pm > UINT16_MAX ? should be something like > (1 <<  
> > RTE_MAX_ETHPORTS) - 1.  
> > > And need to be sure pm type can hold RTE_MAX_ETHPORTS bits,  
> > otherwise port 0 may unlikely be all the time visible in the loop below.  
> > >  
> > 
> > The DPDK assumes a lot of places that unsigned long will hold a port mask.  
> 
> So, all are bugs, no?

I don't think 32 bit build is that well tested. But yes a mask
needs to hold 64 ports.


> > If some extra bits are set, the error is visible later when the bits are leftover
> > after finding ports.  
> 
> Yes, but if there is a valid port which its port id is bigger than the portmask bits number - port 0 will be all the time visible in the check -> bug.
> 
> > The original code had worse problems, it would not catch invalid pm values at
> > all and truncate silently.  
> 
> Yes, maybe, but I really don't understand why you chose to limit for 16 ports, where this number come from?
> So, my approach here, 2 options:

The problem  here was my mistake for not having wide enough portmask.

[dpdk-dev] [PATCH v7 0/2] examples/client_server_mp: port id (fixes only)

[Stephen Hemminger]

v7 - widen port mask to allow 64 ports

v6 - just include fixes; cleanups can wait until 19.11

v5 - change logic in server_mp for evaluating port mask

v4 - fix checkpatch warning
     add patches to fix style issues and use ether format addr

v3 - merge both patches in one series
     use alternative algorithm to check port ownership (N^2)
     because reviewer didn't like direct check.

Stephen Hemminger (2):
  examples/multi_process/client_server_mp: check port validity
  examples/multi_process/client_server_mp - fix crash in mp_client with
    sparse ports

 .../client_server_mp/mp_client/client.c       | 18 ++++-----
 .../client_server_mp/mp_server/args.c         | 40 ++++++++++---------
 .../client_server_mp/mp_server/args.h         |  2 +-
 .../client_server_mp/mp_server/init.c         |  7 +---
 4 files changed, 34 insertions(+), 33 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v7 1/2] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

From: Stephen Hemminger <sthemmin@microsoft.com>

The mp_server incorrectly allows a port mask that included hidden
ports and which later caused either lost packets or failed initialization.

This fixes explicitly checking that each bit in portmask is a
valid port before using it.

Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 .../client_server_mp/mp_server/args.c         | 40 ++++++++++---------
 .../client_server_mp/mp_server/args.h         |  2 +-
 .../client_server_mp/mp_server/init.c         |  7 +---
 3 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_server/args.c b/examples/multi_process/client_server_mp/mp_server/args.c
index b0d8d7665c85..3c2ca266b096 100644
--- a/examples/multi_process/client_server_mp/mp_server/args.c
+++ b/examples/multi_process/client_server_mp/mp_server/args.c
@@ -10,6 +10,7 @@
 #include <errno.h>
 
 #include <rte_memory.h>
+#include <rte_ethdev.h>
 #include <rte_string_fns.h>
 
 #include "common.h"
@@ -41,31 +42,34 @@ usage(void)
  * array variable
  */
 static int
-parse_portmask(uint8_t max_ports, const char *portmask)
+parse_portmask(const char *portmask)
 {
 	char *end = NULL;
-	unsigned long pm;
-	uint16_t count = 0;
+	unsigned long long pm;
+	uint16_t id;
 
 	if (portmask == NULL || *portmask == '\0')
 		return -1;
 
 	/* convert parameter to a number and verify */
-	pm = strtoul(portmask, &end, 16);
-	if (end == NULL || *end != '\0' || pm == 0)
+	errno = 0;
+	pm = strtoull(portmask, &end, 16);
+	if (errno != 0 || end == NULL || *end != '\0')
 		return -1;
 
-	/* loop through bits of the mask and mark ports */
-	while (pm != 0){
-		if (pm & 0x01){ /* bit is set in mask, use port */
-			if (count >= max_ports)
-				printf("WARNING: requested port %u not present"
-				" - ignoring\n", (unsigned)count);
-			else
-			    ports->id[ports->num_ports++] = count;
-		}
-		pm = (pm >> 1);
-		count++;
+	RTE_ETH_FOREACH_DEV(id) {
+		unsigned long msk = 1u << id;
+
+		if ((pm & msk) == 0)
+			continue;
+
+		pm &= ~msk;
+		ports->id[ports->num_ports++] = id;
+	}
+
+	if (pm != 0) {
+		printf("WARNING: leftover ports in mask %#llx - ignoring\n",
+		       pm);
 	}
 
 	return 0;
@@ -99,7 +103,7 @@ parse_num_clients(const char *clients)
  * on error.
  */
 int
-parse_app_args(uint16_t max_ports, int argc, char *argv[])
+parse_app_args(int argc, char *argv[])
 {
 	int option_index, opt;
 	char **argvopt = argv;
@@ -112,7 +116,7 @@ parse_app_args(uint16_t max_ports, int argc, char *argv[])
 		&option_index)) != EOF){
 		switch (opt){
 			case 'p':
-				if (parse_portmask(max_ports, optarg) != 0){
+				if (parse_portmask(optarg) != 0) {
 					usage();
 					return -1;
 				}
diff --git a/examples/multi_process/client_server_mp/mp_server/args.h b/examples/multi_process/client_server_mp/mp_server/args.h
index 79c190a33a37..52c8cc86e6f0 100644
--- a/examples/multi_process/client_server_mp/mp_server/args.h
+++ b/examples/multi_process/client_server_mp/mp_server/args.h
@@ -5,6 +5,6 @@
 #ifndef _ARGS_H_
 #define _ARGS_H_
 
-int parse_app_args(uint16_t max_ports, int argc, char *argv[]);
+int parse_app_args(int argc, char *argv[]);
 
 #endif /* ifndef _ARGS_H_ */
diff --git a/examples/multi_process/client_server_mp/mp_server/init.c b/examples/multi_process/client_server_mp/mp_server/init.c
index 3af5dc6994bf..1b0569937b51 100644
--- a/examples/multi_process/client_server_mp/mp_server/init.c
+++ b/examples/multi_process/client_server_mp/mp_server/init.c
@@ -238,7 +238,7 @@ init(int argc, char *argv[])
 {
 	int retval;
 	const struct rte_memzone *mz;
-	uint16_t i, total_ports;
+	uint16_t i;
 
 	/* init EAL, parsing EAL args */
 	retval = rte_eal_init(argc, argv);
@@ -247,9 +247,6 @@ init(int argc, char *argv[])
 	argc -= retval;
 	argv += retval;
 
-	/* get total number of ports */
-	total_ports = rte_eth_dev_count_total();
-
 	/* set up array for port data */
 	mz = rte_memzone_reserve(MZ_PORT_INFO, sizeof(*ports),
 				rte_socket_id(), NO_FLAGS);
@@ -259,7 +256,7 @@ init(int argc, char *argv[])
 	ports = mz->addr;
 
 	/* parse additional, application arguments */
-	retval = parse_app_args(total_ports, argc, argv);
+	retval = parse_app_args(argc, argv);
 	if (retval != 0)
 		return -1;
 
-- 
2.20.1

[dpdk-dev] [PATCH v7 2/2] examples/multi_process/client_server_mp - fix crash in mp_client with sparse ports

[Stephen Hemminger]

From: Stephen Hemminger <sthemmin@microsoft.com>

The mp_client crashes if run on Azure or any system where ethdev
ports are owned. In that case, the tx_buffer and tx_stats for the
real port were initialized correctly, but the wrong port was used.

For example if the server has Ports 3 and 5. Then calling
rte_eth_tx_buffer_flush on any other buffer will dereference null
because the tx buffer for that port was not allocated.

Also:
   - the flush code is common enough that it should not be marked
     unlikely
   - combine conditions to reduce indentation
   - avoid unnecessary if() if sent is zero.

Fixes: e2366e74e029 ("examples: use buffered Tx")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 .../client_server_mp/mp_client/client.c        | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/examples/multi_process/client_server_mp/mp_client/client.c b/examples/multi_process/client_server_mp/mp_client/client.c
index c23dd3f378f7..361d90b54b2d 100644
--- a/examples/multi_process/client_server_mp/mp_client/client.c
+++ b/examples/multi_process/client_server_mp/mp_client/client.c
@@ -246,19 +246,19 @@ main(int argc, char *argv[])
 
 	for (;;) {
 		uint16_t i, rx_pkts;
-		uint16_t port;
 
 		rx_pkts = rte_ring_dequeue_burst(rx_ring, pkts,
 				PKT_READ_SIZE, NULL);
 
-		if (unlikely(rx_pkts == 0)){
-			if (need_flush)
-				for (port = 0; port < ports->num_ports; port++) {
-					sent = rte_eth_tx_buffer_flush(ports->id[port], client_id,
-							tx_buffer[port]);
-					if (unlikely(sent))
-						tx_stats->tx[port] += sent;
-				}
+		if (rx_pkts == 0 && need_flush) {
+			for (i = 0; i < ports->num_ports; i++) {
+				uint16_t port = ports->id[i];
+
+				sent = rte_eth_tx_buffer_flush(port,
+							       client_id,
+							       tx_buffer[port]);
+				tx_stats->tx[port] += sent;
+			}
 			need_flush = 0;
 			continue;
 		}
-- 
2.20.1

Re: [dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Matan Azrad]

From: Stephen Hemminger
> On Sun, 4 Aug 2019 08:31:54 +0000
> Matan Azrad <matan@mellanox.com> wrote:
> 
> > > > >  	/* convert parameter to a number and verify */
> > > > >  	pm = strtoul(portmask, &end, 16);
> > > > > -	if (end == NULL || *end != '\0' || pm == 0)
> > > > > +	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm
> == 0)
> > > >
> > > > Why pm > UINT16_MAX ? should be something like > (1 <<
> > > RTE_MAX_ETHPORTS) - 1.
> > > > And need to be sure pm type can hold RTE_MAX_ETHPORTS bits,
> > > otherwise port 0 may unlikely be all the time visible in the loop below.
> > > >
> > >
> > > The DPDK assumes a lot of places that unsigned long will hold a port
> mask.
> >
> > So, all are bugs, no?
> 
> I don't think 32 bit build is that well tested. But yes a mask needs to hold 64
> ports.

What if someone changes RTE_MAX_ETHPORTS to be bigger than 64 in config file?

Assume the user changes RTE_MAX_ETHPORTS to 128, and there is a valid port in range [64, 127].
Then,  assume the failsafe sub device owns port ID 0.

Because the mask bits are not enough to handle the above range, you will get port 0 as valid port - bug.

I think you need one more check to the RTE_MAX_ETHPORTS > 64 case. 


> > > If some extra bits are set, the error is visible later when the bits
> > > are leftover after finding ports.
> >
> > Yes, but if there is a valid port which its port id is bigger than the portmask
> bits number - port 0 will be all the time visible in the check -> bug.
> >
> > > The original code had worse problems, it would not catch invalid pm
> > > values at all and truncate silently.
> >
> > Yes, maybe, but I really don't understand why you chose to limit for 16
> ports, where this number come from?
> > So, my approach here, 2 options:
> 
> The problem  here was my mistake for not having wide enough portmask.

Re: [dpdk-dev] [PATCH v7 1/2] examples/multi_process/client_server_mp: check port validity

[Matan Azrad]

Hi


> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> Sent: Monday, August 5, 2019 7:38 PM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <sthemmin@microsoft.com>
> Subject: [dpdk-dev] [PATCH v7 1/2]
> examples/multi_process/client_server_mp: check port validity
> 
> From: Stephen Hemminger <sthemmin@microsoft.com>
> 
> The mp_server incorrectly allows a port mask that included hidden ports and
> which later caused either lost packets or failed initialization.
> 
> This fixes explicitly checking that each bit in portmask is a valid port before
> using it.
> 
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>  .../client_server_mp/mp_server/args.c         | 40 ++++++++++---------
>  .../client_server_mp/mp_server/args.h         |  2 +-
>  .../client_server_mp/mp_server/init.c         |  7 +---
>  3 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/examples/multi_process/client_server_mp/mp_server/args.c
> b/examples/multi_process/client_server_mp/mp_server/args.c
> index b0d8d7665c85..3c2ca266b096 100644
> --- a/examples/multi_process/client_server_mp/mp_server/args.c
> +++ b/examples/multi_process/client_server_mp/mp_server/args.c
> @@ -10,6 +10,7 @@
>  #include <errno.h>
> 
>  #include <rte_memory.h>
> +#include <rte_ethdev.h>
>  #include <rte_string_fns.h>
> 
>  #include "common.h"
> @@ -41,31 +42,34 @@ usage(void)
>   * array variable
>   */
>  static int
> -parse_portmask(uint8_t max_ports, const char *portmask)
> +parse_portmask(const char *portmask)
>  {
>  	char *end = NULL;
> -	unsigned long pm;
> -	uint16_t count = 0;
> +	unsigned long long pm;
> +	uint16_t id;
> 
>  	if (portmask == NULL || *portmask == '\0')
>  		return -1;
> 
>  	/* convert parameter to a number and verify */
> -	pm = strtoul(portmask, &end, 16);
> -	if (end == NULL || *end != '\0' || pm == 0)
> +	errno = 0;
> +	pm = strtoull(portmask, &end, 16);
> +	if (errno != 0 || end == NULL || *end != '\0')
>  		return -1;
> 
Please Continue discussion on this on V5 thread.

> -	/* loop through bits of the mask and mark ports */
> -	while (pm != 0){
> -		if (pm & 0x01){ /* bit is set in mask, use port */
> -			if (count >= max_ports)
> -				printf("WARNING: requested port %u not
> present"
> -				" - ignoring\n", (unsigned)count);
> -			else
> -			    ports->id[ports->num_ports++] = count;
> -		}
> -		pm = (pm >> 1);
> -		count++;
> +	RTE_ETH_FOREACH_DEV(id) {
> +		unsigned long msk = 1u << id;
> +
> +		if ((pm & msk) == 0)
> +			continue;
> +
> +		pm &= ~msk;
> +		ports->id[ports->num_ports++] = id;
> +	}
> +
> +	if (pm != 0) {
> +		printf("WARNING: leftover ports in mask %#llx - ignoring\n",
> +		       pm);
>  	}
> 
>  	return 0;
> @@ -99,7 +103,7 @@ parse_num_clients(const char *clients)
>   * on error.
>   */
>  int
> -parse_app_args(uint16_t max_ports, int argc, char *argv[])
> +parse_app_args(int argc, char *argv[])
>  {
>  	int option_index, opt;
>  	char **argvopt = argv;
> @@ -112,7 +116,7 @@ parse_app_args(uint16_t max_ports, int argc, char
> *argv[])
>  		&option_index)) != EOF){
>  		switch (opt){
>  			case 'p':
> -				if (parse_portmask(max_ports, optarg) != 0){
> +				if (parse_portmask(optarg) != 0) {
>  					usage();
>  					return -1;
>  				}
> diff --git a/examples/multi_process/client_server_mp/mp_server/args.h
> b/examples/multi_process/client_server_mp/mp_server/args.h
> index 79c190a33a37..52c8cc86e6f0 100644
> --- a/examples/multi_process/client_server_mp/mp_server/args.h
> +++ b/examples/multi_process/client_server_mp/mp_server/args.h
> @@ -5,6 +5,6 @@
>  #ifndef _ARGS_H_
>  #define _ARGS_H_
> 
> -int parse_app_args(uint16_t max_ports, int argc, char *argv[]);
> +int parse_app_args(int argc, char *argv[]);
> 
>  #endif /* ifndef _ARGS_H_ */
> diff --git a/examples/multi_process/client_server_mp/mp_server/init.c
> b/examples/multi_process/client_server_mp/mp_server/init.c
> index 3af5dc6994bf..1b0569937b51 100644
> --- a/examples/multi_process/client_server_mp/mp_server/init.c
> +++ b/examples/multi_process/client_server_mp/mp_server/init.c
> @@ -238,7 +238,7 @@ init(int argc, char *argv[])  {
>  	int retval;
>  	const struct rte_memzone *mz;
> -	uint16_t i, total_ports;
> +	uint16_t i;
> 
>  	/* init EAL, parsing EAL args */
>  	retval = rte_eal_init(argc, argv);
> @@ -247,9 +247,6 @@ init(int argc, char *argv[])
>  	argc -= retval;
>  	argv += retval;
> 
> -	/* get total number of ports */
> -	total_ports = rte_eth_dev_count_total();
> -
>  	/* set up array for port data */
>  	mz = rte_memzone_reserve(MZ_PORT_INFO, sizeof(*ports),
>  				rte_socket_id(), NO_FLAGS);
> @@ -259,7 +256,7 @@ init(int argc, char *argv[])
>  	ports = mz->addr;
> 
>  	/* parse additional, application arguments */
> -	retval = parse_app_args(total_ports, argc, argv);
> +	retval = parse_app_args(argc, argv);
>  	if (retval != 0)
>  		return -1;
> 
> --
> 2.20.1

Re: [dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

On Tue, 6 Aug 2019 08:19:01 +0000
Matan Azrad <matan@mellanox.com> wrote:

> From: Stephen Hemminger
> > On Sun, 4 Aug 2019 08:31:54 +0000
> > Matan Azrad <matan@mellanox.com> wrote:
> >   
> > > > > >  	/* convert parameter to a number and verify */
> > > > > >  	pm = strtoul(portmask, &end, 16);
> > > > > > -	if (end == NULL || *end != '\0' || pm == 0)
> > > > > > +	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm  
> > == 0)  
> > > > >
> > > > > Why pm > UINT16_MAX ? should be something like > (1 <<  
> > > > RTE_MAX_ETHPORTS) - 1.  
> > > > > And need to be sure pm type can hold RTE_MAX_ETHPORTS bits,  
> > > > otherwise port 0 may unlikely be all the time visible in the loop below.  
> > > > >  
> > > >
> > > > The DPDK assumes a lot of places that unsigned long will hold a port  
> > mask.  
> > >
> > > So, all are bugs, no?  
> > 
> > I don't think 32 bit build is that well tested. But yes a mask needs to hold 64
> > ports.  
> 
> What if someone changes RTE_MAX_ETHPORTS to be bigger than 64 in config file?
> 
> Assume the user changes RTE_MAX_ETHPORTS to 128, and there is a valid port in range [64, 127].
> Then,  assume the failsafe sub device owns port ID 0.
> 
> Because the mask bits are not enough to handle the above range, you will get port 0 as valid port - bug.
> 
> I think you need one more check to the RTE_MAX_ETHPORTS > 64 case. 

Not really needed.

The DPDK has lots of hard coded assumptions of all ports fitting in 64 bits.
Examples include testpmd/parameters.c etc.

The original concept of a small set of assigned values for portid is not going
to scale. It really should have been more like ifindex; something that is not
used by common API's much larger range; and assigned purely sequentially.

The API's should all be using names, but the DPDK port naming is also a mess...

Re: [dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Matan Azrad]

Hi

From: Stephen Hemminger
> Sent: Tuesday, August 6, 2019 6:40 PM
> To: Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> Subject: Re: [dpdk-dev] [PATCH v5 1/4]
> examples/multi_process/client_server_mp: check port validity
> 
> On Tue, 6 Aug 2019 08:19:01 +0000
> Matan Azrad <matan@mellanox.com> wrote:
> 
> > From: Stephen Hemminger
> > > On Sun, 4 Aug 2019 08:31:54 +0000
> > > Matan Azrad <matan@mellanox.com> wrote:
> > >
> > > > > > >  	/* convert parameter to a number and verify */
> > > > > > >  	pm = strtoul(portmask, &end, 16);
> > > > > > > -	if (end == NULL || *end != '\0' || pm == 0)
> > > > > > > +	if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm
> > > == 0)
> > > > > >
> > > > > > Why pm > UINT16_MAX ? should be something like > (1 <<
> > > > > RTE_MAX_ETHPORTS) - 1.
> > > > > > And need to be sure pm type can hold RTE_MAX_ETHPORTS bits,
> > > > > otherwise port 0 may unlikely be all the time visible in the loop below.
> > > > > >
> > > > >
> > > > > The DPDK assumes a lot of places that unsigned long will hold a
> > > > > port
> > > mask.
> > > >
> > > > So, all are bugs, no?
> > >
> > > I don't think 32 bit build is that well tested. But yes a mask needs
> > > to hold 64 ports.
> >
> > What if someone changes RTE_MAX_ETHPORTS to be bigger than 64 in
> config file?
> >
> > Assume the user changes RTE_MAX_ETHPORTS to 128, and there is a valid
> port in range [64, 127].
> > Then,  assume the failsafe sub device owns port ID 0.
> >
> > Because the mask bits are not enough to handle the above range, you will
> get port 0 as valid port - bug.
> >
> > I think you need one more check to the RTE_MAX_ETHPORTS > 64 case.
> 
> Not really needed.
> 
> The DPDK has lots of hard coded assumptions of all ports fitting in 64 bits.
> Examples include testpmd/parameters.c etc.

Yes, I understand, but the user should know not to change the default value of 
RTE_MAX_ETHPORTS, at least it should be documented. 

> The original concept of a small set of assigned values for portid is not going to
> scale. It really should have been more like ifindex; something that is not used
> by common API's much larger range; and assigned purely sequentially.
> 
> The API's should all be using names, but the DPDK port naming is also a
> mess...

Port ID is OK, user can run port info, then to find the wanted port ID and configure it by port id list\bitmap.

Re: [dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

On Tue, 6 Aug 2019 20:03:22 +0000
Matan Azrad <matan@mellanox.com> wrote:

> > 
> > The DPDK has lots of hard coded assumptions of all ports fitting in 64 bits.
> > Examples include testpmd/parameters.c etc.  
> 
> Yes, I understand, but the user should know not to change the default value of 
> RTE_MAX_ETHPORTS, at least it should be documented. 
> 
> > The original concept of a small set of assigned values for portid is not going to
> > scale. It really should have been more like ifindex; something that is not used
> > by common API's much larger range; and assigned purely sequentially.
> > 
> > The API's should all be using names, but the DPDK port naming is also a
> > mess...  
> 
> Port ID is OK, user can run port info, then to find the wanted port ID and configure it by port id list\bitmap.
> 


The examples are toy programs. If user changes RTE_MAX_ETHPORTS it will break
lots of other places. Why put more checks in the examples. Sorry, it really
would not help to pretend that fixing the example is going to help this.

The original code was broken with behavior introduced with port ownership
and these patches try to help fix that.

Still think Port ID is a lousy API for real applications. It is fine for the
DPDK literati but terrible user experience for the average user.
It requires too hands on an experience. For example, the port id values change in
non-obvious ways when using port ownership like failsafe does.

Re: [dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Matan Azrad]

From: Stephen Hemminger 
> Sent: Wednesday, August 7, 2019 2:09 AM
> To: Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> Subject: Re: [dpdk-dev] [PATCH v5 1/4]
> examples/multi_process/client_server_mp: check port validity
> 
> On Tue, 6 Aug 2019 20:03:22 +0000
> Matan Azrad <matan@mellanox.com> wrote:
> 
> > >
> > > The DPDK has lots of hard coded assumptions of all ports fitting in 64 bits.
> > > Examples include testpmd/parameters.c etc.
> >
> > Yes, I understand, but the user should know not to change the default
> > value of RTE_MAX_ETHPORTS, at least it should be documented.
> >
> > > The original concept of a small set of assigned values for portid is
> > > not going to scale. It really should have been more like ifindex;
> > > something that is not used by common API's much larger range; and
> assigned purely sequentially.
> > >
> > > The API's should all be using names, but the DPDK port naming is
> > > also a mess...
> >
> > Port ID is OK, user can run port info, then to find the wanted port ID and
> configure it by port id list\bitmap.
> >
> 
> 
> The examples are toy programs. If user changes RTE_MAX_ETHPORTS it will
> break lots of other places. Why put more checks in the examples. Sorry, it
> really would not help to pretend that fixing the example is going to help this.


Agree that it is not needed to fix all the places now.
It is better just to update the example documentation that RTE_MAX_ETHPORTS must not be changed when running this application.

I will ack your series(v7) , Consider to update the doc if you want to be completely perfect here.  

Re: [dpdk-dev] [PATCH v7 0/2] examples/client_server_mp: port id (fixes only)

[Matan Azrad]

Acked-by: Matan Azrad <matan@mellanox.com>

From:  Stephen Hemminger
> Sent: Monday, August 5, 2019 7:38 PM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Subject: [dpdk-dev] [PATCH v7 0/2] examples/client_server_mp: port id
> (fixes only)
> 
> v7 - widen port mask to allow 64 ports
> 
> v6 - just include fixes; cleanups can wait until 19.11
> 
> v5 - change logic in server_mp for evaluating port mask
> 
> v4 - fix checkpatch warning
>      add patches to fix style issues and use ether format addr
> 
> v3 - merge both patches in one series
>      use alternative algorithm to check port ownership (N^2)
>      because reviewer didn't like direct check.
> 
> Stephen Hemminger (2):
>   examples/multi_process/client_server_mp: check port validity
>   examples/multi_process/client_server_mp - fix crash in mp_client with
>     sparse ports
> 
>  .../client_server_mp/mp_client/client.c       | 18 ++++-----
>  .../client_server_mp/mp_server/args.c         | 40 ++++++++++---------
>  .../client_server_mp/mp_server/args.h         |  2 +-
>  .../client_server_mp/mp_server/init.c         |  7 +---
>  4 files changed, 34 insertions(+), 33 deletions(-)
> 
> --
> 2.20.1

Re: [dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

On Wed, 7 Aug 2019 05:38:42 +0000
Matan Azrad <matan@mellanox.com> wrote:

> From: Stephen Hemminger 
> > Sent: Wednesday, August 7, 2019 2:09 AM
> > To: Matan Azrad <matan@mellanox.com>
> > Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> > Subject: Re: [dpdk-dev] [PATCH v5 1/4]
> > examples/multi_process/client_server_mp: check port validity
> > 
> > On Tue, 6 Aug 2019 20:03:22 +0000
> > Matan Azrad <matan@mellanox.com> wrote:
> >   
> > > >
> > > > The DPDK has lots of hard coded assumptions of all ports fitting in 64 bits.
> > > > Examples include testpmd/parameters.c etc.  
> > >
> > > Yes, I understand, but the user should know not to change the default
> > > value of RTE_MAX_ETHPORTS, at least it should be documented.
> > >  
> > > > The original concept of a small set of assigned values for portid is
> > > > not going to scale. It really should have been more like ifindex;
> > > > something that is not used by common API's much larger range; and  
> > assigned purely sequentially.  
> > > >
> > > > The API's should all be using names, but the DPDK port naming is
> > > > also a mess...  
> > >
> > > Port ID is OK, user can run port info, then to find the wanted port ID and  
> > configure it by port id list\bitmap.  
> > >  
> > 
> > 
> > The examples are toy programs. If user changes RTE_MAX_ETHPORTS it will
> > break lots of other places. Why put more checks in the examples. Sorry, it
> > really would not help to pretend that fixing the example is going to help this.  
> 
> 
> Agree that it is not needed to fix all the places now.
> It is better just to update the example documentation that RTE_MAX_ETHPORTS must not be changed when running this application.
> 
> I will ack your series(v7) , Consider to update the doc if you want to be completely perfect here.  

Perhaps the right place to tell the users is somewhere in the documentation?

One place would be here:

diff --git a/doc/guides/faq/faq.rst b/doc/guides/faq/faq.rst
index f19c1389b6af..a847d9ceda22 100644
--- a/doc/guides/faq/faq.rst
+++ b/doc/guides/faq/faq.rst
@@ -195,3 +195,8 @@ Why can't my application receive packets on my system with UEFI Secure Boot enab
 
 If UEFI secure boot is enabled, the Linux kernel may disallow the use of UIO on the system.
 Therefore, devices for use by DPDK should be bound to the ``vfio-pci`` kernel module rather than ``igb_uio`` or ``uio_pci_generic``.
+
+What is the maximum number of ethernet devices?
+-----------------------------------------------
+
+The limit on the number of Ethernet devices is controlled by the RTE_MAX_ETHPORTS configuration setting. Since many of the applications use a 64 bit value for port mask; the current upper limit is 64 ports.

Re: [dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Matan Azrad]

From: Stephen Hemminger
> On Wed, 7 Aug 2019 05:38:42 +0000
> Matan Azrad <matan@mellanox.com> wrote:
> 
> > From: Stephen Hemminger
> > > Sent: Wednesday, August 7, 2019 2:09 AM
> > > To: Matan Azrad <matan@mellanox.com>
> > > Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> > > Subject: Re: [dpdk-dev] [PATCH v5 1/4]
> > > examples/multi_process/client_server_mp: check port validity
> > >
> > > On Tue, 6 Aug 2019 20:03:22 +0000
> > > Matan Azrad <matan@mellanox.com> wrote:
> > >
> > > > >
> > > > > The DPDK has lots of hard coded assumptions of all ports fitting in 64
> bits.
> > > > > Examples include testpmd/parameters.c etc.
> > > >
> > > > Yes, I understand, but the user should know not to change the
> > > > default value of RTE_MAX_ETHPORTS, at least it should be
> documented.
> > > >
> > > > > The original concept of a small set of assigned values for
> > > > > portid is not going to scale. It really should have been more
> > > > > like ifindex; something that is not used by common API's much
> > > > > larger range; and
> > > assigned purely sequentially.
> > > > >
> > > > > The API's should all be using names, but the DPDK port naming is
> > > > > also a mess...
> > > >
> > > > Port ID is OK, user can run port info, then to find the wanted
> > > > port ID and
> > > configure it by port id list\bitmap.
> > > >
> > >
> > >
> > > The examples are toy programs. If user changes RTE_MAX_ETHPORTS it
> > > will break lots of other places. Why put more checks in the
> > > examples. Sorry, it really would not help to pretend that fixing the
> example is going to help this.
> >
> >
> > Agree that it is not needed to fix all the places now.
> > It is better just to update the example documentation that
> RTE_MAX_ETHPORTS must not be changed when running this application.
> >
> > I will ack your series(v7) , Consider to update the doc if you want to be
> completely perfect here.
> 
> Perhaps the right place to tell the users is somewhere in the documentation?
> 
> One place would be here:
> 
> diff --git a/doc/guides/faq/faq.rst b/doc/guides/faq/faq.rst index
> f19c1389b6af..a847d9ceda22 100644
> --- a/doc/guides/faq/faq.rst
> +++ b/doc/guides/faq/faq.rst
> @@ -195,3 +195,8 @@ Why can't my application receive packets on my
> system with UEFI Secure Boot enab
> 
>  If UEFI secure boot is enabled, the Linux kernel may disallow the use of UIO
> on the system.
>  Therefore, devices for use by DPDK should be bound to the ``vfio-pci``
> kernel module rather than ``igb_uio`` or ``uio_pci_generic``.
> +
> +What is the maximum number of ethernet devices?
> +-----------------------------------------------
> +
> +The limit on the number of Ethernet devices is controlled by the
> RTE_MAX_ETHPORTS configuration setting. Since many of the applications
> use a 64 bit value for port mask; the current upper limit is 64 ports.
> 
I think there are systems with a lot of virtual ports which may use more than 64.

So update all the docs when the mask is defined, would be option too.

Re: [dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

On Wed, 7 Aug 2019 07:02:02 +0000
Matan Azrad <matan@mellanox.com> wrote:

> From: Stephen Hemminger
> > On Wed, 7 Aug 2019 05:38:42 +0000
> > Matan Azrad <matan@mellanox.com> wrote:
> >   
> > > From: Stephen Hemminger  
> > > > Sent: Wednesday, August 7, 2019 2:09 AM
> > > > To: Matan Azrad <matan@mellanox.com>
> > > > Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> > > > Subject: Re: [dpdk-dev] [PATCH v5 1/4]
> > > > examples/multi_process/client_server_mp: check port validity
> > > >
> > > > On Tue, 6 Aug 2019 20:03:22 +0000
> > > > Matan Azrad <matan@mellanox.com> wrote:
> > > >  
> > > > > >
> > > > > > The DPDK has lots of hard coded assumptions of all ports fitting in 64  
> > bits.  
> > > > > > Examples include testpmd/parameters.c etc.  
> > > > >
> > > > > Yes, I understand, but the user should know not to change the
> > > > > default value of RTE_MAX_ETHPORTS, at least it should be  
> > documented.  
> > > > >  
> > > > > > The original concept of a small set of assigned values for
> > > > > > portid is not going to scale. It really should have been more
> > > > > > like ifindex; something that is not used by common API's much
> > > > > > larger range; and  
> > > > assigned purely sequentially.  
> > > > > >
> > > > > > The API's should all be using names, but the DPDK port naming is
> > > > > > also a mess...  
> > > > >
> > > > > Port ID is OK, user can run port info, then to find the wanted
> > > > > port ID and  
> > > > configure it by port id list\bitmap.  
> > > > >  
> > > >
> > > >
> > > > The examples are toy programs. If user changes RTE_MAX_ETHPORTS it
> > > > will break lots of other places. Why put more checks in the
> > > > examples. Sorry, it really would not help to pretend that fixing the  
> > example is going to help this.  
> > >
> > >
> > > Agree that it is not needed to fix all the places now.
> > > It is better just to update the example documentation that  
> > RTE_MAX_ETHPORTS must not be changed when running this application.  
> > >
> > > I will ack your series(v7) , Consider to update the doc if you want to be  
> > completely perfect here.
> > 
> > Perhaps the right place to tell the users is somewhere in the documentation?
> > 
> > One place would be here:
> > 
> > diff --git a/doc/guides/faq/faq.rst b/doc/guides/faq/faq.rst index
> > f19c1389b6af..a847d9ceda22 100644
> > --- a/doc/guides/faq/faq.rst
> > +++ b/doc/guides/faq/faq.rst
> > @@ -195,3 +195,8 @@ Why can't my application receive packets on my
> > system with UEFI Secure Boot enab
> > 
> >  If UEFI secure boot is enabled, the Linux kernel may disallow the use of UIO
> > on the system.
> >  Therefore, devices for use by DPDK should be bound to the ``vfio-pci``
> > kernel module rather than ``igb_uio`` or ``uio_pci_generic``.
> > +
> > +What is the maximum number of ethernet devices?
> > +-----------------------------------------------
> > +
> > +The limit on the number of Ethernet devices is controlled by the
> > RTE_MAX_ETHPORTS configuration setting. Since many of the applications
> > use a 64 bit value for port mask; the current upper limit is 64 ports.
> >   
> I think there are systems with a lot of virtual ports which may use more than 64.
> 
> So update all the docs when the mask is defined, would be option too.

It would be good if (ie someone should do it but I don't have time);
to have a new type "port_set"  which is a variable length bit mask

It could be backwards compatible with existing usage.
Something like existing cpuset command format.

       Examples of the Mask Format:

           00000001                        # just bit 0 set
           40000000,00000000,00000000      # just bit 94 set
           00000001,00000000,00000000      # just bit 64 set
           000000ff,00000000               # bits 32-39 set
           00000000,000e3862               # 1,5,6,11-13,17-19 set

Re: [dpdk-dev] [PATCH v7 1/2] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger]

On Tue, 6 Aug 2019 12:07:25 +0000
Matan Azrad <matan@mellanox.com> wrote:

> Hi
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> > Sent: Monday, August 5, 2019 7:38 PM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > Subject: [dpdk-dev] [PATCH v7 1/2]
> > examples/multi_process/client_server_mp: check port validity
> > 
> > From: Stephen Hemminger <sthemmin@microsoft.com>
> > 
> > The mp_server incorrectly allows a port mask that included hidden ports and
> > which later caused either lost packets or failed initialization.
> > 
> > This fixes explicitly checking that each bit in portmask is a valid port before
> > using it.
> > 
> > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > ---
> >  .../client_server_mp/mp_server/args.c         | 40 ++++++++++---------
> >  .../client_server_mp/mp_server/args.h         |  2 +-
> >  .../client_server_mp/mp_server/init.c         |  7 +---
> >  3 files changed, 25 insertions(+), 24 deletions(-)
> > 
> > diff --git a/examples/multi_process/client_server_mp/mp_server/args.c
> > b/examples/multi_process/client_server_mp/mp_server/args.c
> > index b0d8d7665c85..3c2ca266b096 100644
> > --- a/examples/multi_process/client_server_mp/mp_server/args.c
> > +++ b/examples/multi_process/client_server_mp/mp_server/args.c
> > @@ -10,6 +10,7 @@
> >  #include <errno.h>
> > 
> >  #include <rte_memory.h>
> > +#include <rte_ethdev.h>
> >  #include <rte_string_fns.h>
> > 
> >  #include "common.h"
> > @@ -41,31 +42,34 @@ usage(void)
> >   * array variable
> >   */
> >  static int
> > -parse_portmask(uint8_t max_ports, const char *portmask)
> > +parse_portmask(const char *portmask)
> >  {
> >  	char *end = NULL;
> > -	unsigned long pm;
> > -	uint16_t count = 0;
> > +	unsigned long long pm;
> > +	uint16_t id;
> > 
> >  	if (portmask == NULL || *portmask == '\0')
> >  		return -1;
> > 
> >  	/* convert parameter to a number and verify */
> > -	pm = strtoul(portmask, &end, 16);
> > -	if (end == NULL || *end != '\0' || pm == 0)
> > +	errno = 0;
> > +	pm = strtoull(portmask, &end, 16);
> > +	if (errno != 0 || end == NULL || *end != '\0')
> >  		return -1;
> >   
> Please Continue discussion on this on V5 thread.

The V5 thread degenerated into "applications should not use portmask".
That is a valid discussion but out of scope for this patch which is a bug
fix for users.

Re: [dpdk-dev] [PATCH v7 0/2] examples/client_server_mp: port id (fixes only)

[Thomas Monjalon]

07/08/2019 07:40, Matan Azrad:
> 
> Acked-by: Matan Azrad <matan@mellanox.com>
> 
> From:  Stephen Hemminger
> > Sent: Monday, August 5, 2019 7:38 PM
> > 
> > v7 - widen port mask to allow 64 ports
> > 
> > v6 - just include fixes; cleanups can wait until 19.11
> > 
> > v5 - change logic in server_mp for evaluating port mask
> > 
> > v4 - fix checkpatch warning
> >      add patches to fix style issues and use ether format addr
> > 
> > v3 - merge both patches in one series
> >      use alternative algorithm to check port ownership (N^2)
> >      because reviewer didn't like direct check.
> > 
> > Stephen Hemminger (2):
> >   examples/multi_process/client_server_mp: check port validity
> >   examples/multi_process/client_server_mp - fix crash in mp_client with
> >     sparse ports

Applied, thanks.

Sorry for having forgotten this patchset.