> -----Original Message-----
> From: Shahaf Shuler
> Sent: Thursday, November 01, 2018 4:01 PM
> To: Ophir Munk <ophi...@mellanox.com>; dev@dpdk.org; Yongseok Koh
> <ys...@mellanox.com>
> Cc: Thomas Monjalon <tho...@monjalon.net>; Olga Shern
> <ol...@mellanox.com>; Asaf Penso <as...@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 <ophi...@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
