DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v1] app/testpmd: revert setting default rss
@ 2018-11-09  9:02 Ophir Munk
  2018-11-09  9:38 ` [dpdk-dev] [PATCH v2] app/testpmd: revert setting default RSS Ophir Munk
  0 siblings, 1 reply; 6+ messages in thread
From: Ophir Munk @ 2018-11-09  9:02 UTC (permalink / raw)
  To: Wenzhuo Lu, Jingjing Wu, Bernard Iremonger, dev, Adrien Mazarguil
  Cc: Asaf Penso, Shahaf Shuler, Thomas Monjalon, Olga Shern, 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [dpdk-dev] [PATCH v2] app/testpmd: revert setting default RSS
  2018-11-09  9:02 [dpdk-dev] [PATCH v1] app/testpmd: revert setting default rss Ophir Munk
@ 2018-11-09  9:38 ` Ophir Munk
       [not found]   ` <CGME20181109104253eucas1p1b60fccca0a181cc6bb974a3aea697748@eucas1p1.samsung.com>
  2018-11-12 14:49   ` [dpdk-dev] [dpdk-stable] [PATCH v2] " Ferruh Yigit
  0 siblings, 2 replies; 6+ messages in thread
From: Ophir Munk @ 2018-11-09  9:38 UTC (permalink / raw)
  To: Wenzhuo Lu, Jingjing Wu, Bernard Iremonger, dev, Adrien Mazarguil
  Cc: Asaf Penso, Shahaf Shuler, Thomas Monjalon, Olga Shern,
	Ophir Munk, stable

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [v2] app/testpmd: revert setting default RSS
       [not found]   ` <CGME20181109104253eucas1p1b60fccca0a181cc6bb974a3aea697748@eucas1p1.samsung.com>
@ 2018-11-09 10:42     ` Ilya Maximets
  2018-11-11  9:56       ` Ophir Munk
  0 siblings, 1 reply; 6+ messages in thread
From: Ilya Maximets @ 2018-11-09 10:42 UTC (permalink / raw)
  To: Ophir Munk, Wenzhuo Lu, Jingjing Wu, Bernard Iremonger, dev,
	Adrien Mazarguil
  Cc: Asaf Penso, Shahaf Shuler, Thomas Monjalon, Olga Shern, stable,
	Ferruh Yigit

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [v2] app/testpmd: revert setting default RSS
  2018-11-09 10:42     ` [dpdk-dev] [v2] " Ilya Maximets
@ 2018-11-11  9:56       ` Ophir Munk
  2018-11-11 11:41         ` Shahaf Shuler
  0 siblings, 1 reply; 6+ messages in thread
From: Ophir Munk @ 2018-11-11  9:56 UTC (permalink / raw)
  To: Ilya Maximets, Wenzhuo Lu, Jingjing Wu, Bernard Iremonger, dev,
	Adrien Mazarguil
  Cc: Asaf Penso, Shahaf Shuler, Thomas Monjalon, Olga Shern, stable,
	Ferruh Yigit



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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [v2] app/testpmd: revert setting default RSS
  2018-11-11  9:56       ` Ophir Munk
@ 2018-11-11 11:41         ` Shahaf Shuler
  0 siblings, 0 replies; 6+ messages in thread
From: Shahaf Shuler @ 2018-11-11 11:41 UTC (permalink / raw)
  To: Ophir Munk, Ilya Maximets, Wenzhuo Lu, Jingjing Wu,
	Bernard Iremonger, dev, Adrien Mazarguil
  Cc: Asaf Penso, Thomas Monjalon, Olga Shern, stable, Ferruh Yigit

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] app/testpmd: revert setting default RSS
  2018-11-09  9:38 ` [dpdk-dev] [PATCH v2] app/testpmd: revert setting default RSS Ophir Munk
       [not found]   ` <CGME20181109104253eucas1p1b60fccca0a181cc6bb974a3aea697748@eucas1p1.samsung.com>
@ 2018-11-12 14:49   ` Ferruh Yigit
  1 sibling, 0 replies; 6+ messages in thread
From: Ferruh Yigit @ 2018-11-12 14:49 UTC (permalink / raw)
  To: Ophir Munk, Wenzhuo Lu, Jingjing Wu, Bernard Iremonger, dev,
	Adrien Mazarguil
  Cc: Asaf Penso, Shahaf Shuler, Thomas Monjalon, Olga Shern, stable

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-11-12 14:49 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-09  9:02 [dpdk-dev] [PATCH v1] app/testpmd: revert setting default rss Ophir Munk
2018-11-09  9:38 ` [dpdk-dev] [PATCH v2] app/testpmd: revert setting default RSS Ophir Munk
     [not found]   ` <CGME20181109104253eucas1p1b60fccca0a181cc6bb974a3aea697748@eucas1p1.samsung.com>
2018-11-09 10:42     ` [dpdk-dev] [v2] " Ilya Maximets
2018-11-11  9:56       ` Ophir Munk
2018-11-11 11:41         ` Shahaf Shuler
2018-11-12 14:49   ` [dpdk-dev] [dpdk-stable] [PATCH v2] " Ferruh Yigit

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git