[dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration

[dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration

[Lijun Ou]

When start the testpmd, the pmd driver initializes the RSS
configuration. Generally, the recommended RSS hash key is
used as the default key in the driver. In addition, the
default key is different from the default RSS flow in testpmd
without specifying RSS hash key. So. if you do not specify
the RSS key when creating an RSS rule, the testpmd uses the
default key as the default RSS key of the RSS rule. As a result,
you may mistakenly consider that the RSS key in use is the valid
default key of the NIC, actually, the key and the valid default
key of the NIC are two values.

Consider the follow usage with testpmd:
1. first, startup testpmd:
testpmd> show port 0 rss-hash key
RSS functions:
  all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
RSS key:
-6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
-20C6A42B73BBEAC01FA
2. create a rss rule
testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
types ipv4-udp end queues end / end

3. show rss-hash key
testpmd> show port 0 rss-hash key
RSS functions:
  all ipv4-udp udp
RSS key:
-74657374706D6427732064656661756C74205253532068617368206B65792C206F
-76657272696465

In order to solve the above problems, it use the NIC valid default
RSS key instead of the testpmd dummy RSS key in the flow configuration
when the RSS key is not specified in the flow rule. If the NIC RSS key
is invalid, it will use testpmd dummy RSS key as the default key.

Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
Cc: stable@dpdk.org

Signed-off-by: Lijun Ou <oulijun@huawei.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
---
V4->V5:
-rewrite the commit log
-add reviewed-by

V3->V4:
-fix checkpatch warning and shorter commit content.

V2->V3:
-fix checkpatch warning.

V1->V2:
-fix the commit.
---
 app/test-pmd/cmdline_flow.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 84bba0f..578555e 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -4735,6 +4735,7 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
 		action_rss_data->queue[i] = i;
 	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
 	    ctx->port != (portid_t)RTE_PORT_ALL) {
+		struct rte_eth_rss_conf rss_conf = {0};
 		struct rte_eth_dev_info info;
 		int ret2;
 
@@ -4745,6 +4746,13 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
 		action_rss_data->conf.key_len =
 			RTE_MIN(sizeof(action_rss_data->key),
 				info.hash_key_size);
+
+		rss_conf.rss_key_len = sizeof(action_rss_data->key);
+		rss_conf.rss_key = action_rss_data->key;
+		ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port, &rss_conf);
+		if (ret2 != 0)
+			return ret2;
+		action_rss_data->conf.key = rss_conf.rss_key;
 	}
 	action->conf = &action_rss_data->conf;
 	return ret;
-- 
2.7.4

[dpdk-dev] [PATCH] app/testpmd: set default RSS key as null

[Lijun Ou]

From: Ophir Munk <ophirmu@mellanox.com>

When creating an RSS rule without specifying a key (see [1]) it is
expected that the device will use the default key.
A NULL key is used to indicate to a PMD it should use
its default key, however testpmd assigns a non-NULL dummy key
(see [2]) instead.
This does not enable testing any PMD behavior when the RSS key is not
specified. This commit fixes this limitation by setting key to NULL.
Also, it fixes the Scenario [3] that enable default RSS action by
setting key=NULL and key_len!=0.
[1]
RSS rule example without specifying a key:
flow create 0 ingress <pattern> / end actions rss queues 0 1 end / end
[2]
Testpmd default key assignment:
.key= "testpmd's default RSS hash key, "
"override it for better balancing"
[3]
flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end

fixes refer to the link: https://patches.dpdk.org/patch/80898/

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
 app/test-pmd/cmdline_flow.c  | 19 ++-----------------
 lib/librte_ethdev/rte_flow.c |  2 +-
 2 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index cd35d5b..3d1dd05 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -4850,30 +4850,15 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
 			.func = RTE_ETH_HASH_FUNCTION_DEFAULT,
 			.level = 0,
 			.types = rss_hf,
-			.key_len = sizeof(action_rss_data->key),
+			.key_len = 0,
 			.queue_num = RTE_MIN(nb_rxq, ACTION_RSS_QUEUE_NUM),
-			.key = action_rss_data->key,
+			.key = NULL,
 			.queue = action_rss_data->queue,
 		},
