Re: [dpdk-dev] [PATCH v2] app/testpmd: fix testpmd failure due to RSS offload check

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, April 25, 2018 8:04 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; adrien.mazarguil@6wind.com
> Cc: thomas@monjalon.net; dev@dpdk.org
> Subject: Re: [PATCH v2] app/testpmd: fix testpmd failure due to RSS offload
> check
> 
> On 4/25/2018 8:43 AM, Qi Zhang wrote:
> > After add RSS hash offload check, default rss_hf will fail on devices
> > that not support all bits, the patch introduce RSS negotiate flag,
> > when it is set, RSS configuration will negotiate with device
> > capability. By default negotiate is turn on, so it will not break
> > exist PMD. It can be disabled by "--disable-rss-neg"
> > in start parameters or be enable/disable by testpmd command "port
> > config all rss neg|noneg".
> 
> Hi Qi,
> 
> Thanks for the patch, but I feel new neg|noneg is extra complexity, I am
> wondering if we can fix this without this complexity.
> 
> "rss_hf" is rss hash function configuration for *all* ports. Default value is
> ETH_RSS_IP.
> 
> Up until recently it was more like default value, Adrien's commit start
> updating it in "port config all rss ..." and it become more like config value
> now.
> 
> And if it is config value, the question is what to do if PMD doesn't support
> requested hash function:
> - Fail
> - Convert request what PMD supports.
> 
> init_port_config() called from many places and it sets rss_conf.rss_hf via
> "rss_hf" and currently this is the problem some pmds hit.
> 
> Previously, before Adrien's update, when "port config all rss ..." failed it was
> printing log and keep continue, not recording the failed value as config value.
> 
> 
> What do you think:
> - In init_port_config() use "rss_hf & dev_info.flow_type_rss_offloads" as you
> suggested.
> - in cmd_config_rss_parsed() set "rss_hf" only if all ethdev successfully called
> rte_eth_dev_rss_hash_update(), this will mean the config has supported by
> all ports so it will be ok to use is as "rss_hf &
> dev_info.flow_type_rss_offloads"
> in init_port_config()
> - I think there is a defect in "port config all rss default", perhaps because of
> me during merged, it shouldn't update "rss_hf"
> 
> After this changes answer to above question, by default config behavior is
> "Convert request what PMD supports" so testpmd don't fail, and when
> configuration updated only values supported by ports accepted.

Ok, thanks for share the background, I'm ok with your suggestion, please check v3.

