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

[Shahaf Shuler <shahafs@mellanox.com>]

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