diff mbox series

[v2] app/testpmd: revert setting default RSS

Message ID 1541756283-16251-1-git-send-email-ophirmu@mellanox.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] app/testpmd: revert setting default RSS |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Ophir Munk Nov. 9, 2018, 9:38 a.m. UTC 
  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(-)

Comments

Ilya Maximets Nov. 9, 2018, 10:42 a.m. UTC | #1 
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.
Ophir Munk Nov. 11, 2018, 9:56 a.m. UTC | #2 
> -----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.
Shahaf Shuler Nov. 11, 2018, 11:41 a.m. UTC | #3 
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.
Ferruh Yigit Nov. 12, 2018, 2:49 p.m. UTC | #4 
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.
diff mbox series

Patch

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;
 }