RE: [PATCH 3/3] net/mlx5: allow FDB RSS

[Suanming Mou <suanmingm@nvidia.com>]

> -----Original Message-----
> From: Bing Zhao <bingz@nvidia.com>
> Sent: Wednesday, February 26, 2025 6:50 PM
> To: Suanming Mou <suanmingm@nvidia.com>; Dariusz Sosnowski
> <dsosnowski@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Ori
> Kam <orika@nvidia.com>; Matan Azrad <matan@nvidia.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>
> Subject: RE: [PATCH 3/3] net/mlx5: allow FDB RSS
> 
> 
> 
> > -----Original Message-----
> > From: Suanming Mou <suanmingm@nvidia.com>
> > Sent: Tuesday, February 25, 2025 8:45 AM
> > To: Dariusz Sosnowski <dsosnowski@nvidia.com>; Slava Ovsiienko
> > <viacheslavo@nvidia.com>; Bing Zhao <bingz@nvidia.com>; Ori Kam
> > <orika@nvidia.com>; Matan Azrad <matan@nvidia.com>
> > Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>
> > Subject: [PATCH 3/3] net/mlx5: allow FDB RSS
> >
> > RSS can be used in FDB Rx rules when JUMP_FDB_RX action to allow jump
> > from FDB Tx to FDB Rx table.
> >
> > Different with NIC RSS, FDB RSS will not do the internal implicit
> > metadata copy.
> >
> > This commit enables the FDB RSS if JUMP_FDB_RX is supported.
> >
> > Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> > ---
> >  drivers/net/mlx5/mlx5_flow.c    |  4 ++--
> >  drivers/net/mlx5/mlx5_flow_dv.c | 32 +++++++++++++++++---------------
> > drivers/net/mlx5/mlx5_flow_hw.c | 19 ++++++++++++++-----
> >  3 files changed, 33 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > b/drivers/net/mlx5/mlx5_flow.c index 3fbe89a9d4..9c6a4f39fb 100644
> > --- a/drivers/net/mlx5/mlx5_flow.c
> > +++ b/drivers/net/mlx5/mlx5_flow.c
> > @@ -1977,9 +1977,9 @@ mlx5_flow_validate_action_mark(struct
> > rte_eth_dev *dev,
> >  					  RTE_FLOW_ERROR_TYPE_ATTR_EGRESS, NULL,
> >  					  "mark action not supported for "
> >  					  "egress");
> > -	if (attr->transfer && mlx5_hws_active(dev))
> > +	if (attr->transfer && !mlx5_hws_active(dev))
> >  		return rte_flow_error_set(error, ENOTSUP,
> > -					  RTE_FLOW_ERROR_TYPE_ATTR_EGRESS, NULL,
> > +					  RTE_FLOW_ERROR_TYPE_ATTR_TRANSFER, NULL,
> >  					  "non-template mark action not supported for
> transfer");
> >  	return 0;
> >  }
> > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> > b/drivers/net/mlx5/mlx5_flow_dv.c index 633c41e358..61d3101ce8
> 100644
> > --- a/drivers/net/mlx5/mlx5_flow_dv.c
> > +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> > @@ -8939,21 +8939,23 @@ flow_dv_validate(struct rte_eth_dev *dev,
> > const struct rte_flow_attr *attr,
> >  						  RTE_FLOW_ERROR_TYPE_ACTION,
> >  						  NULL,
> >  						  "unsupported action MARK");
> > -		if (action_flags & MLX5_FLOW_ACTION_QUEUE)
> > -			return rte_flow_error_set(error, ENOTSUP,
> > -						  RTE_FLOW_ERROR_TYPE_ACTION,
> > -						  NULL,
> > -						  "unsupported action QUEUE");
> > -		if (action_flags & MLX5_FLOW_ACTION_RSS)
> > -			return rte_flow_error_set(error, ENOTSUP,
> > -						  RTE_FLOW_ERROR_TYPE_ACTION,
> > -						  NULL,
> > -						  "unsupported action RSS");
> > -		if (!(action_flags & MLX5_FLOW_FATE_ESWITCH_ACTIONS))
> > -			return rte_flow_error_set(error, EINVAL,
> > -						  RTE_FLOW_ERROR_TYPE_ACTION,
> > -						  actions,
> > -						  "no fate action is found");
> > +		if (!priv->jump_fdb_rx_en) {
> > +			if (action_flags & MLX5_FLOW_ACTION_QUEUE)
> > +				return rte_flow_error_set(error, ENOTSUP,
> > +							  RTE_FLOW_ERROR_TYPE_ACTION,
> > +							  NULL,
> > +							  "unsupported action QUEUE");
> > +			if (action_flags & MLX5_FLOW_ACTION_RSS)
> > +				return rte_flow_error_set(error, ENOTSUP,
> > +							  RTE_FLOW_ERROR_TYPE_ACTION,
> > +							  NULL,
> > +							  "unsupported action RSS");
> > +			if (!(action_flags & MLX5_FLOW_FATE_ESWITCH_ACTIONS))
> > +				return rte_flow_error_set(error, EINVAL,
> > +							  RTE_FLOW_ERROR_TYPE_ACTION,
> > +							  actions,
> > +							  "no fate action is found");
> > +		}
> >  	} else {
> >  		if (!(action_flags & MLX5_FLOW_FATE_ACTIONS) && attr->ingress)
> >  			return rte_flow_error_set(error, EINVAL, diff --git
> > a/drivers/net/mlx5/mlx5_flow_hw.c b/drivers/net/mlx5/mlx5_flow_hw.c
> > index 83f55ed3e8..a063e5ac9c 100644
> > --- a/drivers/net/mlx5/mlx5_flow_hw.c
> > +++ b/drivers/net/mlx5/mlx5_flow_hw.c
> > @@ -7026,6 +7026,7 @@ mlx5_hw_validate_action_queue(struct
> rte_eth_dev
> > *dev,
> >  			      uint64_t action_flags,
> >  			      struct rte_flow_error *error)  {
> > +	struct mlx5_priv *priv = dev->data->dev_private;
> >  	const struct rte_flow_action_queue *queue_mask = template_mask-
> > >conf;
> >  	const struct rte_flow_attr attr = {
> >  		.ingress = template_attr->ingress,
> > @@ -7034,7 +7035,7 @@ mlx5_hw_validate_action_queue(struct
> rte_eth_dev
> > *dev,
> >  	};
> >  	bool masked = queue_mask != NULL && queue_mask->index;
> >
> > -	if (template_attr->egress || template_attr->transfer)
> > +	if (template_attr->egress || (template_attr->transfer &&
> > +!priv->jump_fdb_rx_en))
> >  		return rte_flow_error_set(error, EINVAL,
> >  					  RTE_FLOW_ERROR_TYPE_ATTR, NULL,
> >  					  "QUEUE action supported for ingress only"); @@
> -7053,9
> > +7054,10 @@ mlx5_hw_validate_action_rss(struct
> > rte_eth_dev *dev,
> >  			      __rte_unused uint64_t action_flags,
> >  			      struct rte_flow_error *error)  {
> > +	struct mlx5_priv *priv = dev->data->dev_private;
> >  	const struct rte_flow_action_rss *mask = template_mask->conf;
> >
> > -	if (template_attr->egress || template_attr->transfer)
> > +	if (template_attr->egress || (template_attr->transfer &&
> > +!priv->jump_fdb_rx_en))
> >  		return rte_flow_error_set(error, EINVAL,
> >  					  RTE_FLOW_ERROR_TYPE_ATTR, NULL,
> >  					  "RSS action supported for ingress only"); @@
> -8104,6 +8106,7
> > @@ __flow_hw_actions_template_create(struct rte_eth_dev *dev,
> >  	}
> >  	if (priv->sh->config.dv_xmeta_en ==
> MLX5_XMETA_MODE_META32_HWS &&
> >  	    priv->sh->config.dv_esw_en &&
> > +	    !attr->transfer &&
> >  	    (action_flags & (MLX5_FLOW_ACTION_QUEUE |
> > MLX5_FLOW_ACTION_RSS))) {
> >  		/* Insert META copy */
> >  		mf_actions[expand_mf_num] = rx_meta_copy_action; @@
> -12112,23
> > +12115,29 @@ __flow_hw_configure(struct rte_eth_dev *dev,
> >  	/* Add global actions. */
> >  	for (i = 0; i < MLX5_HW_ACTION_FLAG_MAX; i++) {
> >  		uint32_t act_flags = 0;
> > +		uint32_t tag_flags = mlx5_hw_act_flag[i][0];
> >
> >  		act_flags = mlx5_hw_act_flag[i][MLX5DR_TABLE_TYPE_NIC_RX] |
> >  			    mlx5_hw_act_flag[i][MLX5DR_TABLE_TYPE_NIC_TX];
> >  		if (is_proxy) {
> > -			if (unified_fdb)
> > +			if (unified_fdb) {
> >  				act_flags |=
> >
> > 	(mlx5_hw_act_flag[i][MLX5DR_TABLE_TYPE_FDB_RX] |
> >
> > mlx5_hw_act_flag[i][MLX5DR_TABLE_TYPE_FDB_TX] |
> >
> > mlx5_hw_act_flag[i][MLX5DR_TABLE_TYPE_FDB_UNIFIED]);
> > -			else
> > +				if (i == MLX5_HW_ACTION_FLAG_NONE_ROOT)
> > +					tag_flags |=
> > mlx5_hw_act_flag[i][MLX5DR_TABLE_TYPE_FDB_RX];
> > +			} else {
> >  				act_flags |=
> > mlx5_hw_act_flag[i][MLX5DR_TABLE_TYPE_FDB];
> > +				if (i == MLX5_HW_ACTION_FLAG_NONE_ROOT)
> > +					tag_flags |=
> > mlx5_hw_act_flag[i][MLX5DR_TABLE_TYPE_FDB];
> > +			}
> >  		}
> 
> One small question about the TAG action flags in this case. If unified FDB is
> supported, should only FDB_RX sub-domain of all FDB sub-domains support
> the TAG? Or FDB_UNI also supports it. And if no unified FDB, will the TAG also
> be supported in the legacy FDB domain? Can we add some code comments
> here and update the limitation?
Yes, it is allowed only in unified FDB Rx domain. Let's do this.

> 
> >  		priv->hw_drop[i] = mlx5dr_action_create_dest_drop(priv-
> > >dr_ctx, act_flags);
> >  		if (!priv->hw_drop[i])
> >  			goto err;
> >  		priv->hw_tag[i] = mlx5dr_action_create_tag
> > -			(priv->dr_ctx, mlx5_hw_act_flag[i][0]);
> > +			(priv->dr_ctx, tag_flags);
> >  		if (!priv->hw_tag[i])
> >  			goto err;
> >  	}
> > --
> > 2.34.1