Re: [dpdk-dev] [PATCH] [v1 1/1] examples/l2fwd: add cmdline option for forwarding port info

["Andrzej Ostruszka [C]" <aostruszka@marvell.com>]

On 4/3/20 5:30 AM, vattunuru@marvell.com wrote:
> From: Vamsi Attunuru <vattunuru@marvell.com>
> 
> Current l2fwd application configures adjacent ports as destination
> ports for forwarding the traffic which is a kind of static mapping
> that can not be altered by the command line options.
> 
> Patch adds a config option to pass the forwarding port pair mapping
> as a command line parameter which allows the user to pass required
> forwarding port mapping.
> 
> If no config argument is specified, destination port map is not
> changed and traffic gets forwarded with existing mapping.
> 
> When port pair mapping is passed in config option, destination port map
> is configured and traffic gets forwarded accordingly.
> 
> Ex: ./l2fwd -c 0xff -- -p 0x3f --config="(0,3)(1,4)(2,5)"
> 
> With above config 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>
> ---
>  doc/guides/rel_notes/release_20_05.rst             |   6 +
>  .../sample_app_ug/l2_forward_real_virtual.rst      |  18 ++-
>  examples/l2fwd/main.c                              | 174 ++++++++++++++++++---
>  3 files changed, 177 insertions(+), 21 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_20_05.rst b/doc/guides/rel_notes/release_20_05.rst
> index 000bbf5..645400b 100644
> --- a/doc/guides/rel_notes/release_20_05.rst
> +++ b/doc/guides/rel_notes/release_20_05.rst
> @@ -62,6 +62,12 @@ New Features
>  
>    * Added support for matching on IPv4 Time To Live and IPv6 Hop Limit.
>  
> +* **Added --config command line parameter to l2fwd example.**
> +
> +  Added new command line option ``--config(port, port)[,(port, port)]`` to

Maybe use the actual syntax with '=' or space after 'config'?

> +  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 39d6b00..e708f88 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
> +                                   --config(port, port)[,(port, port)]

Ditto

>  
>  where,
>  
> @@ -99,7 +102,10 @@ 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)
> +
> +*   --config(port,port)[,(port,port)]: Determines which ports are mapped to
> +    which ports for packet forwarding.

Ditto

>  
>  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 +114,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 and MAC address updating enabled, 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 --config="(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 09257aa..a8b98b9 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];
> +static struct port_pair_params *port_pair_params;
> +static uint16_t nb_port_pair_params = 1;

Why this initialization to 1?  Below (during parsing) it is initialized
to 0, so I would leave this default initialized.

> +
>  static unsigned int l2fwd_rx_queue_per_lcore = 1;
>  
>  #define MAX_RX_QUEUE_PER_LCORE 16
> @@ -319,6 +329,60 @@ 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;
> +
> +		snprintf(s, sizeof(s), "%.*s", size, p);

Same comment as for recent l2fwd-event - this can be simple memcpy.

> +		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)

I think this check should be >=, RTE_MAX_ETHPORTS is an invalid port id.

> +				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;
> +}

[...]

> @@ -565,26 +686,40 @@ 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;
> +
> +		for (idx = 0; idx < (nb_port_pair_params << 1); idx++) {
> +			if (idx % 2 == 0) {
> +				portid = port_pair_params[idx >> 1].port[0];
> +				l2fwd_dst_ports[portid] =
> +					port_pair_params[idx >> 1].port[1];
> +			} else {
> +				portid = port_pair_params[idx >> 1].port[1];
> +				l2fwd_dst_ports[portid] =
> +					port_pair_params[idx >> 1].port[0];
> +			}

I think this conditional can be rephrased like:

	uint16_t idx, p;
...
	p = idx & 1;
	port_id = port_pair_params[idx >> 1].port[p];
	l2fwd_dst_ports[port_id] =
		port_pair_params[idx >> 1].port[p ^ 1];

but I'm not sure which one is more readable - I leave you the decision,
 feel free to skip this comment altogether :).

> +		}
> +	} else {
> +		RTE_ETH_FOREACH_DEV(portid) {
> +			/* skip ports that are not enabled */
> +			if ((l2fwd_enabled_port_mask & (1 << portid)) == 0)
> +				continue;
> +
> +			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) {
> -			l2fwd_dst_ports[portid] = last_port;
> -			l2fwd_dst_ports[last_port] = portid;
> +			printf("Notice: odd number of ports in portmask.\n");
> +			l2fwd_dst_ports[last_port] = last_port;
>  		}
> -		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 +748,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 +
> 

Apart from these comments:

Reviewed-by: Andrzej Ostruszka <aostruszka@marvell.com>

With regards
Andrzej Ostruszka