[v4] examples/l2fwd: add cmdline option for forwarding port info

  From: Vamsi Attunuru <vattunuru@marvell.com>

Current l2fwd application statically configures adjacent ports as
destination ports for forwarding the traffic.

Add a portmap option to pass the forwarding port pair mapping which allows
the user to configure forwarding port mapping.

If no portmap argument is specified, destination port map is not
changed and traffic gets forwarded with existing mapping.

To align port/queue configuration of each lcore with destination port
map, port/queue configuration of each lcore gets modified when portmap
option is specified.

Ex: ./l2fwd -c 0xff -- -p 0x3f -q 2 --portmap="(0,3)(1,4)(2,5)"

With above portmap option, traffic received from portid = 0 gets forwarded
to port = 3 and vice versa, similarly traffic gets forwarded on other port
pairs (1,4) and (2,5)

Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
Acked-by: Andrzej Ostruszka <aostruszka@marvell.com>
---
 v4 Changes:
 * Documentation changes. (Sunil)
 * reduce port_pair_params_array size. (Sunil)

 v3 Changes:
 * s/config/portmap/
 * rebase on master

 v2 Changes:
 * Fix command option format in docs.
 * Use memcpy instead of snprintf.
 * Rephrase if-else condition.

 doc/guides/rel_notes/release_20_05.rst        |   7 +
 .../sample_app_ug/l2_forward_real_virtual.rst |  17 +-
 examples/l2fwd/main.c                         | 182 +++++++++++++++---
 3 files changed, 180 insertions(+), 26 deletions(-)

--
2.17.1
  

Looks okay. 

Regards
Sunil Kumar Kori

>-----Original Message-----
>From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
>Sent: Tuesday, April 28, 2020 12:01 AM
>To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; thomas@monjalon.net;
>John McNamara <john.mcnamara@intel.com>; Marko Kovacevic
><marko.kovacevic@intel.com>; Ori Kam <orika@mellanox.com>; Bruce
>Richardson <bruce.richardson@intel.com>; Radu Nicolau
><radu.nicolau@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>; Tomasz
>Kantecki <tomasz.kantecki@intel.com>; Sunil Kumar Kori
><skori@marvell.com>; Pavan Nikhilesh Bhagavatula
><pbhagavatula@marvell.com>
>Cc: Andrzej Ostruszka [C] <aostruszka@marvell.com>; dev@dpdk.org; Vamsi
>Krishna Attunuru <vattunuru@marvell.com>
>Subject: [dpdk-dev] [PATCH v4] examples/l2fwd: add cmdline option for
>forwarding port info
>
>From: Vamsi Attunuru <vattunuru@marvell.com>
>
>Current l2fwd application statically configures adjacent ports as destination
>ports for forwarding the traffic.
>
>Add a portmap option to pass the forwarding port pair mapping which allows
>the user to configure forwarding port mapping.
>
>If no portmap argument is specified, destination port map is not changed and
>traffic gets forwarded with existing mapping.
>
>To align port/queue configuration of each lcore with destination port map,
>port/queue configuration of each lcore gets modified when portmap option is
>specified.
>
>Ex: ./l2fwd -c 0xff -- -p 0x3f -q 2 --portmap="(0,3)(1,4)(2,5)"
>
>With above portmap option, traffic received from portid = 0 gets forwarded to
>port = 3 and vice versa, similarly traffic gets forwarded on other port pairs
>(1,4) and (2,5)
>
>Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
>Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>Acked-by: Andrzej Ostruszka <aostruszka@marvell.com>
Acked-by: Sunil Kumar Kori <skori@marvell.com>

