[dpdk-dev] [PATCH v1] app/testpmd: revert setting default rss

[dpdk-dev] [PATCH v1] app/testpmd: revert setting default rss

[Ophir Munk]

This reverts the patch that enables default RSS action by setting
key=NULL and key_len=0.
In current testpmd implementation a key pointer must exist if
key_len!=0. For example, the following flow rule will cause a
segmentation fault:
flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end

Fixes: a4391f8bae ("app/testpmd: set default RSS key as null")

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
 app/test-pmd/cmdline_flow.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 91e2e35..23ea7cc 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -3248,15 +3248,26 @@ static int comp_vc_action_rss_queue(struct context *, const struct token *,
 			.func = RTE_ETH_HASH_FUNCTION_DEFAULT,
 			.level = 0,
 			.types = rss_hf,
-			.key_len = 0,
+			.key_len = sizeof(action_rss_data->key),
 			.queue_num = RTE_MIN(nb_rxq, ACTION_RSS_QUEUE_NUM),
-			.key = NULL,
+			.key = action_rss_data->key,
 			.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;
+
+		rte_eth_dev_info_get(ctx->port, &info);
+		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;
 }
-- 
1.8.3.1

[dpdk-dev] [PATCH v2] app/testpmd: revert setting default RSS

[Ophir Munk]

This reverts the patch that enables default RSS action by setting
key=NULL and key_len=0.
In current testpmd implementation a key pointer must exist if
key_len!=0. For example, the following flow rule will cause a
segmentation fault:
flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end

Fixes: a4391f8bae ("app/testpmd: set default RSS key as null")
Cc: stable@dpdk.org

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
v1:
Initial version

v2:
Update commit message (add Cc: stable@dpdk.org)

 app/test-pmd/cmdline_flow.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 91e2e35..23ea7cc 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -3248,15 +3248,26 @@ static int comp_vc_action_rss_queue(struct context *, const struct token *,
 			.func = RTE_ETH_HASH_FUNCTION_DEFAULT,
 			.level = 0,
 			.types = rss_hf,
-			.key_len = 0,
+			.key_len = sizeof(action_rss_data->key),
 			.queue_num = RTE_MIN(nb_rxq, ACTION_RSS_QUEUE_NUM),
-			.key = NULL,
+			.key = action_rss_data->key,
 			.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;
+
+		rte_eth_dev_info_get(ctx->port, &info);
+		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;
 }
-- 
1.8.3.1

Re: [dpdk-dev] [v2] app/testpmd: revert setting default RSS

[Ilya Maximets]

On 09.11.2018 12:38, Ophir Munk wrote:
> This reverts the patch that enables default RSS action by setting
> key=NULL and key_len=0.
> In current testpmd implementation a key pointer must exist if
> key_len!=0. For example, the following flow rule will cause a
> segmentation fault:
> flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end

Maybe it's better to check that 'key_len' and 'key' passed both or none?

BTW, is there any profit from the 'key_len' argument for testpmd?
Can we just always use the size of the passed 'key' and drop the
configurable from the user interface?

Best regards, Ilya Maximets.

Re: [dpdk-dev] [v2] app/testpmd: revert setting default RSS

[Ophir Munk]

> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Friday, November 09, 2018 12:43 PM
> To: Ophir Munk <ophirmu@mellanox.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Bernard
> Iremonger <bernard.iremonger@intel.com>; dev@dpdk.org; Adrien
> Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Asaf Penso <asafp@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> Olga Shern <olgas@mellanox.com>; stable@dpdk.org; Ferruh Yigit
> <ferruh.yigit@intel.com>
> Subject: Re: [v2] app/testpmd: revert setting default RSS
> 
> On 09.11.2018 12:38, Ophir Munk wrote:
> > This reverts the patch that enables default RSS action by setting
> > key=NULL and key_len=0.
> > In current testpmd implementation a key pointer must exist if
> > key_len!=0. For example, the following flow rule will cause a
> > segmentation fault:
> > flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end
> 
> Maybe it's better to check that 'key_len' and 'key' passed both or none?

I agree. However I don't see this option easily added to current testpmd flow implementation.
Adrien - how would you recommend adding this check?
Please note that currently if no key and no key_len are specified - testpmd still assign a dummy string.

> 
> BTW, is there any profit from the 'key_len' argument for testpmd?
> Can we just always use the size of the passed 'key' and drop the configurable
> from the user interface?
> 

If you just specify key without key_len then the key length is calculated implicitly from the key itself. So this is an already implemented feature. You can still use key_len (with different values) maybe for special case handling / debugging in the PMD.

> Best regards, Ilya Maximets.

Re: [dpdk-dev] [v2] app/testpmd: revert setting default RSS

[Shahaf Shuler]

Sunday, November 11, 2018 11:56 AM, Ophir Munk:
> Subject: RE: [v2] app/testpmd: revert setting default RSS
> > On 09.11.2018 12:38, Ophir Munk wrote:
> > > This reverts the patch that enables default RSS action by setting
> > > key=NULL and key_len=0.
> > > In current testpmd implementation a key pointer must exist if
> > > key_len!=0. For example, the following flow rule will cause a
> > > segmentation fault:
> > > flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end
> >
> > Maybe it's better to check that 'key_len' and 'key' passed both or none?
> 
> I agree. However I don't see this option easily added to current testpmd flow
> implementation.
> Adrien - how would you recommend adding this check?
> Please note that currently if no key and no key_len are specified - testpmd
> still assign a dummy string.

AFAIU, this patch is to restore the previous behavior of testpmd. it might not be perfect, yet worked. 
The current mode is that testpmd is broken. 

So I suggest to take this patch as is, and have the optimization later/in other release.

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] app/testpmd: revert setting default RSS

[Ferruh Yigit]

On 11/9/2018 9:38 AM, Ophir Munk wrote:
> This reverts the patch that enables default RSS action by setting
> key=NULL and key_len=0.
> In current testpmd implementation a key pointer must exist if
> key_len!=0. For example, the following flow rule will cause a
> segmentation fault:
> flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end
> 
> Fixes: a4391f8bae ("app/testpmd: set default RSS key as null")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

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