Regards
Qi
> 
> 
> 
> >
> > Fixes: 527624c663f8 ("ethdev: add supported hash function check")
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> >
> > v2:
> > - fix command help string.
> >
> >  app/test-pmd/cmdline.c                      | 22
> ++++++++++++++++------
> >  app/test-pmd/parameters.c                   |  5 +++++
> >  app/test-pmd/testpmd.c                      | 12 +++++++++++-
> >  app/test-pmd/testpmd.h                      |  1 +
> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++-
> >  5 files changed, 36 insertions(+), 8 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 333852194..9390be741 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -822,7 +822,7 @@ static void cmd_help_long_parsed(void
> *parsed_result,
> >  			" for ports.\n\n"
> >
> >  			"port config all rss (all|default|ip|tcp|udp|sctp|"
> > -			"ether|port|vxlan|geneve|nvgre|none|<flowtype_id>)\n"
> > +
> 	"ether|port|vxlan|geneve|nvgre|none|neg|noneg|<flowtype_id>)\n"
> >  			"    Set the RSS mode.\n\n"
> >
> >  			"port config port-id rss reta (hash,queue)[,(hash,queue)]\n"
> > @@ -2029,7 +2029,13 @@ cmd_config_rss_parsed(void *parsed_result,
> >  	else if (isdigit(res->value[0]) && atoi(res->value) > 0 &&
> >  						atoi(res->value) < 64)
> >  		rss_conf.rss_hf = 1ULL << atoi(res->value);
> > -	else {
> > +	else if (!strcmp(res->value, "neg")) {
> > +		rss_hf_noneg = 0;
> > +		return;
> > +	} else if (!strcmp(res->value, "noneg")) {
> > +		rss_hf_noneg = 1;
> > +		return;
> > +	} else {
> >  		printf("Unknown parameter\n");
> >  		return;
> >  	}
> > @@ -2037,10 +2043,14 @@ cmd_config_rss_parsed(void *parsed_result,
> >  	/* Update global configuration for RSS types. */
> >  	rss_hf = rss_conf.rss_hf;
> >  	RTE_ETH_FOREACH_DEV(i) {
> > -		if (!strcmp(res->value, "default")) {
> > -			rte_eth_dev_info_get(i, &dev_info);
> > +		rte_eth_dev_info_get(i, &dev_info);
> > +		if (!strcmp(res->value, "default"))
> >  			rss_conf.rss_hf = dev_info.flow_type_rss_offloads;
> > -		}
> > +		else
> > +			rss_conf.rss_hf &= (rss_hf_noneg ?
> > +				rss_conf.rss_hf :
> > +				dev_info.flow_type_rss_offloads);
> > +
> >  		diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
> >  		if (diag < 0)
> >  			printf("Configuration of RSS hash at ethernet port %d "
> > @@ -2064,7 +2074,7 @@ cmdline_parse_inst_t cmd_config_rss = {
> >  	.f = cmd_config_rss_parsed,
> >  	.data = NULL,
> >  	.help_str = "port config all rss "
> > -
> 	"all|default|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none|<fl
> owtype_id>",
> > +
> >
> +"all|default|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none|neg|n
> > +oneg|<flowtype_id>",
> >  	.tokens = {
> >  		(void *)&cmd_config_rss_port,
> >  		(void *)&cmd_config_rss_keyword,
> > diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> > index 394fa6d92..4758ec1d9 100644
> > --- a/app/test-pmd/parameters.c
> > +++ b/app/test-pmd/parameters.c
> > @@ -139,6 +139,7 @@ usage(char* progname)
> >  	printf("  --enable-hw-vlan-extend: enable hardware vlan extend.\n");
> >  	printf("  --enable-drop-en: enable per queue packet drop.\n");
> >  	printf("  --disable-rss: disable rss.\n");
> > +	printf("  --disable-rss-neg: disable rss negotiate with device
> > +capa.\n");
> >  	printf("  --port-topology=N: set port topology (N: paired (default) or "
> >  	       "chained).\n");
> >  	printf("  --forward-mode=N: set forwarding mode (N: %s).\n", @@
> > -594,6 +595,7 @@ launch_args_parse(int argc, char** argv)
> >  		{ "enable-hw-vlan-extend",      0, 0, 0 },
> >  		{ "enable-drop-en",            0, 0, 0 },
> >  		{ "disable-rss",                0, 0, 0 },
> > +		{ "disable-rss-neg",            0, 0, 0 },
> >  		{ "port-topology",              1, 0, 0 },
> >  		{ "forward-mode",               1, 0, 0 },
> >  		{ "rss-ip",			0, 0, 0 },
> > @@ -909,6 +911,9 @@ launch_args_parse(int argc, char** argv)
> >
> >  			if (!strcmp(lgopts[opt_idx].name, "disable-rss"))
> >  				rss_hf = 0;
> > +			if (!strcmp(lgopts[opt_idx].name, "disable-rss-neg")) {
> > +				rss_hf_noneg = 1;
> > +			}
> >  			if (!strcmp(lgopts[opt_idx].name, "port-topology")) {
> >  				if (!strcmp(optarg, "paired"))
> >  					port_topology = PORT_TOPOLOGY_PAIRED; diff --git
> > a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > d6da41927..68214677c 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -257,6 +257,12 @@ int16_t tx_rs_thresh = RTE_PMD_PARAM_UNSET;
> > uint64_t rss_hf = ETH_RSS_IP; /* RSS IP by default. */
> >
> >  /*
> > + * Negotiate with device capability when config
> > + * Receive Side Scaling (RSS).
> > + */
> > +uint8_t rss_hf_noneg = 0; /* Negotiate by default */
> > +
> > +/*
> >   * Port topology configuration
> >   */
> >  uint16_t port_topology = PORT_TOPOLOGY_PAIRED; /* Ports are paired
> by
> > default */ @@ -2265,13 +2271,17 @@ init_port_config(void)  {
> >  	portid_t pid;
> >  	struct rte_port *port;
> > +	struct rte_eth_dev_info dev_info;
> >
> >  	RTE_ETH_FOREACH_DEV(pid) {
> >  		port = &ports[pid];
> >  		port->dev_conf.fdir_conf = fdir_conf;
> >  		if (nb_rxq > 1) {
> > +			rte_eth_dev_info_get(pid, &dev_info);
> >  			port->dev_conf.rx_adv_conf.rss_conf.rss_key = NULL;
> > -			port->dev_conf.rx_adv_conf.rss_conf.rss_hf = rss_hf;
> > +			port->dev_conf.rx_adv_conf.rss_conf.rss_hf =
> > +				rss_hf_noneg ? rss_hf :
> > +				(rss_hf & dev_info.flow_type_rss_offloads);
> >  		} else {
> >  			port->dev_conf.rx_adv_conf.rss_conf.rss_key = NULL;
> >  			port->dev_conf.rx_adv_conf.rss_conf.rss_hf = 0; diff --git
> > a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> > 070919822..c37c4ee70 100644
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> > @@ -385,6 +385,7 @@ extern struct rte_eth_rxmode rx_mode;  extern
> > struct rte_eth_txmode tx_mode;
> >
> >  extern uint64_t rss_hf;
> > +extern uint8_t rss_hf_noneg;
> >
> >  extern queueid_t nb_rxq;
> >  extern queueid_t nb_txq;
> > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > index 593b13a3d..8db9cf5c1 100644
> > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > @@ -1751,13 +1751,15 @@ port config - RSS
> >
> >  Set the RSS (Receive Side Scaling) mode on or off::
> >
> > -   testpmd> port config all rss
> (all|default|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none)
> > +   testpmd> port config all rss
> > +
> (all|default|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none|neg|
> > + noneg)
> >
> >  RSS is on by default.
> >
> >  The ``all`` option is equivalent to ip|tcp|udp|sctp|ether.
> >  The ``default`` option enables all supported RSS types reported by device
> info.
> >  The ``none`` option is equivalent to the ``--disable-rss`` command-line
> option.
> > +The ``neg`` option enable the negotiation with device capability when
> configure RSS.
> > +The ``noneg`` option disable the negotiation and is equivalent to
> ``--disable-rss-neg`` command-line option.
> >
> >  port config - RSS Reta
> >  ~~~~~~~~~~~~~~~~~~~~~~
> >