[dpdk-dev] [PATCH] net/mlx5: set RSS key len 0 to indicate default RSS

[dpdk-dev] [PATCH] net/mlx5: set RSS key len 0 to indicate default RSS

[Ophir Munk]

Applications which add an RSS flow must supply an RSS key table and an
RSS key length. If an application needs to add the default RSS flow it
should not care about the exact RSS default key table and its length.
By setting key length to 0 - the PMD will know that it should use the
default RSS key table and length.

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 3f548a9..18eacf5 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -2062,7 +2062,8 @@ struct mlx5_flow_tunnel_info {
 					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
 					  &rss->level,
 					  "tunnel RSS is not supported");
-	if (rss->key_len < MLX5_RSS_HASH_KEY_LEN)
+	/* key_len 0 means using default RSS key */
+	if (rss->key_len > 0 && rss->key_len < MLX5_RSS_HASH_KEY_LEN)
 		return rte_flow_error_set(error, ENOTSUP,
 					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
 					  &rss->key_len,
@@ -2106,7 +2107,7 @@ struct mlx5_flow_tunnel_info {
 		memcpy((*flow->queue), rss->queue,
 		       rss->queue_num * sizeof(uint16_t));
 	flow->rss.queue_num = rss->queue_num;
-	memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
+	memcpy(flow->key, rss->key, rss->key_len);
 	flow->rss.types = rss->types;
 	flow->rss.level = rss->level;
 	flow->fate |= MLX5_FLOW_FATE_RSS;
@@ -2948,7 +2949,7 @@ struct mlx5_flow_tunnel_info {
 					     flow->rss.queue_num);
 			if (!hrxq)
 				hrxq = mlx5_hrxq_new(dev, flow->key,
-						     MLX5_RSS_HASH_KEY_LEN,
+						     flow->rss.key_len,
 						     verbs->hash_fields,
 						     (*flow->queue),
 						     flow->rss.queue_num,
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: set RSS key len 0 to indicate default RSS

[Yongseok Koh]

> On Oct 3, 2018, at 10:37 AM, Ophir Munk <ophirmu@mellanox.com> wrote:
> 
> Applications which add an RSS flow must supply an RSS key table and an
> RSS key length. If an application needs to add the default RSS flow it
> should not care about the exact RSS default key table and its length.
> By setting key length to 0 - the PMD will know that it should use the
> default RSS key table and length.
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
> drivers/net/mlx5/mlx5_flow.c | 7 ++++---

Please rebase the code on top of the latest dpdk-next-net-mlx.
Actually, it is better to rebase on top of PR#878.

Thanks,
Yongseok

> 1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 3f548a9..18eacf5 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -2062,7 +2062,8 @@ struct mlx5_flow_tunnel_info {
> 					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> 					  &rss->level,
> 					  "tunnel RSS is not supported");
> -	if (rss->key_len < MLX5_RSS_HASH_KEY_LEN)
> +	/* key_len 0 means using default RSS key */
> +	if (rss->key_len > 0 && rss->key_len < MLX5_RSS_HASH_KEY_LEN)
> 		return rte_flow_error_set(error, ENOTSUP,
> 					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> 					  &rss->key_len,
> @@ -2106,7 +2107,7 @@ struct mlx5_flow_tunnel_info {
> 		memcpy((*flow->queue), rss->queue,
> 		       rss->queue_num * sizeof(uint16_t));
> 	flow->rss.queue_num = rss->queue_num;
> -	memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
> +	memcpy(flow->key, rss->key, rss->key_len);
> 	flow->rss.types = rss->types;
> 	flow->rss.level = rss->level;
> 	flow->fate |= MLX5_FLOW_FATE_RSS;
> @@ -2948,7 +2949,7 @@ struct mlx5_flow_tunnel_info {
> 					     flow->rss.queue_num);
> 			if (!hrxq)
> 				hrxq = mlx5_hrxq_new(dev, flow->key,
> -						     MLX5_RSS_HASH_KEY_LEN,
> +						     flow->rss.key_len,
> 						     verbs->hash_fields,
> 						     (*flow->queue),
> 						     flow->rss.queue_num,
> -- 
> 1.8.3.1
> 

Re: [dpdk-dev] [PATCH] net/mlx5: set RSS key len 0 to indicate default RSS

[Shahaf Shuler]

Hi Ophir,

To add on top of Koh's comment

Wednesday, October 3, 2018 9:56 PM, Yongseok Koh:
> Subject: Re: [PATCH] net/mlx5: set RSS key len 0 to indicate default RSS
> > On Oct 3, 2018, at 10:37 AM, Ophir Munk <ophirmu@mellanox.com>
> wrote:
> >
> > Applications which add an RSS flow must supply an RSS key table and an
> > RSS key length. If an application needs to add the default RSS flow it
> > should not care about the exact RSS default key table and its length.
> > By setting key length to 0 - the PMD will know that it should use the
> > default RSS key table and length.

Can you refer where in the rte_flow API this is documented?  I couldn't find it under struct rte_flow_action_rss  definition. 
I agree it is better for the PMD to behave this way, but this should be explained in the API, so for this patch to be accepted we need also doc update in rte_flow.h.

In my opinion, it is better for the key to be NULL to request from the PMD to use the default key, similar to struct rte_eth_rss_conf. 

> >
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > ---
> > drivers/net/mlx5/mlx5_flow.c | 7 ++++---
> 
> Please rebase the code on top of the latest dpdk-next-net-mlx.
> Actually, it is better to rebase on top of PR#878.
> 
> Thanks,
> Yongseok
> 
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > b/drivers/net/mlx5/mlx5_flow.c index 3f548a9..18eacf5 100644
> > --- a/drivers/net/mlx5/mlx5_flow.c
> > +++ b/drivers/net/mlx5/mlx5_flow.c
> > @@ -2062,7 +2062,8 @@ struct mlx5_flow_tunnel_info {
> >
> RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> > 					  &rss->level,
> > 					  "tunnel RSS is not supported");
> > -	if (rss->key_len < MLX5_RSS_HASH_KEY_LEN)
> > +	/* key_len 0 means using default RSS key */
> > +	if (rss->key_len > 0 && rss->key_len < MLX5_RSS_HASH_KEY_LEN)
> > 		return rte_flow_error_set(error, ENOTSUP,
> >
> RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> > 					  &rss->key_len,
> > @@ -2106,7 +2107,7 @@ struct mlx5_flow_tunnel_info {
> > 		memcpy((*flow->queue), rss->queue,
> > 		       rss->queue_num * sizeof(uint16_t));
> > 	flow->rss.queue_num = rss->queue_num;
> > -	memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
> > +	memcpy(flow->key, rss->key, rss->key_len);

In Mellanox devices 40B of a key is a must. If you intended to copy 0 bytes, it is better to do it in other ways. 
 
> > 	flow->rss.types = rss->types;
> > 	flow->rss.level = rss->level;
> > 	flow->fate |= MLX5_FLOW_FATE_RSS;
> > @@ -2948,7 +2949,7 @@ struct mlx5_flow_tunnel_info {
> > 					     flow->rss.queue_num);
> > 			if (!hrxq)
> > 				hrxq = mlx5_hrxq_new(dev, flow->key,
> > -
> MLX5_RSS_HASH_KEY_LEN,
> > +						     flow->rss.key_len,
> > 						     verbs->hash_fields,
> > 						     (*flow->queue),
> > 						     flow->rss.queue_num,
> > --
> > 1.8.3.1
> >

Re: [dpdk-dev] [PATCH] net/mlx5: set RSS key len 0 to indicate default RSS

[Ophir Munk]

Please find comments inline.

> -----Original Message-----
> From: Yongseok Koh
> Sent: Wednesday, October 03, 2018 9:56 PM
> To: Ophir Munk <ophirmu@mellanox.com>
> Cc: dev@dpdk.org; Asaf Penso <asafp@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> Olga Shern <olgas@mellanox.com>
> Subject: Re: [PATCH] net/mlx5: set RSS key len 0 to indicate default RSS
> 
> 
> > On Oct 3, 2018, at 10:37 AM, Ophir Munk <ophirmu@mellanox.com>
> wrote:
> >
> > Applications which add an RSS flow must supply an RSS key table and an
> > RSS key length. If an application needs to add the default RSS flow it
> > should not care about the exact RSS default key table and its length.
> > By setting key length to 0 - the PMD will know that it should use the
> > default RSS key table and length.
> >
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > ---
> > drivers/net/mlx5/mlx5_flow.c | 7 ++++---
> 
> Please rebase the code on top of the latest dpdk-next-net-mlx.
> Actually, it is better to rebase on top of PR#878.
> 
> Thanks,
> Yongseok

I have rebased the code on master branch dated 18.11 28-Oct-18.
After the rebase I have discovered more seg fault scenarios which are fixed in V2.

> 
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > b/drivers/net/mlx5/mlx5_flow.c index 3f548a9..18eacf5 100644
> > --- a/drivers/net/mlx5/mlx5_flow.c
> > +++ b/drivers/net/mlx5/mlx5_flow.c
> > @@ -2062,7 +2062,8 @@ struct mlx5_flow_tunnel_info {
> >
> RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> > 					  &rss->level,
> > 					  "tunnel RSS is not supported");
> > -	if (rss->key_len < MLX5_RSS_HASH_KEY_LEN)
> > +	/* key_len 0 means using default RSS key */
> > +	if (rss->key_len > 0 && rss->key_len < MLX5_RSS_HASH_KEY_LEN)
> > 		return rte_flow_error_set(error, ENOTSUP,
> >
> RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> > 					  &rss->key_len,
> > @@ -2106,7 +2107,7 @@ struct mlx5_flow_tunnel_info {
> > 		memcpy((*flow->queue), rss->queue,
> > 		       rss->queue_num * sizeof(uint16_t));
> > 	flow->rss.queue_num = rss->queue_num;
> > -	memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
> > +	memcpy(flow->key, rss->key, rss->key_len);
> > 	flow->rss.types = rss->types;
> > 	flow->rss.level = rss->level;
> > 	flow->fate |= MLX5_FLOW_FATE_RSS;
> > @@ -2948,7 +2949,7 @@ struct mlx5_flow_tunnel_info {
> > 					     flow->rss.queue_num);
> > 			if (!hrxq)
> > 				hrxq = mlx5_hrxq_new(dev, flow->key,
> > -
> MLX5_RSS_HASH_KEY_LEN,
> > +						     flow->rss.key_len,
> > 						     verbs->hash_fields,
> > 						     (*flow->queue),
> > 						     flow->rss.queue_num,
> > --
> > 1.8.3.1
> >

Re: [dpdk-dev] [PATCH] net/mlx5: set RSS key len 0 to indicate default RSS

[Ophir Munk]

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Sunday, October 07, 2018 2:21 PM
> To: Yongseok Koh <yskoh@mellanox.com>; Ophir Munk
> <ophirmu@mellanox.com>
> Cc: dev@dpdk.org; Asaf Penso <asafp@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; Olga Shern <olgas@mellanox.com>
> Subject: RE: [PATCH] net/mlx5: set RSS key len 0 to indicate default RSS
> 
> > > On Oct 3, 2018, at 10:37 AM, Ophir Munk <ophirmu@mellanox.com>
> > wrote:
> > >
> > > Applications which add an RSS flow must supply an RSS key table and
> > > an RSS key length. If an application needs to add the default RSS
> > > flow it should not care about the exact RSS default key table and its
> length.
> > > By setting key length to 0 - the PMD will know that it should use
> > > the default RSS key table and length.
> 
> Can you refer where in the rte_flow API this is documented?  I couldn't find it
> under struct rte_flow_action_rss  definition.
> I agree it is better for the PMD to behave this way, but this should be
> explained in the API, so for this patch to be accepted we need also doc
> update in rte_flow.h.
> 

Will update documentation in v2

> In my opinion, it is better for the key to be NULL to request from the PMD to
> use the default key, similar to struct rte_eth_rss_conf.
> 

I suggest considering both (NULL key or key_len 0) as a criteria for using default RSS key.
In addition if type==0 it will be considered a default type and mlx5 will use ETH_RSS_IP
Will update in v2

> > > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > b/drivers/net/mlx5/mlx5_flow.c index 3f548a9..18eacf5 100644
> > > --- a/drivers/net/mlx5/mlx5_flow.c
> > > +++ b/drivers/net/mlx5/mlx5_flow.c
> > > @@ -2062,7 +2062,8 @@ struct mlx5_flow_tunnel_info {
> > >
> > > @@ -2106,7 +2107,7 @@ struct mlx5_flow_tunnel_info {
> > > 		memcpy((*flow->queue), rss->queue,
> > > 		       rss->queue_num * sizeof(uint16_t));
> > > 	flow->rss.queue_num = rss->queue_num;
> > > -	memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
> > > +	memcpy(flow->key, rss->key, rss->key_len);
> 
> In Mellanox devices 40B of a key is a must. If you intended to copy 0 bytes, it
> is better to do it in other ways.

rss->key_len can be 0 or 40. Please consider this memcpy() a generic code which fits both cases.

> 

[dpdk-dev] [PATCH v2] net/mlx5: set RSS key to NULL to indicate default RSS

[Ophir Munk]

Applications which add RSS rules must supply an RSS key and length.
If an application is only interested in default RSS operation it
should not care about the exact RSS key.
By setting the key to NULL - the PMD will use the default RSS key.
In addition if the application does not care about the RSS type it can
set it to 0 and the PMD will use the default type (ETH_RSS_IP).

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
 doc/guides/nics/mlx5.rst           |  1 +
 drivers/net/mlx5/mlx5_flow.c       |  8 +++++++-
 drivers/net/mlx5/mlx5_flow_dv.c    |  8 ++++++--
 drivers/net/mlx5/mlx5_flow_verbs.c |  9 +++++++--
 drivers/net/mlx5/mlx5_rxq.c        | 14 ++++++--------
 lib/librte_ethdev/rte_flow.h       |  2 +-
 6 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
index 6769628..de8f15b 100644
--- a/doc/guides/nics/mlx5.rst
+++ b/doc/guides/nics/mlx5.rst
@@ -54,6 +54,7 @@ Features
 - Support for scattered TX and RX frames.
 - IPv4, IPv6, TCPv4, TCPv6, UDPv4 and UDPv6 RSS on any number of queues.
 - Several RSS hash keys, one for each flow type.
+- NULL RSS key indicates default RSS key
 - Configurable RETA table.
 - Support for multiple MAC addresses.
 - VLAN filtering.
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 280af0a..5742989 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -912,7 +912,13 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
 					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
 					  &rss->level,
 					  "tunnel RSS is not supported");
-	if (rss->key_len < MLX5_RSS_HASH_KEY_LEN)
+	/* allow RSS key_len 0 in case of NULL (default) RSS key */
+	if (rss->key_len == 0 && rss->key != NULL)
+		return rte_flow_error_set(error, ENOTSUP,
+					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
+					  &rss->key_len,
+					  "RSS hash key length 0");
+	if (rss->key_len > 0 && rss->key_len < MLX5_RSS_HASH_KEY_LEN)
 		return rte_flow_error_set(error, ENOTSUP,
 					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
 					  &rss->key_len,
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 8f729f4..fd52bf4 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -1056,6 +1056,7 @@
 {
 	const struct rte_flow_action_queue *queue;
 	const struct rte_flow_action_rss *rss;
+	uint8_t	*rss_key;
 	int actions_n = dev_flow->dv.actions_n;
 	struct rte_flow *flow = dev_flow->flow;
 
@@ -1094,8 +1095,11 @@
 			memcpy((*flow->queue), rss->queue,
 			       rss->queue_num * sizeof(uint16_t));
 		flow->rss.queue_num = rss->queue_num;
-		memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
-		flow->rss.types = rss->types;
+		/* NULL RSS key indicates default RSS key */
+		rss_key = (!rss->key) ? rss_hash_default_key : rss->key;
+		memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);
+		/* RSS type 0 indicates default RSS type ETH_RSS_IP */
+		flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
 		flow->rss.level = rss->level;
 		/* Added to array only in apply since we need the QP */
 		flow->actions |= MLX5_FLOW_ACTION_RSS;
diff --git a/drivers/net/mlx5/mlx5_flow_verbs.c b/drivers/net/mlx5/mlx5_flow_verbs.c
index 81bc39f..ae9042f 100644
--- a/drivers/net/mlx5/mlx5_flow_verbs.c
+++ b/drivers/net/mlx5/mlx5_flow_verbs.c
@@ -925,14 +925,19 @@
 				struct mlx5_flow *dev_flow)
 {
 	const struct rte_flow_action_rss *rss = action->conf;
+	const uint8_t *rss_key;
 	struct rte_flow *flow = dev_flow->flow;
 
 	if (flow->queue)
 		memcpy((*flow->queue), rss->queue,
 		       rss->queue_num * sizeof(uint16_t));
 	flow->rss.queue_num = rss->queue_num;
-	memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
-	flow->rss.types = rss->types;
+
+	/* NULL RSS key indicates default RSS key */
+	rss_key = !rss->key ? rss_hash_default_key : rss->key;
+	memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);
+	/* RSS type 0 indicates default RSS type (ETH_RSS_IP) */
+	flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
 	flow->rss.level = rss->level;
 	*action_flags |= MLX5_FLOW_ACTION_RSS;
 }
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index ed993ea..36fc202 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -1783,10 +1783,6 @@ struct mlx5_hrxq *
 		rte_errno = ENOMEM;
 		return NULL;
 	}
-	if (!rss_key_len) {
-		rss_key_len = MLX5_RSS_HASH_KEY_LEN;
-		rss_key = rss_hash_default_key;
-	}
 #ifdef HAVE_IBV_DEVICE_TUNNEL_SUPPORT
 	qp = mlx5_glue->dv_create_qp
 		(priv->ctx,
@@ -1798,8 +1794,7 @@ struct mlx5_hrxq *
 				IBV_QP_INIT_ATTR_RX_HASH,
 			.rx_hash_conf = (struct ibv_rx_hash_conf){
 				.rx_hash_function = IBV_RX_HASH_FUNC_TOEPLITZ,
-				.rx_hash_key_len = rss_key_len ? rss_key_len :
-						   MLX5_RSS_HASH_KEY_LEN,
+				.rx_hash_key_len = rss_key_len,
 				.rx_hash_key = rss_key ?
 					       (void *)(uintptr_t)rss_key :
 					       rss_hash_default_key,
@@ -1824,8 +1819,7 @@ struct mlx5_hrxq *
 				IBV_QP_INIT_ATTR_RX_HASH,
 			.rx_hash_conf = (struct ibv_rx_hash_conf){
 				.rx_hash_function = IBV_RX_HASH_FUNC_TOEPLITZ,
-				.rx_hash_key_len = rss_key_len ? rss_key_len :
-						   MLX5_RSS_HASH_KEY_LEN,
+				.rx_hash_key_len = rss_key_len,
 				.rx_hash_key = rss_key ?
 					       (void *)(uintptr_t)rss_key :
 					       rss_hash_default_key,
@@ -1888,8 +1882,12 @@ struct mlx5_hrxq *
 	LIST_FOREACH(hrxq, &priv->hrxqs, next) {
 		struct mlx5_ind_table_ibv *ind_tbl;
 
+		if (!rss_key_len)
+			rss_key_len = MLX5_RSS_HASH_KEY_LEN;
 		if (hrxq->rss_key_len != rss_key_len)
 			continue;
+		if (!rss_key)
+			rss_key = rss_hash_default_key;
 		if (memcmp(hrxq->rss_key, rss_key, rss_key_len))
 			continue;
 		if (hrxq->hash_fields != hash_fields)
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index c0fe879..6cc72ab 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -1785,7 +1785,7 @@ struct rte_flow_action_rss {
 	uint64_t types; /**< Specific RSS hash types (see ETH_RSS_*). */
 	uint32_t key_len; /**< Hash key length in bytes. */
 	uint32_t queue_num; /**< Number of entries in @p queue. */
-	const uint8_t *key; /**< Hash key. */
+	const uint8_t *key; /**< Hash key. NULL should indicate default key */
 	const uint16_t *queue; /**< Queue indices to use. */
 };
 
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] net/mlx5: set RSS key to NULL to indicate default RSS

[Shahaf Shuler]

Hi Ophir, 

Thursday, November 1, 2018 10:59 AM, Ophir Munk:
> Subject: [PATCH v2] net/mlx5: set RSS key to NULL to indicate default RSS
> 
> Applications which add RSS rules must supply an RSS key and length.
> If an application is only interested in default RSS operation it should not care
> about the exact RSS key.
> By setting the key to NULL - the PMD will use the default RSS key.
> In addition if the application does not care about the RSS type it can set it to 0
> and the PMD will use the default type (ETH_RSS_IP).
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
>  doc/guides/nics/mlx5.rst           |  1 +
>  drivers/net/mlx5/mlx5_flow.c       |  8 +++++++-
>  drivers/net/mlx5/mlx5_flow_dv.c    |  8 ++++++--
>  drivers/net/mlx5/mlx5_flow_verbs.c |  9 +++++++--
>  drivers/net/mlx5/mlx5_rxq.c        | 14 ++++++--------
>  lib/librte_ethdev/rte_flow.h       |  2 +-
>  6 files changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst index
> 6769628..de8f15b 100644
> --- a/doc/guides/nics/mlx5.rst
> +++ b/doc/guides/nics/mlx5.rst
> @@ -54,6 +54,7 @@ Features
>  - Support for scattered TX and RX frames.
>  - IPv4, IPv6, TCPv4, TCPv6, UDPv4 and UDPv6 RSS on any number of queues.
>  - Several RSS hash keys, one for each flow type.
> +- NULL RSS key indicates default RSS key

I don't think it is a feature, it should be an API implementation. 

>  - Configurable RETA table.
>  - Support for multiple MAC addresses.
>  - VLAN filtering.
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 280af0a..5742989 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -912,7 +912,13 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
> 
> RTE_FLOW_ERROR_TYPE_ACTION_CONF,
>  					  &rss->level,
>  					  "tunnel RSS is not supported");
> -	if (rss->key_len < MLX5_RSS_HASH_KEY_LEN)
> +	/* allow RSS key_len 0 in case of NULL (default) RSS key */

Missing period. 

> +	if (rss->key_len == 0 && rss->key != NULL)
> +		return rte_flow_error_set(error, ENOTSUP,
> +
> RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> +					  &rss->key_len,
> +					  "RSS hash key length 0");
> +	if (rss->key_len > 0 && rss->key_len < MLX5_RSS_HASH_KEY_LEN)

Can't you simplify using a single condition: 
If (rss->key && rss->key_len < MLX5_RSS_HASH_KEY_LEN)

Same rss->key addition should be for the case the key_len is too large. 

>  		return rte_flow_error_set(error, ENOTSUP,
> 
> RTE_FLOW_ERROR_TYPE_ACTION_CONF,
>  					  &rss->key_len,
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c index 8f729f4..fd52bf4 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -1056,6 +1056,7 @@
>  {
>  	const struct rte_flow_action_queue *queue;
>  	const struct rte_flow_action_rss *rss;
> +	uint8_t	*rss_key;

Why no declare it inside the RSS case? 

>  	int actions_n = dev_flow->dv.actions_n;
>  	struct rte_flow *flow = dev_flow->flow;
> 
> @@ -1094,8 +1095,11 @@
>  			memcpy((*flow->queue), rss->queue,
>  			       rss->queue_num * sizeof(uint16_t));
>  		flow->rss.queue_num = rss->queue_num;
> -		memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
> -		flow->rss.types = rss->types;
> +		/* NULL RSS key indicates default RSS key */

Missing period. 

> +		rss_key = (!rss->key) ? rss_hash_default_key : rss->key;
> +		memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);

For consistency you should also set the key_len on the flow structure to MLX5_RSS_HASH_KEY_LEN.

> +		/* RSS type 0 indicates default RSS type ETH_RSS_IP */

Period. 

> +		flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
>  		flow->rss.level = rss->level;
>  		/* Added to array only in apply since we need the QP */
>  		flow->actions |= MLX5_FLOW_ACTION_RSS; diff --git
> a/drivers/net/mlx5/mlx5_flow_verbs.c
> b/drivers/net/mlx5/mlx5_flow_verbs.c
> index 81bc39f..ae9042f 100644
> --- a/drivers/net/mlx5/mlx5_flow_verbs.c
> +++ b/drivers/net/mlx5/mlx5_flow_verbs.c
> @@ -925,14 +925,19 @@
>  				struct mlx5_flow *dev_flow)
>  {
>  	const struct rte_flow_action_rss *rss = action->conf;
> +	const uint8_t *rss_key;
>  	struct rte_flow *flow = dev_flow->flow;
> 
>  	if (flow->queue)
>  		memcpy((*flow->queue), rss->queue,
>  		       rss->queue_num * sizeof(uint16_t));
>  	flow->rss.queue_num = rss->queue_num;
> -	memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
> -	flow->rss.types = rss->types;
> +
> +	/* NULL RSS key indicates default RSS key */

period

> +	rss_key = !rss->key ? rss_hash_default_key : rss->key;

For consistency you should also set the key_len on the flow structure to MLX5_RSS_HASH_KEY_LEN. 

> +	memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);
> +	/* RSS type 0 indicates default RSS type (ETH_RSS_IP) */

period

> +	flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
>  	flow->rss.level = rss->level;
>  	*action_flags |= MLX5_FLOW_ACTION_RSS;  } diff --git
> a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c index
> ed993ea..36fc202 100644
> --- a/drivers/net/mlx5/mlx5_rxq.c
> +++ b/drivers/net/mlx5/mlx5_rxq.c
> @@ -1783,10 +1783,6 @@ struct mlx5_hrxq *
>  		rte_errno = ENOMEM;
>  		return NULL;
>  	}
> -	if (!rss_key_len) {
> -		rss_key_len = MLX5_RSS_HASH_KEY_LEN;
> -		rss_key = rss_hash_default_key;
> -	}
>  #ifdef HAVE_IBV_DEVICE_TUNNEL_SUPPORT
>  	qp = mlx5_glue->dv_create_qp
>  		(priv->ctx,
> @@ -1798,8 +1794,7 @@ struct mlx5_hrxq *
>  				IBV_QP_INIT_ATTR_RX_HASH,
>  			.rx_hash_conf = (struct ibv_rx_hash_conf){
>  				.rx_hash_function =
> IBV_RX_HASH_FUNC_TOEPLITZ,
> -				.rx_hash_key_len = rss_key_len ?
> rss_key_len :
> -						   MLX5_RSS_HASH_KEY_LEN,
> +				.rx_hash_key_len = rss_key_len,
>  				.rx_hash_key = rss_key ?
>  					       (void *)(uintptr_t)rss_key :
>  					       rss_hash_default_key,
> @@ -1824,8 +1819,7 @@ struct mlx5_hrxq *
>  				IBV_QP_INIT_ATTR_RX_HASH,
>  			.rx_hash_conf = (struct ibv_rx_hash_conf){
>  				.rx_hash_function =
> IBV_RX_HASH_FUNC_TOEPLITZ,
> -				.rx_hash_key_len = rss_key_len ?
> rss_key_len :
> -						   MLX5_RSS_HASH_KEY_LEN,
> +				.rx_hash_key_len = rss_key_len,
>  				.rx_hash_key = rss_key ?
>  					       (void *)(uintptr_t)rss_key :
>  					       rss_hash_default_key,
> @@ -1888,8 +1882,12 @@ struct mlx5_hrxq *
>  	LIST_FOREACH(hrxq, &priv->hrxqs, next) {
>  		struct mlx5_ind_table_ibv *ind_tbl;
> 
> +		if (!rss_key_len)
> +			rss_key_len = MLX5_RSS_HASH_KEY_LEN;

In the mlx5_hrxq_new above you trust the rss_key_len to be the correct one, and it is ok since you validate it on the rss validate function.
Why it is not the case here as well? 

>  		if (hrxq->rss_key_len != rss_key_len)
>  			continue;
> +		if (!rss_key)
> +			rss_key = rss_hash_default_key;

Same question. You already copied to correct key in flow_verbs_translate_action_rss or on the flow_dv_create_action funciton. 

>  		if (memcmp(hrxq->rss_key, rss_key, rss_key_len))
>  			continue;
>  		if (hrxq->hash_fields != hash_fields) diff --git
> a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h index
> c0fe879..6cc72ab 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1785,7 +1785,7 @@ struct rte_flow_action_rss {
>  	uint64_t types; /**< Specific RSS hash types (see ETH_RSS_*). */
>  	uint32_t key_len; /**< Hash key length in bytes. */
>  	uint32_t queue_num; /**< Number of entries in @p queue. */
> -	const uint8_t *key; /**< Hash key. */
> +	const uint8_t *key; /**< Hash key. NULL should indicate default key

This change should be on a separate commit in a separate series. 
It is better to put the doc update above the function and not inline on the field so that you can elaborate more. 
Also you are missing the same doc update for the types field. 

Note: your patch for mlx5 can be accepted also w/o it, but it is better to align all vendors and I think it is aligned w/ the spirit of the rte_flow maintainer. 

> */
>  	const uint16_t *queue; /**< Queue indices to use. */  };
> 
> --
> 1.8.3.1

[dpdk-dev] FW: [PATCH v2] net/mlx5: set RSS key to NULL to indicate default RSS

[Ophir Munk]

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Thursday, November 01, 2018 4:01 PM
> To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org; Yongseok Koh 
> <yskoh@mellanox.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern 
> <olgas@mellanox.com>; Asaf Penso <asafp@mellanox.com>
> Subject: RE: [PATCH v2] net/mlx5: set RSS key to NULL to indicate 
> default RSS
> 
> Hi Ophir,
> 
> Thursday, November 1, 2018 10:59 AM, Ophir Munk:
> > Subject: [PATCH v2] net/mlx5: set RSS key to NULL to indicate 
> > default RSS
> >
> > Applications which add RSS rules must supply an RSS key and length.
> > If an application is only interested in default RSS operation it 
> > should not care about the exact RSS key.
> > By setting the key to NULL - the PMD will use the default RSS key.
> > In addition if the application does not care about the RSS type it 
> > can set it to 0 and the PMD will use the default type (ETH_RSS_IP).
> >
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > ---
> >  doc/guides/nics/mlx5.rst           |  1 +
> >  drivers/net/mlx5/mlx5_flow.c       |  8 +++++++-
> >  drivers/net/mlx5/mlx5_flow_dv.c    |  8 ++++++--
> >  drivers/net/mlx5/mlx5_flow_verbs.c |  9 +++++++--
> >  drivers/net/mlx5/mlx5_rxq.c        | 14 ++++++--------
> >  lib/librte_ethdev/rte_flow.h       |  2 +-
> >  6 files changed, 28 insertions(+), 14 deletions(-)
> >
> > diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst 
> > index 6769628..de8f15b 100644
> > --- a/doc/guides/nics/mlx5.rst
> > +++ b/doc/guides/nics/mlx5.rst
> > @@ -54,6 +54,7 @@ Features
> >  - Support for scattered TX and RX frames.
> >  - IPv4, IPv6, TCPv4, TCPv6, UDPv4 and UDPv6 RSS on any number of
> queues.
> >  - Several RSS hash keys, one for each flow type.
> > +- NULL RSS key indicates default RSS key
> 
> I don't think it is a feature, it should be an API implementation.

1. There is no "API implementation" section in the mlx5.rst doc. Please indicate where exactly you think it should be added.
2. I think you can consider this a feature: applications can use default RSS without knowing any specific RSS parameters.
Setting key to NULL is just the mean to get it (maybe should not be mentioned).

How about:
Default RSS operation with no hash key specification

> 
> >  - Configurable RETA table.
> >  - Support for multiple MAC addresses.
> >  - VLAN filtering.
> > diff --git a/drivers/net/mlx5/mlx5_flow.c 
> > b/drivers/net/mlx5/mlx5_flow.c index 280af0a..5742989 100644
> > --- a/drivers/net/mlx5/mlx5_flow.c
> > +++ b/drivers/net/mlx5/mlx5_flow.c
> > @@ -912,7 +912,13 @@ uint32_t mlx5_flow_adjust_priority(struct 
> > rte_eth_dev *dev, int32_t priority,
> >
> > RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> >  					  &rss->level,
> >  					  "tunnel RSS is not supported");
> > -	if (rss->key_len < MLX5_RSS_HASH_KEY_LEN)
> > +	/* allow RSS key_len 0 in case of NULL (default) RSS key */
> 
> Missing period.

Will be added in v3

> 
> > +	if (rss->key_len == 0 && rss->key != NULL)
> > +		return rte_flow_error_set(error, ENOTSUP,
> > +
> > RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> > +					  &rss->key_len,
> > +					  "RSS hash key length 0");
> > +	if (rss->key_len > 0 && rss->key_len < MLX5_RSS_HASH_KEY_LEN)
> 
> Can't you simplify using a single condition:
> If (rss->key && rss->key_len < MLX5_RSS_HASH_KEY_LEN)
> 

Do you mean that if key==NULL we do not care about the key_len?
(since if key==NULL key_len can have any value? 
Wil update in V3

> Same rss->key addition should be for the case the key_len is too large.
> 
> >  		return rte_flow_error_set(error, ENOTSUP,
> >
> > RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> >  					  &rss->key_len,
> > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c 
> > b/drivers/net/mlx5/mlx5_flow_dv.c index 8f729f4..fd52bf4 100644
> > --- a/drivers/net/mlx5/mlx5_flow_dv.c
> > +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> > @@ -1056,6 +1056,7 @@
> >  {
> >  	const struct rte_flow_action_queue *queue;
> >  	const struct rte_flow_action_rss *rss;
> > +	uint8_t	*rss_key;
> 
> Why no declare it inside the RSS case?

It is consistent with the coding style in this file.
1. There is no variable declaration inside any case.
2. If rss_key is declared inside the RSS case - the same should be for rss variable (for consistency) and there can be more cases to check.

> 
> >  	int actions_n = dev_flow->dv.actions_n;
> >  	struct rte_flow *flow = dev_flow->flow;
> >
> > @@ -1094,8 +1095,11 @@
> >  			memcpy((*flow->queue), rss->queue,
> >  			       rss->queue_num * sizeof(uint16_t));
> >  		flow->rss.queue_num = rss->queue_num;
> > -		memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
> > -		flow->rss.types = rss->types;
> > +		/* NULL RSS key indicates default RSS key */
> 
> Missing period.

Will be added in v3

> 
> > +		rss_key = (!rss->key) ? rss_hash_default_key : rss->key;
> > +		memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);
> 
> For consistency you should also set the key_len on the flow structure 
> to MLX5_RSS_HASH_KEY_LEN.
> 

There is no key_len on the flow structure. 
I guess the key_len field is missing since it is redundant (key_len is always 40 bytes).

> > +		/* RSS type 0 indicates default RSS type ETH_RSS_IP */
> 
> Period.

Will be added in v3

> 
> > +		flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
> >  		flow->rss.level = rss->level;
> >  		/* Added to array only in apply since we need the QP */
> >  		flow->actions |= MLX5_FLOW_ACTION_RSS; diff --git 
> > a/drivers/net/mlx5/mlx5_flow_verbs.c
> > b/drivers/net/mlx5/mlx5_flow_verbs.c
> > index 81bc39f..ae9042f 100644
> > --- a/drivers/net/mlx5/mlx5_flow_verbs.c
> > +++ b/drivers/net/mlx5/mlx5_flow_verbs.c
> > @@ -925,14 +925,19 @@
> >  				struct mlx5_flow *dev_flow)
> >  {
> >  	const struct rte_flow_action_rss *rss = action->conf;
> > +	const uint8_t *rss_key;
> >  	struct rte_flow *flow = dev_flow->flow;
> >
> >  	if (flow->queue)
> >  		memcpy((*flow->queue), rss->queue,
> >  		       rss->queue_num * sizeof(uint16_t));
> >  	flow->rss.queue_num = rss->queue_num;
> > -	memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
> > -	flow->rss.types = rss->types;
> > +
> > +	/* NULL RSS key indicates default RSS key */
> 
> period

Will be added in v3

> 
> > +	rss_key = !rss->key ? rss_hash_default_key : rss->key;
> 
> For consistency you should also set the key_len on the flow structure 
> to MLX5_RSS_HASH_KEY_LEN.
> 

There is no key_len on the flow structure.

> > +	memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);
> > +	/* RSS type 0 indicates default RSS type (ETH_RSS_IP) */
> 
> period

Will be added in v3

> 
> > +	flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
> >  	flow->rss.level = rss->level;
> >  	*action_flags |= MLX5_FLOW_ACTION_RSS;  } diff --git 
> > a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c index
> > ed993ea..36fc202 100644
> > --- a/drivers/net/mlx5/mlx5_rxq.c
> > +++ b/drivers/net/mlx5/mlx5_rxq.c
> > @@ -1783,10 +1783,6 @@ struct mlx5_hrxq *
> >  		rte_errno = ENOMEM;
> >  		return NULL;
> >  	}
> > -	if (!rss_key_len) {
> > -		rss_key_len = MLX5_RSS_HASH_KEY_LEN;
> > -		rss_key = rss_hash_default_key;
> > -	}
> >  #ifdef HAVE_IBV_DEVICE_TUNNEL_SUPPORT
> >  	qp = mlx5_glue->dv_create_qp
> >  		(priv->ctx,
> > @@ -1798,8 +1794,7 @@ struct mlx5_hrxq *
> >  				IBV_QP_INIT_ATTR_RX_HASH,
> >  			.rx_hash_conf = (struct ibv_rx_hash_conf){
> >  				.rx_hash_function =
> > IBV_RX_HASH_FUNC_TOEPLITZ,
> > -				.rx_hash_key_len = rss_key_len ?
> > rss_key_len :
> > -
> MLX5_RSS_HASH_KEY_LEN,
> > +				.rx_hash_key_len = rss_key_len,
> >  				.rx_hash_key = rss_key ?
> >  					       (void *)(uintptr_t)rss_key :
> >  					       rss_hash_default_key,
> > @@ -1824,8 +1819,7 @@ struct mlx5_hrxq *
> >  				IBV_QP_INIT_ATTR_RX_HASH,
> >  			.rx_hash_conf = (struct ibv_rx_hash_conf){
> >  				.rx_hash_function =
> > IBV_RX_HASH_FUNC_TOEPLITZ,
> > -				.rx_hash_key_len = rss_key_len ?
> > rss_key_len :
> > -
> MLX5_RSS_HASH_KEY_LEN,
> > +				.rx_hash_key_len = rss_key_len,
> >  				.rx_hash_key = rss_key ?
> >  					       (void *)(uintptr_t)rss_key :
> >  					       rss_hash_default_key,
> > @@ -1888,8 +1882,12 @@ struct mlx5_hrxq *
> >  	LIST_FOREACH(hrxq, &priv->hrxqs, next) {
> >  		struct mlx5_ind_table_ibv *ind_tbl;
> >
> > +		if (!rss_key_len)
> > +			rss_key_len = MLX5_RSS_HASH_KEY_LEN;
> 
> In the mlx5_hrxq_new above you trust the rss_key_len to be the correct 
> one, and it is ok since you validate it on the rss validate function.
> Why it is not the case here as well?
> 

Will be updated in v3.
Remark: rss_key_len assignment can be dropped  because calling mlx5_hrxq_get is done with constant MLX5_RSS_HASH_KEY_LEN already assigned to rss_key_len 

> >  		if (hrxq->rss_key_len != rss_key_len)
> >  			continue;
> > +		if (!rss_key)
> > +			rss_key = rss_hash_default_key;
> 
> Same question. You already copied to correct key in 
> flow_verbs_translate_action_rss or on the flow_dv_create_action funciton.

Will be dropped in v3
Will also drop in v3 another legacy code which does redundant rss_key==NULL check

> 
> >  		if (memcmp(hrxq->rss_key, rss_key, rss_key_len))
> >  			continue;
> >  		if (hrxq->hash_fields != hash_fields) diff --git 
> > a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h index 
> > c0fe879..6cc72ab 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -1785,7 +1785,7 @@ struct rte_flow_action_rss {
> >  	uint64_t types; /**< Specific RSS hash types (see ETH_RSS_*). */
> >  	uint32_t key_len; /**< Hash key length in bytes. */
> >  	uint32_t queue_num; /**< Number of entries in @p queue. */
> > -	const uint8_t *key; /**< Hash key. */
> > +	const uint8_t *key; /**< Hash key. NULL should indicate default 
> > +key
> 
> This change should be on a separate commit in a separate series.
> It is better to put the doc update above the function and not inline 
> on the field so that you can elaborate more.
> Also you are missing the same doc update for the types field.
> 

Will issue a sepearte commit in a separate series

> Note: your patch for mlx5 can be accepted also w/o it, but it is 
> better to align all vendors and I think it is aligned w/ the spirit of 
> the rte_flow maintainer.
> 
> > */
> >  	const uint16_t *queue; /**< Queue indices to use. */  };
> >
> > --
> > 1.8.3.1

[dpdk-dev] [PATCH v3] net/mlx5: set RSS key to NULL to indicate default RSS

[Ophir Munk]

Applications which add RSS rules must supply an RSS key and length.
If an application is only interested in default RSS operation it
should not care about the exact RSS key.
By setting the key to NULL - the PMD will use the default RSS key.
In addition if the application does not care about the RSS type it can
set it to 0 and the PMD will use the default type (ETH_RSS_IP).

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

v2, v3:
Rebase + following code review

 doc/guides/nics/mlx5.rst           |  1 +
 drivers/net/mlx5/mlx5_flow.c       |  4 ++--
 drivers/net/mlx5/mlx5_flow_dv.c    |  7 +++++--
 drivers/net/mlx5/mlx5_flow_verbs.c |  8 ++++++--
 drivers/net/mlx5/mlx5_rxq.c        | 18 ++++--------------
 5 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
index 1dc3282..0303152 100644
--- a/doc/guides/nics/mlx5.rst
+++ b/doc/guides/nics/mlx5.rst
@@ -54,6 +54,7 @@ Features
 - Support for scattered TX and RX frames.
 - IPv4, IPv6, TCPv4, TCPv6, UDPv4 and UDPv6 RSS on any number of queues.
 - Several RSS hash keys, one for each flow type.
+- Default RSS operation with no hash key specification.
 - Configurable RETA table.
 - Support for multiple MAC addresses.
 - VLAN filtering.
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 107a4f0..677d2c4 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -912,12 +912,12 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
 					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
 					  &rss->level,
 					  "tunnel RSS is not supported");
-	if (rss->key_len < MLX5_RSS_HASH_KEY_LEN)
+	if (rss->key && rss->key_len < MLX5_RSS_HASH_KEY_LEN)
 		return rte_flow_error_set(error, ENOTSUP,
 					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
 					  &rss->key_len,
 					  "RSS hash key too small");
-	if (rss->key_len > MLX5_RSS_HASH_KEY_LEN)
+	if (rss->key && rss->key_len > MLX5_RSS_HASH_KEY_LEN)
 		return rte_flow_error_set(error, ENOTSUP,
 					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
 					  &rss->key_len,
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index c11ecd4..cdf3377 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -1758,8 +1758,11 @@
 			memcpy((*flow->queue), rss->queue,
 			       rss->queue_num * sizeof(uint16_t));
 		flow->rss.queue_num = rss->queue_num;
-		memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
-		flow->rss.types = rss->types;
+		/* NULL RSS key indicates default RSS key. */
+		rss_key = !rss->key ? rss_hash_default_key : rss->key;
+		memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);
+		/* RSS type 0 indicates default RSS type ETH_RSS_IP. */
+		flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
 		flow->rss.level = rss->level;
 		/* Added to array only in apply since we need the QP */
 		flow->actions |= MLX5_FLOW_ACTION_RSS;
diff --git a/drivers/net/mlx5/mlx5_flow_verbs.c b/drivers/net/mlx5/mlx5_flow_verbs.c
index 2e506b9..54ac620 100644
--- a/drivers/net/mlx5/mlx5_flow_verbs.c
+++ b/drivers/net/mlx5/mlx5_flow_verbs.c
@@ -925,14 +925,18 @@
 				struct mlx5_flow *dev_flow)
 {
 	const struct rte_flow_action_rss *rss = action->conf;
+	const uint8_t *rss_key;
 	struct rte_flow *flow = dev_flow->flow;
 
 	if (flow->queue)
 		memcpy((*flow->queue), rss->queue,
 		       rss->queue_num * sizeof(uint16_t));
 	flow->rss.queue_num = rss->queue_num;
-	memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
-	flow->rss.types = rss->types;
+	/* NULL RSS key indicates default RSS key. */
+	rss_key = !rss->key ? rss_hash_default_key : rss->key;
+	memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);
+	/* RSS type 0 indicates default RSS type (ETH_RSS_IP). */
+	flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
 	flow->rss.level = rss->level;
 	*action_flags |= MLX5_FLOW_ACTION_RSS;
 }
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 6df8997..eef4850 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -1794,10 +1794,6 @@ struct mlx5_hrxq *
 		rte_errno = ENOMEM;
 		return NULL;
 	}
-	if (!rss_key_len) {
-		rss_key_len = MLX5_RSS_HASH_KEY_LEN;
-		rss_key = rss_hash_default_key;
-	}
 #ifdef HAVE_IBV_DEVICE_TUNNEL_SUPPORT
 	if (tunnel) {
 		qp_init_attr.comp_mask =
@@ -1823,11 +1819,8 @@ struct mlx5_hrxq *
 				IBV_QP_INIT_ATTR_RX_HASH,
 			.rx_hash_conf = (struct ibv_rx_hash_conf){
 				.rx_hash_function = IBV_RX_HASH_FUNC_TOEPLITZ,
-				.rx_hash_key_len = rss_key_len ? rss_key_len :
-						   MLX5_RSS_HASH_KEY_LEN,
-				.rx_hash_key = rss_key ?
-					       (void *)(uintptr_t)rss_key :
-					       rss_hash_default_key,
+				.rx_hash_key_len = rss_key_len,
+				.rx_hash_key = (void *)(uintptr_t)rss_key,
 				.rx_hash_fields_mask = hash_fields,
 			},
 			.rwq_ind_tbl = ind_tbl->ind_table,
@@ -1845,11 +1838,8 @@ struct mlx5_hrxq *
 				IBV_QP_INIT_ATTR_RX_HASH,
 			.rx_hash_conf = (struct ibv_rx_hash_conf){
 				.rx_hash_function = IBV_RX_HASH_FUNC_TOEPLITZ,
-				.rx_hash_key_len = rss_key_len ? rss_key_len :
-						   MLX5_RSS_HASH_KEY_LEN,
-				.rx_hash_key = rss_key ?
-					       (void *)(uintptr_t)rss_key :
-					       rss_hash_default_key,
+				.rx_hash_key_len = rss_key_len,
+				.rx_hash_key = (void *)(uintptr_t)rss_key,
 				.rx_hash_fields_mask = hash_fields,
 			},
 			.rwq_ind_tbl = ind_tbl->ind_table,
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] net/mlx5: set RSS key to NULL to indicate default RSS

[Ophir Munk]

Please see comments inline

> -----Original Message-----
> From: Ophir Munk
> Sent: Friday, November 02, 2018 7:55 PM
> To: dev@dpdk.org; Yongseok Koh <yskoh@mellanox.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Asaf Penso
> <asafp@mellanox.com>; Olga Shern <olgas@mellanox.com>; Ophir Munk
> <ophirmu@mellanox.com>
> Subject: FW: [PATCH v2] net/mlx5: set RSS key to NULL to indicate default
> RSS
> 
> 
> > -----Original Message-----
> > From: Shahaf Shuler
> > Sent: Thursday, November 01, 2018 4:01 PM
> > To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org; Yongseok Koh
> > <yskoh@mellanox.com>
> > Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> > <olgas@mellanox.com>; Asaf Penso <asafp@mellanox.com>
> > Subject: RE: [PATCH v2] net/mlx5: set RSS key to NULL to indicate
> > default RSS
> >
> > Hi Ophir,
> >
> 
> >
> > > +	if (rss->key_len == 0 && rss->key != NULL)
> > > +		return rte_flow_error_set(error, ENOTSUP,
> > > +
> > > RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> > > +					  &rss->key_len,
> > > +					  "RSS hash key length 0");
> > > +	if (rss->key_len > 0 && rss->key_len < MLX5_RSS_HASH_KEY_LEN)
> >
> > Can't you simplify using a single condition:
> > If (rss->key && rss->key_len < MLX5_RSS_HASH_KEY_LEN)
> >
> 
> Do you mean that if key==NULL we do not care about the key_len?
> (since if key==NULL key_len can have any value?
> Wil update in V3

After applying in v3 the reviewed condition code - action validation confirms a NULL key with key length 6.
But then testpmd crashes when creating the rule. Crash occurs in rte_flow_conv_action_conf().
When reverting to v2 condition code action validation is failing (as expected) with error message:
Caught error type 15 (action configuration): cause: 0x7fffffffbf18, RSS hash key too small: Unknown error 95
and flow creation fails with the same error.

I will issue v4 with v2  condition code.

> 
> > Same rss->key addition should be for the case the key_len is too large.

[dpdk-dev] [PATCH v4] net/mlx5: set RSS key to NULL to indicate default RSS

[Ophir Munk]

Applications which add RSS rules must supply an RSS key and length.
If an application is only interested in default RSS operation it
should not care about the exact RSS key.
By setting the key to NULL - the PMD will use the default RSS key.
In addition if the application does not care about the RSS type it can
set it to 0 and the PMD will use the default type (ETH_RSS_IP).

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

v2, v3:
Rebase + following code review

v4:
Avoid sgementation faulut by not allowing in cation validation key=NULL ane key_len!=0
See https://patches.dpdk.org/patch/47645/


 doc/guides/nics/mlx5.rst           |  1 +
 drivers/net/mlx5/mlx5_flow.c       |  8 +++++++-
 drivers/net/mlx5/mlx5_flow_dv.c    |  7 +++++--
 drivers/net/mlx5/mlx5_flow_verbs.c |  8 ++++++--
 drivers/net/mlx5/mlx5_rxq.c        | 18 ++++--------------
 5 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
index 1dc3282..0303152 100644
--- a/doc/guides/nics/mlx5.rst
+++ b/doc/guides/nics/mlx5.rst
@@ -54,6 +54,7 @@ Features
 - Support for scattered TX and RX frames.
 - IPv4, IPv6, TCPv4, TCPv6, UDPv4 and UDPv6 RSS on any number of queues.
 - Several RSS hash keys, one for each flow type.
+- Default RSS operation with no hash key specification.
 - Configurable RETA table.
 - Support for multiple MAC addresses.
 - VLAN filtering.
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 107a4f0..be2cc6b 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -912,7 +912,13 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
 					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
 					  &rss->level,
 					  "tunnel RSS is not supported");
-	if (rss->key_len < MLX5_RSS_HASH_KEY_LEN)
+	/* allow RSS key_len 0 in case of NULL (default) RSS key. */
+	if (rss->key_len == 0 && rss->key != NULL)
+		return rte_flow_error_set(error, ENOTSUP,
+					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
+					  &rss->key_len,
+					  "RSS hash key length 0");
+	if (rss->key_len > 0 && rss->key_len < MLX5_RSS_HASH_KEY_LEN)
 		return rte_flow_error_set(error, ENOTSUP,
 					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
 					  &rss->key_len,
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index c11ecd4..cdf3377 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -1758,8 +1758,11 @@
 			memcpy((*flow->queue), rss->queue,
 			       rss->queue_num * sizeof(uint16_t));
 		flow->rss.queue_num = rss->queue_num;
-		memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
-		flow->rss.types = rss->types;
+		/* NULL RSS key indicates default RSS key. */
+		rss_key = !rss->key ? rss_hash_default_key : rss->key;
+		memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);
+		/* RSS type 0 indicates default RSS type ETH_RSS_IP. */
+		flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
 		flow->rss.level = rss->level;
 		/* Added to array only in apply since we need the QP */
 		flow->actions |= MLX5_FLOW_ACTION_RSS;
diff --git a/drivers/net/mlx5/mlx5_flow_verbs.c b/drivers/net/mlx5/mlx5_flow_verbs.c
index 2e506b9..54ac620 100644
--- a/drivers/net/mlx5/mlx5_flow_verbs.c
+++ b/drivers/net/mlx5/mlx5_flow_verbs.c
@@ -925,14 +925,18 @@
 				struct mlx5_flow *dev_flow)
 {
 	const struct rte_flow_action_rss *rss = action->conf;
+	const uint8_t *rss_key;
 	struct rte_flow *flow = dev_flow->flow;
 
 	if (flow->queue)
 		memcpy((*flow->queue), rss->queue,
 		       rss->queue_num * sizeof(uint16_t));
 	flow->rss.queue_num = rss->queue_num;
-	memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
-	flow->rss.types = rss->types;
+	/* NULL RSS key indicates default RSS key. */
+	rss_key = !rss->key ? rss_hash_default_key : rss->key;
+	memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);
+	/* RSS type 0 indicates default RSS type (ETH_RSS_IP). */
+	flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
 	flow->rss.level = rss->level;
 	*action_flags |= MLX5_FLOW_ACTION_RSS;
 }
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 6df8997..eef4850 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -1794,10 +1794,6 @@ struct mlx5_hrxq *
 		rte_errno = ENOMEM;
 		return NULL;
 	}
-	if (!rss_key_len) {
-		rss_key_len = MLX5_RSS_HASH_KEY_LEN;
-		rss_key = rss_hash_default_key;
-	}
 #ifdef HAVE_IBV_DEVICE_TUNNEL_SUPPORT
 	if (tunnel) {
 		qp_init_attr.comp_mask =
@@ -1823,11 +1819,8 @@ struct mlx5_hrxq *
 				IBV_QP_INIT_ATTR_RX_HASH,
 			.rx_hash_conf = (struct ibv_rx_hash_conf){
 				.rx_hash_function = IBV_RX_HASH_FUNC_TOEPLITZ,
-				.rx_hash_key_len = rss_key_len ? rss_key_len :
-						   MLX5_RSS_HASH_KEY_LEN,
-				.rx_hash_key = rss_key ?
-					       (void *)(uintptr_t)rss_key :
-					       rss_hash_default_key,
+				.rx_hash_key_len = rss_key_len,
+				.rx_hash_key = (void *)(uintptr_t)rss_key,
 				.rx_hash_fields_mask = hash_fields,
 			},
 			.rwq_ind_tbl = ind_tbl->ind_table,
@@ -1845,11 +1838,8 @@ struct mlx5_hrxq *
 				IBV_QP_INIT_ATTR_RX_HASH,
 			.rx_hash_conf = (struct ibv_rx_hash_conf){
 				.rx_hash_function = IBV_RX_HASH_FUNC_TOEPLITZ,
-				.rx_hash_key_len = rss_key_len ? rss_key_len :
-						   MLX5_RSS_HASH_KEY_LEN,
-				.rx_hash_key = rss_key ?
-					       (void *)(uintptr_t)rss_key :
-					       rss_hash_default_key,
+				.rx_hash_key_len = rss_key_len,
+				.rx_hash_key = (void *)(uintptr_t)rss_key,
 				.rx_hash_fields_mask = hash_fields,
 			},
 			.rwq_ind_tbl = ind_tbl->ind_table,
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v4] net/mlx5: set RSS key to NULL to indicate default RSS

[Shahaf Shuler]

Hi Ophir, 

Saturday, November 3, 2018 7:40 PM, Ophir Munk
> Subject: [PATCH v4] net/mlx5: set RSS key to NULL to indicate default RSS
> 
> Applications which add RSS rules must supply an RSS key and length.
> If an application is only interested in default RSS operation it should not care
> about the exact RSS key.
> By setting the key to NULL - the PMD will use the default RSS key.
> In addition if the application does not care about the RSS type it can set it to 0
> and the PMD will use the default type (ETH_RSS_IP).
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
> v1:
> Initial release
> 
> v2, v3:
> Rebase + following code review
> 
> v4:
> Avoid sgementation faulut by not allowing in cation validation key=NULL ane
> key_len!=0 See https://patches.dpdk.org/patch/47645/
> 
> 
>  doc/guides/nics/mlx5.rst           |  1 +
>  drivers/net/mlx5/mlx5_flow.c       |  8 +++++++-
>  drivers/net/mlx5/mlx5_flow_dv.c    |  7 +++++--
>  drivers/net/mlx5/mlx5_flow_verbs.c |  8 ++++++--
>  drivers/net/mlx5/mlx5_rxq.c        | 18 ++++--------------
>  5 files changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst index
> 1dc3282..0303152 100644
> --- a/doc/guides/nics/mlx5.rst
> +++ b/doc/guides/nics/mlx5.rst
> @@ -54,6 +54,7 @@ Features
>  - Support for scattered TX and RX frames.
>  - IPv4, IPv6, TCPv4, TCPv6, UDPv4 and UDPv6 RSS on any number of queues.
>  - Several RSS hash keys, one for each flow type.
> +- Default RSS operation with no hash key specification.
>  - Configurable RETA table.
>  - Support for multiple MAC addresses.
>  - VLAN filtering.
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 107a4f0..be2cc6b 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -912,7 +912,13 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
> 
> RTE_FLOW_ERROR_TYPE_ACTION_CONF,
>  					  &rss->level,
>  					  "tunnel RSS is not supported");
> -	if (rss->key_len < MLX5_RSS_HASH_KEY_LEN)
> +	/* allow RSS key_len 0 in case of NULL (default) RSS key. */
> +	if (rss->key_len == 0 && rss->key != NULL)
> +		return rte_flow_error_set(error, ENOTSUP,
> +
> RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> +					  &rss->key_len,
> +					  "RSS hash key length 0");
> +	if (rss->key_len > 0 && rss->key_len < MLX5_RSS_HASH_KEY_LEN)
>  		return rte_flow_error_set(error, ENOTSUP,
> 
> RTE_FLOW_ERROR_TYPE_ACTION_CONF,
>  					  &rss->key_len,
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c index c11ecd4..cdf3377 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -1758,8 +1758,11 @@
>  			memcpy((*flow->queue), rss->queue,
>  			       rss->queue_num * sizeof(uint16_t));
>  		flow->rss.queue_num = rss->queue_num;
> -		memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
> -		flow->rss.types = rss->types;
> +		/* NULL RSS key indicates default RSS key. */
> +		rss_key = !rss->key ? rss_hash_default_key : rss->key;

Getting compilation error: 
error: 'rss_key' undeclared (first use in this function)                                                                                                            
   rss_key = !rss->key ? rss_hash_default_key : rss->key;   

please address. 
Other than that, no more comments. You can put my ack when on the next version. 


> +		memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);
> +		/* RSS type 0 indicates default RSS type ETH_RSS_IP. */
> +		flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
>  		flow->rss.level = rss->level;
>  		/* Added to array only in apply since we need the QP */
>  		flow->actions |= MLX5_FLOW_ACTION_RSS; diff --git
> a/drivers/net/mlx5/mlx5_flow_verbs.c
> b/drivers/net/mlx5/mlx5_flow_verbs.c
> index 2e506b9..54ac620 100644
> --- a/drivers/net/mlx5/mlx5_flow_verbs.c
> +++ b/drivers/net/mlx5/mlx5_flow_verbs.c
> @@ -925,14 +925,18 @@
>  				struct mlx5_flow *dev_flow)
>  {
>  	const struct rte_flow_action_rss *rss = action->conf;
> +	const uint8_t *rss_key;
>  	struct rte_flow *flow = dev_flow->flow;
> 
>  	if (flow->queue)
>  		memcpy((*flow->queue), rss->queue,
>  		       rss->queue_num * sizeof(uint16_t));
>  	flow->rss.queue_num = rss->queue_num;
> -	memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
> -	flow->rss.types = rss->types;
> +	/* NULL RSS key indicates default RSS key. */
> +	rss_key = !rss->key ? rss_hash_default_key : rss->key;
> +	memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);
> +	/* RSS type 0 indicates default RSS type (ETH_RSS_IP). */
> +	flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
>  	flow->rss.level = rss->level;
>  	*action_flags |= MLX5_FLOW_ACTION_RSS;  } diff --git
> a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c index
> 6df8997..eef4850 100644
> --- a/drivers/net/mlx5/mlx5_rxq.c
> +++ b/drivers/net/mlx5/mlx5_rxq.c
> @@ -1794,10 +1794,6 @@ struct mlx5_hrxq *
>  		rte_errno = ENOMEM;
>  		return NULL;
>  	}
> -	if (!rss_key_len) {
> -		rss_key_len = MLX5_RSS_HASH_KEY_LEN;
> -		rss_key = rss_hash_default_key;
> -	}
>  #ifdef HAVE_IBV_DEVICE_TUNNEL_SUPPORT
>  	if (tunnel) {
>  		qp_init_attr.comp_mask =
> @@ -1823,11 +1819,8 @@ struct mlx5_hrxq *
>  				IBV_QP_INIT_ATTR_RX_HASH,
>  			.rx_hash_conf = (struct ibv_rx_hash_conf){
>  				.rx_hash_function =
> IBV_RX_HASH_FUNC_TOEPLITZ,
> -				.rx_hash_key_len = rss_key_len ?
> rss_key_len :
> -						   MLX5_RSS_HASH_KEY_LEN,
> -				.rx_hash_key = rss_key ?
> -					       (void *)(uintptr_t)rss_key :
> -					       rss_hash_default_key,
> +				.rx_hash_key_len = rss_key_len,
> +				.rx_hash_key = (void *)(uintptr_t)rss_key,
>  				.rx_hash_fields_mask = hash_fields,
>  			},
>  			.rwq_ind_tbl = ind_tbl->ind_table,
> @@ -1845,11 +1838,8 @@ struct mlx5_hrxq *
>  				IBV_QP_INIT_ATTR_RX_HASH,
>  			.rx_hash_conf = (struct ibv_rx_hash_conf){
>  				.rx_hash_function =
> IBV_RX_HASH_FUNC_TOEPLITZ,
> -				.rx_hash_key_len = rss_key_len ?
> rss_key_len :
> -						   MLX5_RSS_HASH_KEY_LEN,
> -				.rx_hash_key = rss_key ?
> -					       (void *)(uintptr_t)rss_key :
> -					       rss_hash_default_key,
> +				.rx_hash_key_len = rss_key_len,
> +				.rx_hash_key = (void *)(uintptr_t)rss_key,
>  				.rx_hash_fields_mask = hash_fields,
>  			},
>  			.rwq_ind_tbl = ind_tbl->ind_table,
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH v4] net/mlx5: set RSS key to NULL to indicate default RSS

[Ophir Munk]

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Sunday, November 04, 2018 8:29 AM
> To: Ophir Munk <ophirmu@mellanox.com>; Yongseok Koh
> <yskoh@mellanox.com>; dev@dpdk.org
> Cc: Asaf Penso <asafp@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; Olga Shern <olgas@mellanox.com>
> Subject: RE: [PATCH v4] net/mlx5: set RSS key to NULL to indicate default
> RSS
> 
> Hi Ophir,
> 
> Saturday, November 3, 2018 7:40 PM, Ophir Munk
> > Subject: [PATCH v4] net/mlx5: set RSS key to NULL to indicate default
> > RSS
> >
> > Applications which add RSS rules must supply an RSS key and length.
> > If an application is only interested in default RSS operation it
> > should not care about the exact RSS key.
> > By setting the key to NULL - the PMD will use the default RSS key.
> > In addition if the application does not care about the RSS type it can
> > set it to 0 and the PMD will use the default type (ETH_RSS_IP).
> >
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > ---
> > v1:
> > Initial release
> >
> > v2, v3:
> > Rebase + following code review
> >
> > v4:
> > Avoid sgementation faulut by not allowing in cation validation
> > key=NULL ane
> > key_len!=0 See https://patches.dpdk.org/patch/47645/
> >
> >
> >  doc/guides/nics/mlx5.rst           |  1 +
> >  drivers/net/mlx5/mlx5_flow.c       |  8 +++++++-
> >  drivers/net/mlx5/mlx5_flow_dv.c    |  7 +++++--
> >  drivers/net/mlx5/mlx5_flow_verbs.c |  8 ++++++--
> >  drivers/net/mlx5/mlx5_rxq.c        | 18 ++++--------------
> >  5 files changed, 23 insertions(+), 19 deletions(-)
> >
> > diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst index
> > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> > b/drivers/net/mlx5/mlx5_flow_dv.c index c11ecd4..cdf3377 100644
> > --- a/drivers/net/mlx5/mlx5_flow_dv.c
> > +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> > @@ -1758,8 +1758,11 @@
> >  			memcpy((*flow->queue), rss->queue,
> >  			       rss->queue_num * sizeof(uint16_t));
> >  		flow->rss.queue_num = rss->queue_num;
> > -		memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
> > -		flow->rss.types = rss->types;
> > +		/* NULL RSS key indicates default RSS key. */
> > +		rss_key = !rss->key ? rss_hash_default_key : rss->key;
> 
> Getting compilation error:
> error: 'rss_key' undeclared (first use in this function)
>    rss_key = !rss->key ? rss_hash_default_key : rss->key;
> 
> please address.

Most of the code in file mlx5_flow_dv.c is under #ifdef HAVE_IBV_FLOW_DV_SUPPORT
In my case this macro is not defined so most of the code is not compiled...
Please note mlx5 Makefile:

$Q sh -- '$<' '$@' \
      HAVE_IBV_FLOW_DV_SUPPORT \
      infiniband/mlx5dv.h \
      func mlx5dv_create_flow_action_packet_reformat \
      $(AUTOCONF_OUTPUT)
 
I will issue v5 with a fix

> Other than that, no more comments. You can put my ack when on the next
> version.
> 
> 
> > +		memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);

[dpdk-dev] [PATCH v5] net/mlx5: set RSS key to NULL to indicate default RSS

[Ophir Munk]

Applications which add RSS rules must supply an RSS key and length.
If an application is only interested in default RSS operation it
should not care about the exact RSS key.
By setting the key to NULL - the PMD will use the default RSS key.
In addition if the application does not care about the RSS type it can
set it to 0 and the PMD will use the default type (ETH_RSS_IP).

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

v2, v3:
Rebase + following code review

v4:
Avoid sgementation faulut by not allowing in cation validation key=NULL ane key_len!=0
See https://patches.dpdk.org/patch/47645/

v5:
Add missing variable const uint8_t *rss_key in function flow_dv_create_action()
(code is under #ifdef HAVE_IBV_FLOW_DV_SUPPORT)
Adding Acked-by: Shahaf Shuler <shahafs@mellanox.com>


 doc/guides/nics/mlx5.rst           |  1 +
 drivers/net/mlx5/mlx5_flow.c       |  8 +++++++-
 drivers/net/mlx5/mlx5_flow_dv.c    |  8 ++++++--
 drivers/net/mlx5/mlx5_flow_verbs.c |  8 ++++++--
 drivers/net/mlx5/mlx5_rxq.c        | 18 ++++--------------
 5 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
index 1dc3282..0303152 100644
--- a/doc/guides/nics/mlx5.rst
+++ b/doc/guides/nics/mlx5.rst
@@ -54,6 +54,7 @@ Features
 - Support for scattered TX and RX frames.
 - IPv4, IPv6, TCPv4, TCPv6, UDPv4 and UDPv6 RSS on any number of queues.
 - Several RSS hash keys, one for each flow type.
+- Default RSS operation with no hash key specification.
 - Configurable RETA table.
 - Support for multiple MAC addresses.
 - VLAN filtering.
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 107a4f0..be2cc6b 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -912,7 +912,13 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
 					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
 					  &rss->level,
 					  "tunnel RSS is not supported");
-	if (rss->key_len < MLX5_RSS_HASH_KEY_LEN)
+	/* allow RSS key_len 0 in case of NULL (default) RSS key. */
+	if (rss->key_len == 0 && rss->key != NULL)
+		return rte_flow_error_set(error, ENOTSUP,
+					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
+					  &rss->key_len,
+					  "RSS hash key length 0");
+	if (rss->key_len > 0 && rss->key_len < MLX5_RSS_HASH_KEY_LEN)
 		return rte_flow_error_set(error, ENOTSUP,
 					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
 					  &rss->key_len,
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index c11ecd4..1d5b6bf 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -1722,6 +1722,7 @@
 	int actions_n = dev_flow->dv.actions_n;
 	struct rte_flow *flow = dev_flow->flow;
 	const struct rte_flow_action *action_ptr = action;
+	const uint8_t *rss_key;
 
 	switch (action->type) {
 	case RTE_FLOW_ACTION_TYPE_VOID:
@@ -1758,8 +1759,11 @@
 			memcpy((*flow->queue), rss->queue,
 			       rss->queue_num * sizeof(uint16_t));
 		flow->rss.queue_num = rss->queue_num;
-		memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
-		flow->rss.types = rss->types;
+		/* NULL RSS key indicates default RSS key. */
+		rss_key = !rss->key ? rss_hash_default_key : rss->key;
+		memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);
+		/* RSS type 0 indicates default RSS type ETH_RSS_IP. */
+		flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
 		flow->rss.level = rss->level;
 		/* Added to array only in apply since we need the QP */
 		flow->actions |= MLX5_FLOW_ACTION_RSS;
diff --git a/drivers/net/mlx5/mlx5_flow_verbs.c b/drivers/net/mlx5/mlx5_flow_verbs.c
index 2e506b9..54ac620 100644
--- a/drivers/net/mlx5/mlx5_flow_verbs.c
+++ b/drivers/net/mlx5/mlx5_flow_verbs.c
@@ -925,14 +925,18 @@
 				struct mlx5_flow *dev_flow)
 {
 	const struct rte_flow_action_rss *rss = action->conf;
+	const uint8_t *rss_key;
 	struct rte_flow *flow = dev_flow->flow;
 
 	if (flow->queue)
 		memcpy((*flow->queue), rss->queue,
 		       rss->queue_num * sizeof(uint16_t));
 	flow->rss.queue_num = rss->queue_num;
-	memcpy(flow->key, rss->key, MLX5_RSS_HASH_KEY_LEN);
-	flow->rss.types = rss->types;
+	/* NULL RSS key indicates default RSS key. */
+	rss_key = !rss->key ? rss_hash_default_key : rss->key;
+	memcpy(flow->key, rss_key, MLX5_RSS_HASH_KEY_LEN);
+	/* RSS type 0 indicates default RSS type (ETH_RSS_IP). */
+	flow->rss.types = !rss->types ? ETH_RSS_IP : rss->types;
 	flow->rss.level = rss->level;
 	*action_flags |= MLX5_FLOW_ACTION_RSS;
 }
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 6df8997..eef4850 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -1794,10 +1794,6 @@ struct mlx5_hrxq *
 		rte_errno = ENOMEM;
 		return NULL;
 	}
-	if (!rss_key_len) {
-		rss_key_len = MLX5_RSS_HASH_KEY_LEN;
-		rss_key = rss_hash_default_key;
-	}
 #ifdef HAVE_IBV_DEVICE_TUNNEL_SUPPORT
 	if (tunnel) {
 		qp_init_attr.comp_mask =
@@ -1823,11 +1819,8 @@ struct mlx5_hrxq *
 				IBV_QP_INIT_ATTR_RX_HASH,
 			.rx_hash_conf = (struct ibv_rx_hash_conf){
 				.rx_hash_function = IBV_RX_HASH_FUNC_TOEPLITZ,
-				.rx_hash_key_len = rss_key_len ? rss_key_len :
-						   MLX5_RSS_HASH_KEY_LEN,
-				.rx_hash_key = rss_key ?
-					       (void *)(uintptr_t)rss_key :
-					       rss_hash_default_key,
+				.rx_hash_key_len = rss_key_len,
+				.rx_hash_key = (void *)(uintptr_t)rss_key,
 				.rx_hash_fields_mask = hash_fields,
 			},
 			.rwq_ind_tbl = ind_tbl->ind_table,
@@ -1845,11 +1838,8 @@ struct mlx5_hrxq *
 				IBV_QP_INIT_ATTR_RX_HASH,
 			.rx_hash_conf = (struct ibv_rx_hash_conf){
 				.rx_hash_function = IBV_RX_HASH_FUNC_TOEPLITZ,
-				.rx_hash_key_len = rss_key_len ? rss_key_len :
-						   MLX5_RSS_HASH_KEY_LEN,
-				.rx_hash_key = rss_key ?
-					       (void *)(uintptr_t)rss_key :
-					       rss_hash_default_key,
+				.rx_hash_key_len = rss_key_len,
+				.rx_hash_key = (void *)(uintptr_t)rss_key,
 				.rx_hash_fields_mask = hash_fields,
 			},
 			.rwq_ind_tbl = ind_tbl->ind_table,
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v5] net/mlx5: set RSS key to NULL to indicate default RSS

[Shahaf Shuler]

Sunday, November 4, 2018 2:10 PM, Ophir Munk:
> Subject: [PATCH v5] net/mlx5: set RSS key to NULL to indicate default RSS
> 
> Applications which add RSS rules must supply an RSS key and length.
> If an application is only interested in default RSS operation it should not care
> about the exact RSS key.
> By setting the key to NULL - the PMD will use the default RSS key.
> In addition if the application does not care about the RSS type it can set it to 0
> and the PMD will use the default type (ETH_RSS_IP).
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>

Applied to next-net-mlx, thanks.