-		.key = "testpmd's default RSS hash key, "
-			"override it for better balancing",
 		.queue = { 0 },
 	};
 	for (i = 0; i < action_rss_data->conf.queue_num; ++i)
 		action_rss_data->queue[i] = i;
-	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
-	    ctx->port != (portid_t)RTE_PORT_ALL) {
-		struct rte_eth_dev_info info;
-		int ret2;
-
-		ret2 = rte_eth_dev_info_get(ctx->port, &info);
-		if (ret2 != 0)
-			return ret2;
-
-		action_rss_data->conf.key_len =
-			RTE_MIN(sizeof(action_rss_data->key),
-				info.hash_key_size);
-	}
 	action->conf = &action_rss_data->conf;
 	return ret;
 }
diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
index d3e5cbc..a06f64c 100644
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -577,7 +577,7 @@ rte_flow_conv_action_conf(void *buf, const size_t size,
 			   }),
 			   size > sizeof(*dst.rss) ? sizeof(*dst.rss) : size);
 		off = sizeof(*dst.rss);
-		if (src.rss->key_len) {
+		if (src.rss->key_len && src.rss->key) {
 			off = RTE_ALIGN_CEIL(off, sizeof(*dst.rss->key));
 			tmp = sizeof(*src.rss->key) * src.rss->key_len;
 			if (size >= off + tmp)
-- 
2.7.4

Re: [dpdk-dev] [PATCH] app/testpmd: set default RSS key as null

[Ferruh Yigit]

On 10/21/2020 11:07 AM, Lijun Ou wrote:
> From: Ophir Munk <ophirmu@mellanox.com>
> 
> When creating an RSS rule without specifying a key (see [1]) it is
> expected that the device will use the default key.
> A NULL key is used to indicate to a PMD it should use
> its default key, however testpmd assigns a non-NULL dummy key
> (see [2]) instead.
> This does not enable testing any PMD behavior when the RSS key is not
> specified. This commit fixes this limitation by setting key to NULL.
> Also, it fixes the Scenario [3] that enable default RSS action by
> setting key=NULL and key_len!=0.
> [1]
> RSS rule example without specifying a key:
> flow create 0 ingress <pattern> / end actions rss queues 0 1 end / end
> [2]
> Testpmd default key assignment:
> .key= "testpmd's default RSS hash key, "
> "override it for better balancing"
> [3]
> flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end
> 
> fixes refer to the link: https://patches.dpdk.org/patch/80898/
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>

Applied to dpdk-next-net/main, thanks.


Updated the commit log as below:

Author: Lijun Ou <oulijun@huawei.com>
Date:   Wed Oct 21 18:07:10 2020 +0800

     app/testpmd: fix RSS key for flow API RSS rule

     When a flow API RSS rule is issued in testpmd, device RSS key is changed
     unexpectedly, device RSS key is changed to the testpmd default RSS key.

     Consider the following usage with testpmd:
     1. first, startup testpmd:
      testpmd> show port 0 rss-hash key
      RSS functions: all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
      RSS key: 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
               20C6A42B73BBEAC01FA
     2. create a rss rule
      testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end \
               actions rss types ipv4-udp end queues end / end

     3. show rss-hash key
      testpmd> show port 0 rss-hash key
      RSS functions: all ipv4-udp udp
      RSS key: 74657374706D6427732064656661756C74205253532068617368206B65792
               C206F76657272696465

     This is because testpmd always sends a key with the RSS rule,
     if user provides a key as part of the rule that key is used, if user
     doesn't provide a key, testpmd default key is sent to the PMDs, which is
     causing device programmed RSS key to be changed.

     There was a previous attempt to fix the same issue [1], but it has been
     reverted back [2] because of the crash when 'key_len' is provided
     without 'key'.

     This patch follows the same approach with the initial fix [1] but also
     addresses the crash.

     After change, testpmd RSS key is 'NULL' by default, if user provides a
     key as part of rule it is used, if not no key is sent to the PMDs at all

     [1]
     Commit a4391f8bae85 ("app/testpmd: set default RSS key as null")

     [2]
     Commit f3698c3d09a6 ("app/testpmd: revert setting default RSS")

     Fixes: d0ad8648b1c5 ("app/testpmd: fix RSS flow action configuration")
     Cc: stable@dpdk.org

     Signed-off-by: Lijun Ou <oulijun@huawei.com>
     Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
     Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>