diff mbox series

[v4] net/mlx5: set RSS key to NULL to indicate default RSS

Message ID 1541266779-10519-1-git-send-email-ophirmu@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Shahaf Shuler
Headers
Series [v4] net/mlx5: set RSS key to NULL to indicate default RSS |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK

Commit Message

Ophir Munk Nov. 3, 2018, 5:39 p.m. UTC 
  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(-)

Comments

Shahaf Shuler Nov. 4, 2018, 6:28 a.m. UTC | #1 
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
Ophir Munk Nov. 4, 2018, 10:08 a.m. UTC | #2 
> -----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);
diff mbox series

Patch

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,