[v3] examples/l2fwd: add cmdline option for forwarding port info
Checks
Commit Message
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>
---
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 | 18 +-
examples/l2fwd/main.c | 182 +++++++++++++++---
3 files changed, 181 insertions(+), 26 deletions(-)
--
2.17.1
Comments
>-----Original Message-----
>From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
>Sent: Monday, April 27, 2020 1:30 PM
>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 v3] 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>
>---
> 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 | 18 +-
> examples/l2fwd/main.c | 182 +++++++++++++++---
> 3 files changed, 181 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..b54321b5b 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)]"
Will it be better to represent as [--portmap="(port, port)[,(port, port)]] as it is a optional parameter ?
>
> 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)
>+
>+* --portmap="(port,port)[,(port,port)]": Determines which ports are
>mapped to
>+ which ports for packet forwarding.
May be rephrased a bit shorter as "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 +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:
IMO, No need to mentioned information about MAC address update enabled because it
is enabled by default and no param is passed in below command. So information is not
relevant here.
>+
>+.. code-block:: console
>+
>+ $ ./build/l2fwd -l 0-3 -n 4 -- -q 8 -p f --portmap="(0,2)(1,3)"
It looks like comma is missed in between two port map information.
>+
> 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..81861a22a 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;
Is there any specific reason to use this syntax to declare two ports instead of following
struct port_pair_params {
uint16_t port1;
uint16_t port2;
};
>+
>+static struct port_pair_params
>+port_pair_params_array[RTE_MAX_ETHPORTS];
Should not be RTE_MAX_ETHPORTS/2 if only 1:1 mapping is allowed ?
As I understood it is used to store CLI port mapping, I think it is better to
Use any other MACRO which define maximum number of port map parameter.
If you consider it, then it can also be updated in documentation.
>+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",
IMO, Indentation changes for other parameters should in separate patch.
> 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
On 27/04/2020 11:19, Sunil Kumar Kori wrote:
>> -----Original Message-----
>> From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
[...]
>> @@ -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;
>
> Is there any specific reason to use this syntax to declare two ports instead of following
> struct port_pair_params {
> uint16_t port1;
> uint16_t port2;
> };
Initially it was so, but I made a comment that this leads to code
duplication in check_port_pair_config() (same checks for port1 and port2
- now handled via loop). I still stand by this comment :)
With regards
Andrzej Ostruszka
>-----Original Message-----
>From: Andrzej Ostruszka [C] <aostruszka@marvell.com>
>Sent: Monday, April 27, 2020 3:06 PM
>To: Sunil Kumar Kori <skori@marvell.com>; Pavan Nikhilesh Bhagavatula
><pbhagavatula@marvell.com>; 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>
>Cc: dev@dpdk.org; Vamsi Krishna Attunuru <vattunuru@marvell.com>
>Subject: Re: [dpdk-dev] [PATCH v3] examples/l2fwd: add cmdline option for
>forwarding port info
>
>On 27/04/2020 11:19, Sunil Kumar Kori wrote:
>>> -----Original Message-----
>>> From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
>[...]
>>> @@ -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;
>>
>> Is there any specific reason to use this syntax to declare two ports
>> instead of following struct port_pair_params {
>> uint16_t port1;
>> uint16_t port2;
>> };
>
>Initially it was so, but I made a comment that this leads to code duplication in
>check_port_pair_config() (same checks for port1 and port2
>- now handled via loop). I still stand by this comment :)
>
Okay, I think having for two variables only is not a big deal. if it is some higher number then it look fine.
IMO, it is more readable when using suggested way. You can take on this.
>With regards
>Andrzej Ostruszka
>>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..b54321b5b 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)]"
>
>Will it be better to represent as [--portmap="(port, port)[,(port, port)]]
>as it is a optional parameter ?
>
Will change in v4.
>>
>> 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)
>>+
>>+* --portmap="(port,port)[,(port,port)]": Determines which ports are
>>mapped to
>>+ which ports for packet forwarding.
>
>May be rephrased a bit shorter as "Determines forwarding ports
>mapping".
Will change in v4.
>>
>> 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:
>
>IMO, No need to mentioned information about MAC address update
>enabled because it
>is enabled by default and no param is passed in below command. So
>information is not
>relevant here.
>
Will change in v4.
>>+
>>+.. code-block:: console
>>+
>>+ $ ./build/l2fwd -l 0-3 -n 4 -- -q 8 -p f --portmap="(0,2)(1,3)"
>It looks like comma is missed in between two port map information.
>>+
>> 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..81861a22a 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;
>
>Is there any specific reason to use this syntax to declare two ports
>instead of following
>struct port_pair_params {
> uint16_t port1;
> uint16_t port2;
>};
>
It was done to reduce code duplication. Will leave as is.
>>+
>>+static struct port_pair_params
>>+port_pair_params_array[RTE_MAX_ETHPORTS];
>Should not be RTE_MAX_ETHPORTS/2 if only 1:1 mapping is allowed ?
>As I understood it is used to store CLI port mapping,
RTE_MAX_ETHPORTS/2 will be sufficient.
>I think it is better to
>Use any other MACRO which define maximum number of port map
>parameter.
>If you consider it, then it can also be updated in documentation.
>
>>+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",
>
>IMO, Indentation changes for other parameters should in separate
>patch.
It's not a functionality change it just minor code formatting change and doesn't
deserve a separate patch.
>> prgname);
>> }
>>
>>@@ -319,6 +331,61 @@ l2fwd_parse_portmask(const char
>*portmask)
>> return pm;
>> }
>>
Sent from Workspace ONE Boxer
On 27-Apr-2020 10:08 PM, Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> wrote:
>
> >>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..b54321b5b 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)]"
> >
> >Will it be better to represent as [--portmap="(port, port)[,(port, port)]]
> >as it is a optional parameter ?
> >
>
> Will change in v4.
>
> >>
> >> 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)
> >>+
> >>+* --portmap="(port,port)[,(port,port)]": Determines which ports are
> >>mapped to
> >>+ which ports for packet forwarding.
> >
> >May be rephrased a bit shorter as "Determines forwarding ports
> >mapping".
>
> Will change in v4.
>
> >>
> >> 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:
> >
> >IMO, No need to mentioned information about MAC address update
> >enabled because it
> >is enabled by default and no param is passed in below command. So
> >information is not
> >relevant here.
> >
>
> Will change in v4.
>
> >>+
> >>+.. code-block:: console
> >>+
> >>+ $ ./build/l2fwd -l 0-3 -n 4 -- -q 8 -p f --portmap="(0,2)(1,3)"
> >It looks like comma is missed in between two port map information.
> >>+
> >> 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..81861a22a 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;
> >
> >Is there any specific reason to use this syntax to declare two ports
> >instead of following
> >struct port_pair_params {
> > uint16_t port1;
> > uint16_t port2;
> >};
> >
>
> It was done to reduce code duplication. Will leave as is.
Ack.
>
> >>+
> >>+static struct port_pair_params
> >>+port_pair_params_array[RTE_MAX_ETHPORTS];
> >Should not be RTE_MAX_ETHPORTS/2 if only 1:1 mapping is allowed ?
> >As I understood it is used to store CLI port mapping,
>
> RTE_MAX_ETHPORTS/2 will be sufficient.
Ack.
>
> >I think it is better to
> >Use any other MACRO which define maximum number of port map
> >parameter.
> >If you consider it, then it can also be updated in documentation.
> >
> >>+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",
> >
> >IMO, Indentation changes for other parameters should in separate
> >patch.
>
> It's not a functionality change it just minor code formatting change and doesn’t
> deserve a separate patch.
Ack.
>
> >> prgname);
> >> }
> >>
> >>@@ -319,6 +331,61 @@ l2fwd_parse_portmask(const char
> >*portmask)
> >> return pm;
> >> }
> >>
@@ -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
-------------
@@ -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,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)
+
+* --portmap="(port,port)[,(port,port)]": Determines which ports are mapped to
+ which ports for packet forwarding.
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 --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.
@@ -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;
+
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 +