DPDK patches and discussions
 help / color / mirror / Atom feed
From: Vamsi Krishna Attunuru <vattunuru@marvell.com>
To: "Andrzej Ostruszka [C]" <aostruszka@marvell.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: "jerinjacobk@gmail.com" <jerinjacobk@gmail.com>,
	"bruce.richardson@intel.com" <bruce.richardson@intel.com>,
	"david.marchand@redhat.com" <david.marchand@redhat.com>,
	"grive@u256.net" <grive@u256.net>,
	Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
	"Pavan Nikhilesh Bhagavatula" <pbhagavatula@marvell.com>,
	"thomas@monjalon.net" <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [PATCH] [v1 1/1] examples/l2fwd: add cmdline option for forwarding port info
Date: Sun, 5 Apr 2020 03:49:00 +0000	[thread overview]
Message-ID: <MWHPR18MB1645274FEB94FE51A0613ECDA6C50@MWHPR18MB1645.namprd18.prod.outlook.com> (raw)
In-Reply-To: <829338ed-51e8-f045-8b5b-0e39a19504b3@marvell.com>



> -----Original Message-----
> From: Andrzej Ostruszka [C] <aostruszka@marvell.com>
> Sent: Friday, April 3, 2020 6:22 PM
> To: Vamsi Krishna Attunuru <vattunuru@marvell.com>; dev@dpdk.org
> Cc: jerinjacobk@gmail.com; bruce.richardson@intel.com;
> david.marchand@redhat.com; grive@u256.net; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>; thomas@monjalon.net
> Subject: Re: [PATCH] [v1 1/1] examples/l2fwd: add cmdline option for
> forwarding port info
> 
> 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

Thanks Andrzej,
Acked all the comments in V2.

  reply	other threads:[~2020-04-05  3:49 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-23 11:56 [dpdk-dev] [PATCH v1 1/1] bus/pci: probe PCI devices in whitelisted order vattunuru
2019-09-25  6:41 ` Slava Ovsiienko
2019-09-25  9:07   ` Gaëtan Rivet
2019-09-26  4:15     ` Vamsi Krishna Attunuru
2019-09-26  8:04       ` Gaëtan Rivet
2019-09-26  9:39         ` [dpdk-dev] [EXT] " Vamsi Krishna Attunuru
2019-09-30 12:51           ` [dpdk-dev] [PATCH v1] eal: add manual probing option Gaetan Rivet
2019-09-30 17:51             ` Aaron Conole
2019-10-01  7:28               ` Gaëtan Rivet
2019-10-01 12:57                 ` Aaron Conole
2019-09-30 18:53             ` Stephen Hemminger
2019-10-01  9:10               ` Gaëtan Rivet
2019-10-01  9:49                 ` Jerin Jacob
2019-10-01 14:09                   ` Gaëtan Rivet
2019-10-01 14:26                     ` Jerin Jacob
2019-10-03  7:58             ` [dpdk-dev] [PATCH v2] " Gaetan Rivet
2019-10-04 12:55               ` [dpdk-dev] [PATCH v3] " Gaetan Rivet
2019-10-07  1:27                 ` Vamsi Krishna Attunuru
2019-10-23  8:44                   ` Gaëtan Rivet
2019-10-25 11:59                 ` Jerin Jacob
2019-10-25 12:50                   ` Gaëtan Rivet
2019-10-25 13:24                     ` Jerin Jacob
2019-10-25 14:41                 ` [dpdk-dev] [PATCH v4] " Gaetan Rivet
2019-10-25 15:01                   ` Jerin Jacob
2019-10-25 15:46                   ` [dpdk-dev] [PATCH v5] " Gaetan Rivet
2019-10-25 15:51                     ` Jerin Jacob
2020-01-22 16:51                     ` Pavan Nikhilesh Bhagavatula
2020-01-23  9:20                       ` Gaetan Rivet
2020-01-23  9:58                     ` [dpdk-dev] [PATCH v7] " Gaetan Rivet
2020-02-03  5:16                       ` Pavan Nikhilesh Bhagavatula
2020-02-03 22:21                         ` Thomas Monjalon
2020-02-04 10:03                           ` Gaetan Rivet
2020-02-04 11:07                             ` Thomas Monjalon
2020-02-04 12:43                               ` Gaetan Rivet
2020-02-04 15:06                                 ` Thomas Monjalon
2020-02-04 16:02                                   ` Gaetan Rivet
2020-02-10 14:51                                     ` Jerin Jacob
2020-02-10 15:27                                       ` Thomas Monjalon
2020-02-10 16:33                                         ` Jerin Jacob
2020-04-03  3:30                                           ` [dpdk-dev] [PATCH] [v1 1/1] examples/l2fwd: add cmdline option for forwarding port info vattunuru
2020-04-03 12:51                                             ` Andrzej Ostruszka [C]
2020-04-05  3:49                                               ` Vamsi Krishna Attunuru [this message]
2020-04-05  3:52                                             ` [dpdk-dev] [PATCH] [v2 " vattunuru
2020-04-06  9:32                                               ` Andrzej Ostruszka [C]
2020-04-26 21:19                                               ` Thomas Monjalon
2020-04-27  7:59                                               ` [dpdk-dev] [PATCH v3] " pbhagavatula
2020-04-27  9:19                                                 ` Sunil Kumar Kori
2020-04-27  9:36                                                   ` Andrzej Ostruszka [C]
2020-04-27 10:14                                                     ` Sunil Kumar Kori
2020-04-27 16:38                                                   ` Pavan Nikhilesh Bhagavatula
2020-04-27 16:49                                                     ` Sunil Kumar Kori
2020-04-27 18:31                                                 ` [dpdk-dev] [PATCH v4] " pbhagavatula
2020-04-28  5:54                                                   ` Sunil Kumar Kori
2020-05-01 14:00                                                   ` Varghese, Vipin
2020-05-01 15:14                                                     ` Pavan Nikhilesh Bhagavatula
2020-05-02  4:34                                                       ` Varghese, Vipin
2020-05-11  0:23                                                         ` Pavan Nikhilesh Bhagavatula
2020-05-24 16:13                                                           ` Thomas Monjalon
2020-05-25  9:29                                                             ` Bruce Richardson
2020-07-04 13:36                                                               ` Jerin Jacob
2020-07-05 12:23                                                                 ` Thomas Monjalon
2020-04-04 16:34                                           ` [dpdk-dev] [EXT] Re: [PATCH v7] eal: add manual probing option Jerin Jacob Kollanukkaran
2023-06-14 19:33                       ` [dpdk-dev] " Stephen Hemminger
2023-06-26 16:12                         ` Gaëtan Rivet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=MWHPR18MB1645274FEB94FE51A0613ECDA6C50@MWHPR18MB1645.namprd18.prod.outlook.com \
    --to=vattunuru@marvell.com \
    --cc=aostruszka@marvell.com \
    --cc=bruce.richardson@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=grive@u256.net \
    --cc=jerinj@marvell.com \
    --cc=jerinjacobk@gmail.com \
    --cc=pbhagavatula@marvell.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).