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

[Ophir Munk <ophirmu@mellanox.com>]

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