[PATCH] app/testpmd: fix testpmd failure due to RSS offload check

[PATCH] app/testpmd: fix testpmd failure due to RSS offload check

[Qi Zhang]

After add RSS hash offload check, default rss_hf will fail on
devices that not support all bits, the patch take rss_hf as
a suggest value and only set bits that device supported base on
rte_eth_dev_get_info.

Fixes: 527624c663f8 ("ethdev: add supported hash function check")
Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 app/test-pmd/testpmd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index d6da41927..af3fc5388 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2265,13 +2265,16 @@ 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 & 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;
-- 
2.13.6

Re: [PATCH] app/testpmd: fix testpmd failure due to RSS offload check

[Ferruh Yigit]

On 4/24/2018 3:18 PM, Qi Zhang wrote:
> After add RSS hash offload check, default rss_hf will fail on
> devices that not support all bits, the patch take rss_hf as
> a suggest value and only set bits that device supported base on
> rte_eth_dev_get_info.
> 
> Fixes: 527624c663f8 ("ethdev: add supported hash function check")
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>  app/test-pmd/testpmd.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index d6da41927..af3fc5388 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2265,13 +2265,16 @@ 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 & dev_info.flow_type_rss_offloads;

"rss_hf" is a global variable, not per port. Adrien's patch [1] made that it has
been updated by "port config all rss ..." command, so that value can change.

Also Xueming's patch did it will set "0" if "default" parameter used [2], like
"port config all rss default"

So not sure if rss_hf is reliable at this point.

What do you think using dev_info.flow_type_rss_offloads directly?


[1] 75d5493eb302 ("app/testpmd: fix RSS flow action configuration")

[2] 8c1f4aff92a6 ("app/testpmd: new parameter for port config all RSS command")

>  		} else {
>  			port->dev_conf.rx_adv_conf.rss_conf.rss_key = NULL;
>  			port->dev_conf.rx_adv_conf.rss_conf.rss_hf = 0;
> 

Re: [PATCH] app/testpmd: fix testpmd failure due to RSS offload check

[Ferruh Yigit]

On 4/24/2018 3:39 PM, Ferruh Yigit wrote:
> On 4/24/2018 3:18 PM, Qi Zhang wrote:
>> After add RSS hash offload check, default rss_hf will fail on
>> devices that not support all bits, the patch take rss_hf as
>> a suggest value and only set bits that device supported base on
>> rte_eth_dev_get_info.
>>
>> Fixes: 527624c663f8 ("ethdev: add supported hash function check")
>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>> ---
>>  app/test-pmd/testpmd.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index d6da41927..af3fc5388 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -2265,13 +2265,16 @@ 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 & dev_info.flow_type_rss_offloads;
> 
> "rss_hf" is a global variable, not per port. Adrien's patch [1] made that it has
> been updated by "port config all rss ..." command, so that value can change.
> 
> Also Xueming's patch did it will set "0" if "default" parameter used [2], like
> "port config all rss default"
> 
> So not sure if rss_hf is reliable at this point.
> 
> What do you think using dev_info.flow_type_rss_offloads directly?

Prevent updating rss_hf in "port config all rss default" and this patch together
lgtm.

> 
> 
> [1] 75d5493eb302 ("app/testpmd: fix RSS flow action configuration")
> 
> [2] 8c1f4aff92a6 ("app/testpmd: new parameter for port config all RSS command")
> 
>>  		} else {
>>  			port->dev_conf.rx_adv_conf.rss_conf.rss_key = NULL;
>>  			port->dev_conf.rx_adv_conf.rss_conf.rss_hf = 0;
>>
> 

Re: [PATCH] app/testpmd: fix testpmd failure due to RSS offload check

[Adrien Mazarguil]

On Tue, Apr 24, 2018 at 05:13:46PM +0100, Ferruh Yigit wrote:
> On 4/24/2018 3:39 PM, Ferruh Yigit wrote:
> > On 4/24/2018 3:18 PM, Qi Zhang wrote:
> >> After add RSS hash offload check, default rss_hf will fail on
> >> devices that not support all bits, the patch take rss_hf as
> >> a suggest value and only set bits that device supported base on
> >> rte_eth_dev_get_info.
> >>
> >> Fixes: 527624c663f8 ("ethdev: add supported hash function check")
> >> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> >> ---
> >>  app/test-pmd/testpmd.c | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> >> index d6da41927..af3fc5388 100644
> >> --- a/app/test-pmd/testpmd.c
> >> +++ b/app/test-pmd/testpmd.c
> >> @@ -2265,13 +2265,16 @@ 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 & dev_info.flow_type_rss_offloads;
> > 
> > "rss_hf" is a global variable, not per port. Adrien's patch [1] made that it has
> > been updated by "port config all rss ..." command, so that value can change.
> > 
> > Also Xueming's patch did it will set "0" if "default" parameter used [2], like
> > "port config all rss default"
> > 
> > So not sure if rss_hf is reliable at this point.
> > 
> > What do you think using dev_info.flow_type_rss_offloads directly?
> 
> Prevent updating rss_hf in "port config all rss default" and this patch together
> lgtm.

I have to take back my previous reply [3] regarding the original series.

After testing it I confirm mlx4 is broken with testpmd as well. The fact
mlx4 doesn't implement the legacy RSS API means it won't ever satisfy the
global rss_hf settings and rte_eth_dev_configure() will always fail.

As an immediate fix, Qi's patch above should deal with this problem.

Now what's needed in testpmd is the ability to distinguish a *configured*
rss_hf value (--rss-ip, --rss-udp command-line parameters or interactively
through "port config all rss $anything_other_than_default") from a default
one (no command-line parameters or "port config all rss default").

I think a second global is needed:

 int rss_hf_enable = 0; /* Take rss_hf into account. */

If unset, rss_hf is never taken into account and PMDs are fed their default
settings (whatever they report as supported), otherwise rss_hf is
enforced as requested by user configuration.

> > [1] 75d5493eb302 ("app/testpmd: fix RSS flow action configuration")
> > 
> > [2] 8c1f4aff92a6 ("app/testpmd: new parameter for port config all RSS command")
> > 
> >>  		} else {
> >>  			port->dev_conf.rx_adv_conf.rss_conf.rss_key = NULL;
> >>  			port->dev_conf.rx_adv_conf.rss_conf.rss_hf = 0;
> >>
> > 
> 

[3] http://dpdk.org/ml/archives/dev/2018-April/097904.html

-- 
Adrien Mazarguil
6WIND

[PATCH] app/testpmd: fix testpmd failure due to RSS offload check

[Qi Zhang]

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".

Fixes: 527624c663f8 ("ethdev: add supported hash function check")
Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 app/test-pmd/cmdline.c                      | 20 +++++++++++++++-----
 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, 35 insertions(+), 7 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 333852194..d6b30e20a 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 "
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
 ~~~~~~~~~~~~~~~~~~~~~~
-- 
2.13.6