>---
> v4 Changes:
> * Documentation changes. (Sunil)
> * reduce port_pair_params_array size. (Sunil)
>
> v3 Changes:
> * s/config/portmap/
> * rebase on master
>
> v2 Changes:
> * Fix command option format in docs.
> * Use memcpy instead of snprintf.
> * Rephrase if-else condition.
>
> doc/guides/rel_notes/release_20_05.rst        |   7 +
> .../sample_app_ug/l2_forward_real_virtual.rst |  17 +-
> examples/l2fwd/main.c                         | 182 +++++++++++++++---
> 3 files changed, 180 insertions(+), 26 deletions(-)
>
>diff --git a/doc/guides/rel_notes/release_20_05.rst
>b/doc/guides/rel_notes/release_20_05.rst
>index b124c3f28..019bec5d7 100644
>--- a/doc/guides/rel_notes/release_20_05.rst
>+++ b/doc/guides/rel_notes/release_20_05.rst
>@@ -212,6 +212,13 @@ New Features
>   * Added IPsec inbound load-distribution support for ipsec-secgw application
>     using NIC load distribution feature(Flow Director).
>
>+* **Added --portmap command line parameter to l2fwd example.**
>+
>+  Added new command line option ``--portmap="(port, port)[,(port,
>+ port)]"`` to  pass forwarding port details.
>+  See the :doc:`doc/guides/sample_app_ug/l2_forward_real_virtual` for
>+ more  details of this parameter usage.
>+
>
> Removed Items
> -------------
>diff --git a/doc/guides/sample_app_ug/l2_forward_real_virtual.rst
>b/doc/guides/sample_app_ug/l2_forward_real_virtual.rst
>index 39d6b0067..90ca609d6 100644
>--- a/doc/guides/sample_app_ug/l2_forward_real_virtual.rst
>+++ b/doc/guides/sample_app_ug/l2_forward_real_virtual.rst
>@@ -91,7 +91,10 @@ The application requires a number of command line
>options:
>
> .. code-block:: console
>
>-    ./build/l2fwd [EAL options] -- -p PORTMASK [-q NQ] --[no-]mac-updating
>+    ./build/l2fwd [EAL options] -- -p PORTMASK
>+                                   [-q NQ]
>+                                   --[no-]mac-updating
>+                                   [--portmap="(port, port)[,(port,
>+ port)]"]
>
> where,
>
>@@ -99,7 +102,9 @@ where,
>
> *   q NQ: A number of queues (=ports) per lcore (default is 1)
>
>-*   --[no-]mac-updating: Enable or disable MAC addresses updating (enabled
>by default).
>+*   --[no-]mac-updating: Enable or disable MAC addresses updating (enabled
>by default)
>+
>+*   --portmap="(port,port)[,(port,port)]": Determines forwarding ports
>mapping.
>
> To run the application in linux environment with 4 lcores, 16 ports and 8 RX
>queues per lcore and MAC address  updating enabled, issue the command:
>@@ -108,6 +113,14 @@ updating enabled, issue the command:
>
>     $ ./build/l2fwd -l 0-3 -n 4 -- -q 8 -p ffff
>
>+To run the application in linux environment with 4 lcores, 4 ports, 8
>+RX queues per lcore, to forward RX traffic of ports 0 & 1 on ports 2 &
>+3 respectively and vice versa, issue the command:
>+
>+.. code-block:: console
>+
>+    $ ./build/l2fwd -l 0-3 -n 4 -- -q 8 -p f --portmap="(0,2)(1,3)"
>+
> Refer to the *DPDK Getting Started Guide* for general information on running
>applications  and the Environment Abstraction Layer (EAL) options.
>
>diff --git a/examples/l2fwd/main.c b/examples/l2fwd/main.c index
>88ddfe589..1d84cd789 100644
>--- a/examples/l2fwd/main.c
>+++ b/examples/l2fwd/main.c
>@@ -38,6 +38,7 @@
> #include <rte_ethdev.h>
> #include <rte_mempool.h>
> #include <rte_mbuf.h>
>+#include <rte_string_fns.h>
>
> static volatile bool force_quit;
>
>@@ -67,6 +68,15 @@ static uint32_t l2fwd_enabled_port_mask = 0;
> /* list of enabled ports */
> static uint32_t l2fwd_dst_ports[RTE_MAX_ETHPORTS];
>
>+struct port_pair_params {
>+#define NUM_PORTS	2
>+	uint16_t port[NUM_PORTS];
>+} __rte_cache_aligned;
>+
>+static struct port_pair_params
>port_pair_params_array[RTE_MAX_ETHPORTS
>+/ 2]; static struct port_pair_params *port_pair_params; static uint16_t
>+nb_port_pair_params;
>+
> static unsigned int l2fwd_rx_queue_per_lcore = 1;
>
> #define MAX_RX_QUEUE_PER_LCORE 16
>@@ -294,11 +304,13 @@ l2fwd_usage(const char *prgname)
> 	printf("%s [EAL options] -- -p PORTMASK [-q NQ]\n"
> 	       "  -p PORTMASK: hexadecimal bitmask of ports to configure\n"
> 	       "  -q NQ: number of queue (=ports) per lcore (default is 1)\n"
>-		   "  -T PERIOD: statistics will be refreshed each PERIOD
>seconds (0 to disable, 10 default, 86400 maximum)\n"
>-		   "  --[no-]mac-updating: Enable or disable MAC addresses
>updating (enabled by default)\n"
>-		   "      When enabled:\n"
>-		   "       - The source MAC address is replaced by the TX port
>MAC address\n"
>-		   "       - The destination MAC address is replaced by
>02:00:00:00:00:TX_PORT_ID\n",
>+	       "  -T PERIOD: statistics will be refreshed each PERIOD seconds (0 to
>disable, 10 default, 86400 maximum)\n"
>+	       "  --[no-]mac-updating: Enable or disable MAC addresses updating
>(enabled by default)\n"
>+	       "      When enabled:\n"
>+	       "       - The source MAC address is replaced by the TX port MAC
>address\n"
>+	       "       - The destination MAC address is replaced by
>02:00:00:00:00:TX_PORT_ID\n"
>+	       "  --portmap: Configure forwarding port pair mapping\n"
>+	       "	      Default: alternate port pairs\n\n",
> 	       prgname);
> }
>
>@@ -319,6 +331,61 @@ l2fwd_parse_portmask(const char *portmask)
> 	return pm;
> }
>
>+static int
>+l2fwd_parse_port_pair_config(const char *q_arg) {
>+	enum fieldnames {
>+		FLD_PORT1 = 0,
>+		FLD_PORT2,
>+		_NUM_FLD
>+	};
>+	unsigned long int_fld[_NUM_FLD];
>+	const char *p, *p0 = q_arg;
>+	char *str_fld[_NUM_FLD];
>+	unsigned int size;
>+	char s[256];
>+	char *end;
>+	int i;
>+
>+	nb_port_pair_params = 0;
>+
>+	while ((p = strchr(p0, '(')) != NULL) {
>+		++p;
>+		p0 = strchr(p, ')');
>+		if (p0 == NULL)
>+			return -1;
>+
>+		size = p0 - p;
>+		if (size >= sizeof(s))
>+			return -1;
>+
>+		memcpy(s, p, size);
>+		s[size] = '\0';
>+		if (rte_strsplit(s, sizeof(s), str_fld,
>+				 _NUM_FLD, ',') != _NUM_FLD)
>+			return -1;
>+		for (i = 0; i < _NUM_FLD; i++) {
>+			errno = 0;
>+			int_fld[i] = strtoul(str_fld[i], &end, 0);
>+			if (errno != 0 || end == str_fld[i] ||
>+			    int_fld[i] >= RTE_MAX_ETHPORTS)
>+				return -1;
>+		}
>+		if (nb_port_pair_params >= RTE_MAX_ETHPORTS/2) {
>+			printf("exceeded max number of port pair params:
>%hu\n",
>+				nb_port_pair_params);
>+			return -1;
>+		}
>+		port_pair_params_array[nb_port_pair_params].port[0] =
>+				(uint16_t)int_fld[FLD_PORT1];
>+		port_pair_params_array[nb_port_pair_params].port[1] =
>+				(uint16_t)int_fld[FLD_PORT2];
>+		++nb_port_pair_params;
>+	}
>+	port_pair_params = port_pair_params_array;
>+	return 0;
>+}
>+
> static unsigned int
> l2fwd_parse_nqueue(const char *q_arg)
> {
>@@ -361,6 +428,7 @@ static const char short_options[] =
>
> #define CMD_LINE_OPT_MAC_UPDATING "mac-updating"
> #define CMD_LINE_OPT_NO_MAC_UPDATING "no-mac-updating"
>+#define CMD_LINE_OPT_PORTMAP_CONFIG "portmap"
>
> enum {
> 	/* long options mapped to a short option */ @@ -368,11 +436,13 @@
>enum {
> 	/* first long only option value must be >= 256, so that we won't
> 	 * conflict with short options */
> 	CMD_LINE_OPT_MIN_NUM = 256,
>+	CMD_LINE_OPT_PORTMAP_NUM,
> };
>
> static const struct option lgopts[] = {
> 	{ CMD_LINE_OPT_MAC_UPDATING, no_argument, &mac_updating,
>1},
> 	{ CMD_LINE_OPT_NO_MAC_UPDATING, no_argument,
>&mac_updating, 0},
>+	{ CMD_LINE_OPT_PORTMAP_CONFIG, 1, 0,
>CMD_LINE_OPT_PORTMAP_NUM},
> 	{NULL, 0, 0, 0}
> };
>
>@@ -386,6 +456,7 @@ l2fwd_parse_args(int argc, char **argv)
> 	char *prgname = argv[0];
>
> 	argvopt = argv;
>+	port_pair_params = NULL;
>
> 	while ((opt = getopt_long(argc, argvopt, short_options,
> 				  lgopts, &option_index)) != EOF) { @@ -423,7
>+494,13 @@ l2fwd_parse_args(int argc, char **argv)
> 			break;
>
> 		/* long options */
>-		case 0:
>+		case CMD_LINE_OPT_PORTMAP_NUM:
>+			ret = l2fwd_parse_port_pair_config(optarg);
>+			if (ret) {
>+				fprintf(stderr, "Invalid config\n");
>+				l2fwd_usage(prgname);
>+				return -1;
>+			}
> 			break;
>
> 		default:
>@@ -440,6 +517,48 @@ l2fwd_parse_args(int argc, char **argv)
> 	return ret;
> }
>
>+/*
>+ * Check port pair config with enabled port mask,
>+ * and for valid port pair combinations.
>+ */
>+static int
>+check_port_pair_config(void)
>+{
>+	uint32_t port_pair_config_mask = 0;
>+	uint32_t port_pair_mask = 0;
>+	uint16_t index, i, portid;
>+
>+	for (index = 0; index < nb_port_pair_params; index++) {
>+		port_pair_mask = 0;
>+
>+		for (i = 0; i < NUM_PORTS; i++)  {
>+			portid = port_pair_params[index].port[i];
>+			if ((l2fwd_enabled_port_mask & (1 << portid)) == 0) {
>+				printf("port %u is not enabled in port
>mask\n",
>+				       portid);
>+				return -1;
>+			}
>+			if (!rte_eth_dev_is_valid_port(portid)) {
>+				printf("port %u is not present on the
>board\n",
>+				       portid);
>+				return -1;
>+			}
>+
>+			port_pair_mask |= 1 << portid;
>+		}
>+
>+		if (port_pair_config_mask & port_pair_mask) {
>+			printf("port %u is used in other port pairs\n", portid);
>+			return -1;
>+		}
>+		port_pair_config_mask |= port_pair_mask;
>+	}
>+
>+	l2fwd_enabled_port_mask &= port_pair_config_mask;
>+
>+	return 0;
>+}
>+
> /* Check the link status of all ports in up to 9s, and print them finally */  static
>void  check_all_ports_link_status(uint32_t port_mask) @@ -555,6 +674,11
>@@ main(int argc, char **argv)
> 	if (nb_ports == 0)
> 		rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
>
>+	if (port_pair_params != NULL) {
>+		if (check_port_pair_config() < 0)
>+			rte_exit(EXIT_FAILURE, "Invalid port pair config\n");
>+	}
>+
> 	/* check port mask to possible port mask */
> 	if (l2fwd_enabled_port_mask & ~((1 << nb_ports) - 1))
> 		rte_exit(EXIT_FAILURE, "Invalid portmask; possible (0x%x)\n",
>@@ -565,26 +689,35 @@ main(int argc, char **argv)
> 		l2fwd_dst_ports[portid] = 0;
> 	last_port = 0;
>
>-	/*
>-	 * Each logical core is assigned a dedicated TX queue on each port.
>-	 */
>-	RTE_ETH_FOREACH_DEV(portid) {
>-		/* skip ports that are not enabled */
>-		if ((l2fwd_enabled_port_mask & (1 << portid)) == 0)
>-			continue;
>+	/* populate destination port details */
>+	if (port_pair_params != NULL) {
>+		uint16_t idx, p;
>
>-		if (nb_ports_in_mask % 2) {
>-			l2fwd_dst_ports[portid] = last_port;
>-			l2fwd_dst_ports[last_port] = portid;
>+		for (idx = 0; idx < (nb_port_pair_params << 1); idx++) {
>+			p = idx & 1;
>+			portid = port_pair_params[idx >> 1].port[p];
>+			l2fwd_dst_ports[portid] =
>+				port_pair_params[idx >> 1].port[p ^ 1];
> 		}
>-		else
>-			last_port = portid;
>+	} else {
>+		RTE_ETH_FOREACH_DEV(portid) {
>+			/* skip ports that are not enabled */
>+			if ((l2fwd_enabled_port_mask & (1 << portid)) == 0)
>+				continue;
>
>-		nb_ports_in_mask++;
>-	}
>-	if (nb_ports_in_mask % 2) {
>-		printf("Notice: odd number of ports in portmask.\n");
>-		l2fwd_dst_ports[last_port] = last_port;
>+			if (nb_ports_in_mask % 2) {
>+				l2fwd_dst_ports[portid] = last_port;
>+				l2fwd_dst_ports[last_port] = portid;
>+			} else {
>+				last_port = portid;
>+			}
>+
>+			nb_ports_in_mask++;
>+		}
>+		if (nb_ports_in_mask % 2) {
>+			printf("Notice: odd number of ports in portmask.\n");
>+			l2fwd_dst_ports[last_port] = last_port;
>+		}
> 	}
>
> 	rx_lcore_id = 0;
>@@ -613,7 +746,8 @@ main(int argc, char **argv)
>
> 		qconf->rx_port_list[qconf->n_rx_port] = portid;
> 		qconf->n_rx_port++;
>-		printf("Lcore %u: RX port %u\n", rx_lcore_id, portid);
>+		printf("Lcore %u: RX port %u TX port %u\n", rx_lcore_id,
>+		       portid, l2fwd_dst_ports[portid]);
> 	}
>
> 	nb_mbufs = RTE_MAX(nb_ports * (nb_rxd + nb_txd +
>MAX_PKT_BURST +
>--
>2.17.1
  

Hi Vamsi & Pavan,

I like this idea, couple of queries

snipped
> +static int
> +check_port_pair_config(void)
> +{
> +	uint32_t port_pair_config_mask = 0;
> +	uint32_t port_pair_mask = 0;
> +	uint16_t index, i, portid;
> +
> +	for (index = 0; index < nb_port_pair_params; index++) {
> +		port_pair_mask = 0;
> +
> +		for (i = 0; i < NUM_PORTS; i++)  {
> +			portid = port_pair_params[index].port[i];
> +			if ((l2fwd_enabled_port_mask & (1 << portid)) == 0) {
> +				printf("port %u is not enabled in port
> mask\n",
> +				       portid);
> +				return -1;
> +			}
> +			if (!rte_eth_dev_is_valid_port(portid)) {
> +				printf("port %u is not present on the
> board\n",
> +				       portid);
> +				return -1;
> +			}
> +

Should we check & warn the user if 
1. port speed mismatch
2. on different NUMA
3. port pairs are physical and vdev like tap, and KNI (performance).

> +			port_pair_mask |= 1 << portid;
> +		}
> +

snipped


> 
> +	if (port_pair_params != NULL) {
> +		if (check_port_pair_config() < 0)
> +			rte_exit(EXIT_FAILURE, "Invalid port pair config\n");
> +	}
> +
>  	/* check port mask to possible port mask */
>  	if (l2fwd_enabled_port_mask & ~((1 << nb_ports) - 1))
>  		rte_exit(EXIT_FAILURE, "Invalid portmask; possible (0x%x)\n",
> @@ -565,26 +689,35 @@ main(int argc, char **argv)
>  		l2fwd_dst_ports[portid] = 0;
>  	last_port = 0;
> 

Should not the check_port_pair be after this? If the port is not enabled in port_mask will you skip that pair? or skip RX-TX from that port?

> -	/*
> -	 * Each logical core is assigned a dedicated TX queue on each port.
> -	 */
> -	RTE_ETH_FOREACH_DEV(portid) {
> -		/* skip ports that are not enabled */
> -		if ((l2fwd_enabled_port_mask & (1 << portid)) == 0)
> -			continue;
> +	/* populate destination port details */
> +	if (port_pair_params != NULL) {
> +		uint16_t idx, p;
> 
> -		if (nb_ports_in_mask % 2) {
> -			l2fwd_dst_ports[portid] = last_port;
> -			l2fwd_dst_ports[last_port] = portid;
> +		for (idx = 0; idx < (nb_port_pair_params << 1); idx++) {
> +			p = idx & 1;
> +			portid = port_pair_params[idx >> 1].port[p];
> +			l2fwd_dst_ports[portid] =
> +				port_pair_params[idx >> 1].port[p ^ 1];
>  		}
> -		else
> -			last_port = portid;
> +	} else {
> +		RTE_ETH_FOREACH_DEV(portid) {
> +			/* skip ports that are not enabled */
> +			if ((l2fwd_enabled_port_mask & (1 << portid)) == 0)
> +				continue;
> 
> -		nb_ports_in_mask++;
> -	}
> -	if (nb_ports_in_mask % 2) {
> -		printf("Notice: odd number of ports in portmask.\n");
> -		l2fwd_dst_ports[last_port] = last_port;
> +			if (nb_ports_in_mask % 2) {
> +				l2fwd_dst_ports[portid] = last_port;
> +				l2fwd_dst_ports[last_port] = portid;
> +			} else {
> +				last_port = portid;
> +			}
> +
> +			nb_ports_in_mask++;
> +		}
> +		if (nb_ports_in_mask % 2) {
> +			printf("Notice: odd number of ports in portmask.\n");
> +			l2fwd_dst_ports[last_port] = last_port;
> +		}
>  	}

As mentioned above there can ports in mask which might be disabled for port pair. Should not that be skipped rather than setting last port rx-tx loopback?

> 
>  	rx_lcore_id = 0;
> @@ -613,7 +746,8 @@ main(int argc, char **argv)
> 
>  		qconf->rx_port_list[qconf->n_rx_port] = portid;
>  		qconf->n_rx_port++;
> -		printf("Lcore %u: RX port %u\n", rx_lcore_id, portid);
> +		printf("Lcore %u: RX port %u TX port %u\n", rx_lcore_id,
> +		       portid, l2fwd_dst_ports[portid]);
>  	}
> 
>  	nb_mbufs = RTE_MAX(nb_ports * (nb_rxd + nb_txd +
> MAX_PKT_BURST +
> --
> 2.17.1
  

>Hi Vamsi & Pavan,
>
>I like this idea, couple of queries
>
>snipped
>> +static int
>> +check_port_pair_config(void)
>> +{
>> +	uint32_t port_pair_config_mask = 0;
>> +	uint32_t port_pair_mask = 0;
>> +	uint16_t index, i, portid;
>> +
>> +	for (index = 0; index < nb_port_pair_params; index++) {
>> +		port_pair_mask = 0;
>> +
>> +		for (i = 0; i < NUM_PORTS; i++)  {
>> +			portid = port_pair_params[index].port[i];
>> +			if ((l2fwd_enabled_port_mask & (1 << portid))
>== 0) {
>> +				printf("port %u is not enabled in port
>> mask\n",
>> +				       portid);
>> +				return -1;
>> +			}
>> +			if (!rte_eth_dev_is_valid_port(portid)) {
>> +				printf("port %u is not present on the
>> board\n",
>> +				       portid);
>> +				return -1;
>> +			}
>> +
>
>Should we check & warn the user if
>1. port speed mismatch
>2. on different NUMA
>3. port pairs are physical and vdev like tap, and KNI (performance).
>

Sure, it can be a separate patch as it will be applicable for multiple examples.

>> +			port_pair_mask |= 1 << portid;
>> +		}
>> +
>
>snipped
>
>
>>
>> +	if (port_pair_params != NULL) {
>> +		if (check_port_pair_config() < 0)
>> +			rte_exit(EXIT_FAILURE, "Invalid port pair
>config\n");
>> +	}
>> +
>>  	/* check port mask to possible port mask */
>>  	if (l2fwd_enabled_port_mask & ~((1 << nb_ports) - 1))
>>  		rte_exit(EXIT_FAILURE, "Invalid portmask; possible
>(0x%x)\n",
>> @@ -565,26 +689,35 @@ main(int argc, char **argv)
>>  		l2fwd_dst_ports[portid] = 0;
>>  	last_port = 0;
>>
>
>Should not the check_port_pair be after this? If the port is not enabled
>in port_mask will you skip that pair? or skip RX-TX from that port?

We check every port pair against l2fwd_enabled_port_mask in 
check_port_pair_config()

>
>> -	/*
>> -	 * Each logical core is assigned a dedicated TX queue on each
>port.
>> -	 */
>> -	RTE_ETH_FOREACH_DEV(portid) {
>> -		/* skip ports that are not enabled */
>> -		if ((l2fwd_enabled_port_mask & (1 << portid)) == 0)
>> -			continue;
>> +	/* populate destination port details */
>> +	if (port_pair_params != NULL) {
>> +		uint16_t idx, p;
>>
>> -		if (nb_ports_in_mask % 2) {
>> -			l2fwd_dst_ports[portid] = last_port;
>> -			l2fwd_dst_ports[last_port] = portid;
>> +		for (idx = 0; idx < (nb_port_pair_params << 1); idx++) {
>> +			p = idx & 1;
>> +			portid = port_pair_params[idx >> 1].port[p];
>> +			l2fwd_dst_ports[portid] =
>> +				port_pair_params[idx >> 1].port[p ^ 1];
>>  		}
>> -		else
>> -			last_port = portid;
>> +	} else {
>> +		RTE_ETH_FOREACH_DEV(portid) {
>> +			/* skip ports that are not enabled */
>> +			if ((l2fwd_enabled_port_mask & (1 << portid))
>== 0)
>> +				continue;
>>
>> -		nb_ports_in_mask++;
>> -	}
>> -	if (nb_ports_in_mask % 2) {
>> -		printf("Notice: odd number of ports in portmask.\n");
>> -		l2fwd_dst_ports[last_port] = last_port;
>> +			if (nb_ports_in_mask % 2) {
>> +				l2fwd_dst_ports[portid] = last_port;
>> +				l2fwd_dst_ports[last_port] = portid;
>> +			} else {
>> +				last_port = portid;
>> +			}
>> +
>> +			nb_ports_in_mask++;
>> +		}
>> +		if (nb_ports_in_mask % 2) {
>> +			printf("Notice: odd number of ports in
>portmask.\n");
>> +			l2fwd_dst_ports[last_port] = last_port;
>> +		}
>>  	}
>
>As mentioned above there can ports in mask which might be disabled
>for port pair. Should not that be skipped rather than setting last port rx-
>tx loopback?

There could be scenarios where user might want to test 2x10G and 1x40G 
Why force the user to explicitly mention 1x40G as port pair of itself in the 
portpair config?

>
>>
>>  	rx_lcore_id = 0;
>> @@ -613,7 +746,8 @@ main(int argc, char **argv)
>>
>>  		qconf->rx_port_list[qconf->n_rx_port] = portid;
>>  		qconf->n_rx_port++;
>> -		printf("Lcore %u: RX port %u\n", rx_lcore_id, portid);
>> +		printf("Lcore %u: RX port %u TX port %u\n",
>rx_lcore_id,
>> +		       portid, l2fwd_dst_ports[portid]);
>>  	}
>>
>>  	nb_mbufs = RTE_MAX(nb_ports * (nb_rxd + nb_txd +
>> MAX_PKT_BURST +
>> --
>> 2.17.1
  

Hi Pavan,

snipped
> >
> >Should we check & warn the user if
> >1. port speed mismatch
> >2. on different NUMA
> >3. port pairs are physical and vdev like tap, and KNI (performance).
> >
> 
> Sure, it can be a separate patch as it will be applicable for multiple examples.
I believe this patch is for example `l2fwd`. But you would like to have to updated for all `example`. I am ok for this.

snipped
> >
> >Should not the check_port_pair be after this? If the port is not
> >enabled in port_mask will you skip that pair? or skip RX-TX from that port?
> 
> We check every port pair against l2fwd_enabled_port_mask in
> check_port_pair_config()


> 
snipped
> >
> >As mentioned above there can ports in mask which might be disabled for
> >port pair. Should not that be skipped rather than setting last port rx-
> >tx loopback?
> 
> There could be scenarios where user might want to test 2x10G and 1x40G Why
> force the user to explicitly mention 1x40G as port pair of itself in the portpair
> config?
I am not sure if I follow your thought, as your current port map only allows `1:1` mapping by `struct port_pair_params`. This can be to self like `(port0:port0),(port1:port1)` or `(port-0:port-1)`.

1. But current `l2fwd_parse_port_pair_config` does not consider the same port mapping as we have hard check for `if (nb_port_pair_params >= RTE_MAX_ETHPORTS/2)`. 

2. `l2fwd_enabled_port_mask` is global variable of user port mask. This can contain both valid and invalid mask. Hence we check `l2fwd_enabled_port_mask & ~((1 << nb_ports) - 1)`.

3. can these scenarios are true if we invoke `check_port_pair_config` before actual port_mask check.
 a. there are only 4 ports, hence possible mask is `0xf`.
 b. user passes port argument as `0xe`
 c. `check_port_pair_config` gets masks for `(1,3)` as input and populates `port_pair_config_mask`.
 d.  As per the code, port 2 which is valid port and part of user port mask will have lastport (which is port 3)? May be I did understand the logic correct. Can you help me?

So my concerns are 1) there is no same port mapping, 2) my understanding on lastport logic is not clear and 3) as per the code there is 1:N but 1:1.

Hence there should be sufficient warning to user if port are of wrong speed and NUMA.

Note: current speed can be fetched only if the port are started too (in Fortville). 

snipped
  

Hi Vipin,

>Hi Pavan,
>
>snipped
>> >
>> >Should we check & warn the user if
>> >1. port speed mismatch
>> >2. on different NUMA
>> >3. port pairs are physical and vdev like tap, and KNI (performance).
>> >
>>
>> Sure, it can be a separate patch as it will be applicable for multiple
>examples.
>I believe this patch is for example `l2fwd`. But you would like to have to
>updated for all `example`. I am ok for this.
>
>snipped
>> >
>> >Should not the check_port_pair be after this? If the port is not
>> >enabled in port_mask will you skip that pair? or skip RX-TX from that
>port?
>>
>> We check every port pair against l2fwd_enabled_port_mask in
>> check_port_pair_config()
>
>
>>
>snipped
>> >
>> >As mentioned above there can ports in mask which might be
>disabled for
>> >port pair. Should not that be skipped rather than setting last port rx-
>> >tx loopback?
>>
>> There could be scenarios where user might want to test 2x10G and
>1x40G Why
>> force the user to explicitly mention 1x40G as port pair of itself in the
>portpair
>> config?
>I am not sure if I follow your thought, as your current port map only
>allows `1:1` mapping by `struct port_pair_params`. This can be to self
>like `(port0:port0),(port1:port1)` or `(port-0:port-1)`.
>
>1. But current `l2fwd_parse_port_pair_config` does not consider the
>same port mapping as we have hard check for `if (nb_port_pair_params
>>= RTE_MAX_ETHPORTS/2)`.
>
>2. `l2fwd_enabled_port_mask` is global variable of user port mask. This
>can contain both valid and invalid mask. Hence we check
>`l2fwd_enabled_port_mask & ~((1 << nb_ports) - 1)`.
>
>3. can these scenarios are true if we invoke `check_port_pair_config`
>before actual port_mask check.
> a. there are only 4 ports, hence possible mask is `0xf`.
> b. user passes port argument as `0xe`
> c. `check_port_pair_config` gets masks for `(1,3)` as input and
>populates `port_pair_config_mask`.
> d.  As per the code, port 2 which is valid port and part of user port mask
>will have lastport (which is port 3)? May be I did understand the logic
>correct. Can you help me?

Here user needs to explicitly mention (2,2) for port 2 to be setup else it 
will be skipped. 
If you see `check_port_pair_config` below we disable the ports that are not 
Mentioned in portmap.

"
check_port_pair_config(void)
{

<snip>
		port_pair_config_mask |= port_pair_mask;
	}

	l2fwd_enabled_port_mask &= port_pair_config_mask;

	return 0;
}
"


>
>So my concerns are 1) there is no same port mapping, 2) my
>understanding on lastport logic is not clear and 3) as per the code there
>is 1:N but 1:1.
>
>Hence there should be sufficient warning to user if port are of wrong
>speed and NUMA.

Unless the user disables stats using -T 0 option all the prints will be skipped.

>
>Note: current speed can be fetched only if the port are started too (in
>Fortville).
>
>snipped
  

Bruce, as maintainer of l2fwd example, any opinion about this change?


11/05/2020 02:23, Pavan Nikhilesh Bhagavatula:
> Hi Vipin,
> 
> >Hi Pavan,
> >
> >snipped
> >> >
> >> >Should we check & warn the user if
> >> >1. port speed mismatch
> >> >2. on different NUMA
> >> >3. port pairs are physical and vdev like tap, and KNI (performance).
> >> >
> >>
> >> Sure, it can be a separate patch as it will be applicable for multiple
> >examples.
> >I believe this patch is for example `l2fwd`. But you would like to have to
> >updated for all `example`. I am ok for this.
> >
> >snipped
> >> >
> >> >Should not the check_port_pair be after this? If the port is not
> >> >enabled in port_mask will you skip that pair? or skip RX-TX from that
> >port?
> >>
> >> We check every port pair against l2fwd_enabled_port_mask in
> >> check_port_pair_config()
> >
> >
> >>
> >snipped
> >> >
> >> >As mentioned above there can ports in mask which might be
> >disabled for
> >> >port pair. Should not that be skipped rather than setting last port rx-
> >> >tx loopback?
> >>
> >> There could be scenarios where user might want to test 2x10G and
> >1x40G Why
> >> force the user to explicitly mention 1x40G as port pair of itself in the
> >portpair
> >> config?
> >I am not sure if I follow your thought, as your current port map only
> >allows `1:1` mapping by `struct port_pair_params`. This can be to self
> >like `(port0:port0),(port1:port1)` or `(port-0:port-1)`.
> >
> >1. But current `l2fwd_parse_port_pair_config` does not consider the
> >same port mapping as we have hard check for `if (nb_port_pair_params
> >>= RTE_MAX_ETHPORTS/2)`.
> >
> >2. `l2fwd_enabled_port_mask` is global variable of user port mask. This
> >can contain both valid and invalid mask. Hence we check
> >`l2fwd_enabled_port_mask & ~((1 << nb_ports) - 1)`.
> >
> >3. can these scenarios are true if we invoke `check_port_pair_config`
> >before actual port_mask check.
> > a. there are only 4 ports, hence possible mask is `0xf`.
> > b. user passes port argument as `0xe`
> > c. `check_port_pair_config` gets masks for `(1,3)` as input and
> >populates `port_pair_config_mask`.
> > d.  As per the code, port 2 which is valid port and part of user port mask
> >will have lastport (which is port 3)? May be I did understand the logic
> >correct. Can you help me?
> 
> Here user needs to explicitly mention (2,2) for port 2 to be setup else it 
> will be skipped. 
> If you see `check_port_pair_config` below we disable the ports that are not 
> Mentioned in portmap.
> 
> "
> check_port_pair_config(void)
> {
> 
> <snip>
> 		port_pair_config_mask |= port_pair_mask;
> 	}
> 
> 	l2fwd_enabled_port_mask &= port_pair_config_mask;
> 
> 	return 0;
> }
> "
> 
> 
> >
> >So my concerns are 1) there is no same port mapping, 2) my
> >understanding on lastport logic is not clear and 3) as per the code there
> >is 1:N but 1:1.
> >
> >Hence there should be sufficient warning to user if port are of wrong
> >speed and NUMA.
> 
> Unless the user disables stats using -T 0 option all the prints will be skipped.
> 
> >
> >Note: current speed can be fetched only if the port are started too (in
> >Fortville).
> >
> >snipped
  

On Sun, May 24, 2020 at 06:13:22PM +0200, Thomas Monjalon wrote:
> Bruce, as maintainer of l2fwd example, any opinion about this change?
> 
Assuming all previous discussion on it is resolved, I'm fine with this
patch, though I suspect it will only make 20.08 now.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

> 
> 11/05/2020 02:23, Pavan Nikhilesh Bhagavatula:
> > Hi Vipin,
> > 
> > >Hi Pavan,
> > >
> > >snipped
> > >> >
> > >> >Should we check & warn the user if
> > >> >1. port speed mismatch
> > >> >2. on different NUMA
> > >> >3. port pairs are physical and vdev like tap, and KNI (performance).
> > >> >
> > >>
> > >> Sure, it can be a separate patch as it will be applicable for multiple
> > >examples.
> > >I believe this patch is for example `l2fwd`. But you would like to have to
> > >updated for all `example`. I am ok for this.
> > >
> > >snipped
> > >> >
> > >> >Should not the check_port_pair be after this? If the port is not
> > >> >enabled in port_mask will you skip that pair? or skip RX-TX from that
> > >port?
> > >>
> > >> We check every port pair against l2fwd_enabled_port_mask in
> > >> check_port_pair_config()
> > >
> > >
> > >>
> > >snipped
> > >> >
> > >> >As mentioned above there can ports in mask which might be
> > >disabled for
> > >> >port pair. Should not that be skipped rather than setting last port rx-
> > >> >tx loopback?
> > >>
> > >> There could be scenarios where user might want to test 2x10G and
> > >1x40G Why
> > >> force the user to explicitly mention 1x40G as port pair of itself in the
> > >portpair
> > >> config?
> > >I am not sure if I follow your thought, as your current port map only
> > >allows `1:1` mapping by `struct port_pair_params`. This can be to self
> > >like `(port0:port0),(port1:port1)` or `(port-0:port-1)`.
> > >
> > >1. But current `l2fwd_parse_port_pair_config` does not consider the
> > >same port mapping as we have hard check for `if (nb_port_pair_params
> > >>= RTE_MAX_ETHPORTS/2)`.
> > >
> > >2. `l2fwd_enabled_port_mask` is global variable of user port mask. This
> > >can contain both valid and invalid mask. Hence we check
> > >`l2fwd_enabled_port_mask & ~((1 << nb_ports) - 1)`.
> > >
> > >3. can these scenarios are true if we invoke `check_port_pair_config`
> > >before actual port_mask check.
> > > a. there are only 4 ports, hence possible mask is `0xf`.
> > > b. user passes port argument as `0xe`
> > > c. `check_port_pair_config` gets masks for `(1,3)` as input and
> > >populates `port_pair_config_mask`.
> > > d.  As per the code, port 2 which is valid port and part of user port mask
> > >will have lastport (which is port 3)? May be I did understand the logic
> > >correct. Can you help me?
> > 
> > Here user needs to explicitly mention (2,2) for port 2 to be setup else it 
> > will be skipped. 
> > If you see `check_port_pair_config` below we disable the ports that are not 
> > Mentioned in portmap.
> > 
> > "
> > check_port_pair_config(void)
> > {
> > 
> > <snip>
> > 		port_pair_config_mask |= port_pair_mask;
> > 	}
> > 
> > 	l2fwd_enabled_port_mask &= port_pair_config_mask;
> > 
> > 	return 0;
> > }
> > "
> > 
> > 
> > >
> > >So my concerns are 1) there is no same port mapping, 2) my
> > >understanding on lastport logic is not clear and 3) as per the code there
> > >is 1:N but 1:1.
> > >
> > >Hence there should be sufficient warning to user if port are of wrong
> > >speed and NUMA.
> > 
> > Unless the user disables stats using -T 0 option all the prints will be skipped.
> > 
> > >
> > >Note: current speed can be fetched only if the port are started too (in
> > >Fortville).
> > >
> > >snipped
> 
> 
> 
>
  

On Mon, May 25, 2020 at 2:59 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Sun, May 24, 2020 at 06:13:22PM +0200, Thomas Monjalon wrote:
> > Bruce, as maintainer of l2fwd example, any opinion about this change?
> >
> Assuming all previous discussion on it is resolved, I'm fine with this
> patch, though I suspect it will only make 20.08 now.
>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Ping for merge.


>
> >
> > 11/05/2020 02:23, Pavan Nikhilesh Bhagavatula:
> > > Hi Vipin,
> > >
> > > >Hi Pavan,
> > > >
> > > >snipped
> > > >> >
> > > >> >Should we check & warn the user if
> > > >> >1. port speed mismatch
> > > >> >2. on different NUMA
> > > >> >3. port pairs are physical and vdev like tap, and KNI (performance).
> > > >> >
> > > >>
> > > >> Sure, it can be a separate patch as it will be applicable for multiple
> > > >examples.
> > > >I believe this patch is for example `l2fwd`. But you would like to have to
> > > >updated for all `example`. I am ok for this.
> > > >
> > > >snipped
> > > >> >
> > > >> >Should not the check_port_pair be after this? If the port is not
> > > >> >enabled in port_mask will you skip that pair? or skip RX-TX from that
> > > >port?
> > > >>
> > > >> We check every port pair against l2fwd_enabled_port_mask in
> > > >> check_port_pair_config()
> > > >
> > > >
> > > >>
> > > >snipped
> > > >> >
> > > >> >As mentioned above there can ports in mask which might be
> > > >disabled for
> > > >> >port pair. Should not that be skipped rather than setting last port rx-
> > > >> >tx loopback?
> > > >>
> > > >> There could be scenarios where user might want to test 2x10G and
> > > >1x40G Why
> > > >> force the user to explicitly mention 1x40G as port pair of itself in the
> > > >portpair
> > > >> config?
> > > >I am not sure if I follow your thought, as your current port map only
> > > >allows `1:1` mapping by `struct port_pair_params`. This can be to self
> > > >like `(port0:port0),(port1:port1)` or `(port-0:port-1)`.
> > > >
> > > >1. But current `l2fwd_parse_port_pair_config` does not consider the
> > > >same port mapping as we have hard check for `if (nb_port_pair_params
> > > >>= RTE_MAX_ETHPORTS/2)`.
> > > >
> > > >2. `l2fwd_enabled_port_mask` is global variable of user port mask. This
> > > >can contain both valid and invalid mask. Hence we check
> > > >`l2fwd_enabled_port_mask & ~((1 << nb_ports) - 1)`.
> > > >
> > > >3. can these scenarios are true if we invoke `check_port_pair_config`
> > > >before actual port_mask check.
> > > > a. there are only 4 ports, hence possible mask is `0xf`.
> > > > b. user passes port argument as `0xe`
> > > > c. `check_port_pair_config` gets masks for `(1,3)` as input and
> > > >populates `port_pair_config_mask`.
> > > > d.  As per the code, port 2 which is valid port and part of user port mask
> > > >will have lastport (which is port 3)? May be I did understand the logic
> > > >correct. Can you help me?
> > >
> > > Here user needs to explicitly mention (2,2) for port 2 to be setup else it
> > > will be skipped.
> > > If you see `check_port_pair_config` below we disable the ports that are not
> > > Mentioned in portmap.
> > >
> > > "
> > > check_port_pair_config(void)
> > > {
> > >
> > > <snip>
> > >             port_pair_config_mask |= port_pair_mask;
> > >     }
> > >
> > >     l2fwd_enabled_port_mask &= port_pair_config_mask;
> > >
> > >     return 0;
> > > }
> > > "
> > >
> > >
> > > >
> > > >So my concerns are 1) there is no same port mapping, 2) my
> > > >understanding on lastport logic is not clear and 3) as per the code there
> > > >is 1:N but 1:1.
> > > >
> > > >Hence there should be sufficient warning to user if port are of wrong
> > > >speed and NUMA.
> > >
> > > Unless the user disables stats using -T 0 option all the prints will be skipped.
> > >
> > > >
> > > >Note: current speed can be fetched only if the port are started too (in
> > > >Fortville).
> > > >
> > > >snipped
> >
> >
> >
> >
  

04/07/2020 15:36, Jerin Jacob:
> On Mon, May 25, 2020 at 2:59 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Sun, May 24, 2020 at 06:13:22PM +0200, Thomas Monjalon wrote:
> > > Bruce, as maintainer of l2fwd example, any opinion about this change?
> > >
> > Assuming all previous discussion on it is resolved, I'm fine with this
> > patch, though I suspect it will only make 20.08 now.
> >
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> Ping for merge.

Applied, thanks