On 4/3/20 5:30 AM, vattun...@marvell.com wrote:
> From: Vamsi Attunuru <vattun...@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 <vattun...@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 <aostrus...@marvell.com>
With regards
Andrzej Ostruszka
