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

[Bing Zhao <bingz@nvidia.com>]

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

>  		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