[dpdk-dev] [PATCH v2] net/mlx5: fix modify actions support limitation

Ori Kam orika at mellanox.com
Thu Jan 16 15:34:06 CET 2020



> -----Original Message-----
> From: Bing Zhao <bingz at mellanox.com>
> Sent: Sunday, January 12, 2020 5:57 PM
> To: Ori Kam <orika at mellanox.com>; Slava Ovsiienko
> <viacheslavo at mellanox.com>; Raslan Darawsheh <rasland at mellanox.com>;
> Matan Azrad <matan at mellanox.com>
> Cc: dev at dpdk.org; stable at dpdk.org
> Subject: [PATCH v2] net/mlx5: fix modify actions support limitation
> 
> In the root table, there is some limitation of total number of header
> modify actions, 16 or 8 for each. But in other tables, there is no
> such strict limitation. In an IPv6 case, the IP fields modifying
> will occupy more actions than that in IPv4, so the total support
> number should be increased in order to support as many actions as
> possible for an IPv6 + TCP packet.
> And in the meanwhile, the memory consumption should also be taken
> into consideration because sometimes only several actions are needed.
> The root table checking could also be done in low layer driver and
> the error code will be returned if the actions number is over the
> maximal supported value.
> 
> Fixes: 0e9d00027686 ("net/mlx5: check maximum modify actions number")
> Signed-off-by: Bing Zhao <bingz at mellanox.com>
> ---
Acked-by: Ori Kam <orika at mellanox.com>

Thanks,
Ori

>  drivers/net/mlx5/mlx5_flow.h    |  15 +++---
>  drivers/net/mlx5/mlx5_flow_dv.c | 104 ++++++++++++++++++++++---------
> ---------
>  2 files changed, 66 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> index 27d82ac..4b493ee 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h
> @@ -385,11 +385,14 @@ struct mlx5_flow_dv_tag_resource {
> 
>  /*
>   * Number of modification commands.
> - * If extensive metadata registers are supported
> - * the maximal actions amount is 16 and 8 otherwise.
> + * If extensive metadata registers are supported, the maximal actions
> amount is
> + * 16 and 8 otherwise on root table. The validation could also be done in the
> + * lower driver layer.
> + * On non-root table, there is no limitation, but 32 is enough right now.
>   */
> -#define MLX5_MODIFY_NUM 16
> -#define MLX5_MODIFY_NUM_NO_MREG 8
> +#define MLX5_MAX_MODIFY_NUM			32
> +#define MLX5_ROOT_TBL_MODIFY_NUM		16
> +#define MLX5_ROOT_TBL_MODIFY_NUM_NO_MREG	8
> 
>  /* Modify resource structure */
>  struct mlx5_flow_dv_modify_hdr_resource {
> @@ -400,9 +403,9 @@ struct mlx5_flow_dv_modify_hdr_resource {
>  	/**< Verbs modify header action object. */
>  	uint8_t ft_type; /**< Flow table type, Rx or Tx. */
>  	uint32_t actions_num; /**< Number of modification actions. */
> -	struct mlx5_modification_cmd actions[MLX5_MODIFY_NUM];
> -	/**< Modification actions. */
>  	uint64_t flags; /**< Flags for RDMA API. */
> +	struct mlx5_modification_cmd actions[];
> +	/**< Modification actions. */
>  };
> 
>  /* Jump action resource structure. */
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c
> index e8a764c..398d60f 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -363,7 +363,7 @@ struct field_modify_info modify_tcp[] = {
>  		uint32_t mask;
>  		uint32_t data;
> 
> -		if (i >= MLX5_MODIFY_NUM)
> +		if (i >= MLX5_MAX_MODIFY_NUM)
>  			return rte_flow_error_set(error, EINVAL,
>  				 RTE_FLOW_ERROR_TYPE_ACTION, NULL,
>  				 "too many items to modify");
> @@ -404,11 +404,11 @@ struct field_modify_info modify_tcp[] = {
>  		++i;
>  		++field;
>  	} while (field->size);
> -	resource->actions_num = i;
> -	if (!resource->actions_num)
> +	if (resource->actions_num == i)
>  		return rte_flow_error_set(error, EINVAL,
>  					  RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
>  					  "invalid modification flow item");
> +	resource->actions_num = i;
>  	return 0;
>  }
> 
> @@ -569,7 +569,7 @@ struct field_modify_info modify_tcp[] = {
>  	struct mlx5_modification_cmd *actions = &resource->actions[i];
>  	struct field_modify_info *field = modify_vlan_out_first_vid;
> 
> -	if (i >= MLX5_MODIFY_NUM)
> +	if (i >= MLX5_MAX_MODIFY_NUM)
>  		return rte_flow_error_set(error, EINVAL,
>  			 RTE_FLOW_ERROR_TYPE_ACTION, NULL,
>  			 "too many items to modify");
> @@ -902,7 +902,7 @@ struct field_modify_info modify_tcp[] = {
>  	struct mlx5_modification_cmd *actions = resource->actions;
>  	uint32_t i = resource->actions_num;
> 
> -	if (i >= MLX5_MODIFY_NUM)
> +	if (i >= MLX5_MAX_MODIFY_NUM)
>  		return rte_flow_error_set(error, EINVAL,
>  					  RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
>  					  "too many items to modify");
> @@ -914,10 +914,6 @@ struct field_modify_info modify_tcp[] = {
>  	actions[i].data1 = rte_cpu_to_be_32(conf->data);
>  	++i;
>  	resource->actions_num = i;
> -	if (!resource->actions_num)
> -		return rte_flow_error_set(error, EINVAL,
> -					  RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
> -					  "invalid modification flow item");
>  	return 0;
>  }
> 
> @@ -2256,7 +2252,6 @@ struct field_modify_info modify_tcp[] = {
>  		domain = sh->rx_domain;
>  	else
>  		domain = sh->tx_domain;
> -
>  	/* Lookup a matching resource from cache. */
>  	LIST_FOREACH(cache_resource, &sh->encaps_decaps, next) {
>  		if (resource->reformat_type == cache_resource-
> >reformat_type &&
> @@ -3367,21 +3362,27 @@ struct field_modify_info modify_tcp[] = {
>   *
>   * @param dev
>   *   Pointer to rte_eth_dev structure.
> + * @param flags
> + *   Flags bits to check if root level.
>   *
>   * @return
>   *   Max number of modify header actions device can support.
>   */
>  static unsigned int
> -flow_dv_modify_hdr_action_max(struct rte_eth_dev *dev)
> +flow_dv_modify_hdr_action_max(struct rte_eth_dev *dev, uint64_t flags)
>  {
>  	/*
>  	 * There's no way to directly query the max cap. Although it has to be
>  	 * acquried by iterative trial, it is a safe assumption that more
>  	 * actions are supported by FW if extensive metadata register is
> -	 * supported.
> +	 * supported. (Only in the root table)
>  	 */
> -	return mlx5_flow_ext_mreg_supported(dev) ?
> MLX5_MODIFY_NUM :
> -
> MLX5_MODIFY_NUM_NO_MREG;
> +	if (!(flags & MLX5DV_DR_ACTION_FLAGS_ROOT_LEVEL))
> +		return MLX5_MAX_MODIFY_NUM;
> +	else
> +		return mlx5_flow_ext_mreg_supported(dev) ?
> +					MLX5_ROOT_TBL_MODIFY_NUM :
> +
> 	MLX5_ROOT_TBL_MODIFY_NUM_NO_MREG;
>  }
> 
>  /**
> @@ -3472,8 +3473,12 @@ struct field_modify_info modify_tcp[] = {
>  	struct mlx5_ibv_shared *sh = priv->sh;
>  	struct mlx5_flow_dv_modify_hdr_resource *cache_resource;
>  	struct mlx5dv_dr_domain *ns;
> +	uint32_t actions_len;
> 
> -	if (resource->actions_num >
> flow_dv_modify_hdr_action_max(dev))
> +	resource->flags =
> +		dev_flow->group ? 0 :
> MLX5DV_DR_ACTION_FLAGS_ROOT_LEVEL;
> +	if (resource->actions_num > flow_dv_modify_hdr_action_max(dev,
> +				    resource->flags))
>  		return rte_flow_error_set(error, EOVERFLOW,
>  					  RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
>  					  "too many modify header items");
> @@ -3483,17 +3488,15 @@ struct field_modify_info modify_tcp[] = {
>  		ns = sh->tx_domain;
>  	else
>  		ns = sh->rx_domain;
> -	resource->flags =
> -		dev_flow->group ? 0 :
> MLX5DV_DR_ACTION_FLAGS_ROOT_LEVEL;
>  	/* Lookup a matching resource from cache. */
> +	actions_len = resource->actions_num * sizeof(resource->actions[0]);
>  	LIST_FOREACH(cache_resource, &sh->modify_cmds, next) {
>  		if (resource->ft_type == cache_resource->ft_type &&
>  		    resource->actions_num == cache_resource->actions_num
> &&
>  		    resource->flags == cache_resource->flags &&
>  		    !memcmp((const void *)resource->actions,
>  			    (const void *)cache_resource->actions,
> -			    (resource->actions_num *
> -					    sizeof(resource->actions[0])))) {
> +			    actions_len)) {
>  			DRV_LOG(DEBUG, "modify-header resource %p:
> refcnt %d++",
>  				(void *)cache_resource,
>  				rte_atomic32_read(&cache_resource-
> >refcnt));
> @@ -3503,18 +3506,18 @@ struct field_modify_info modify_tcp[] = {
>  		}
>  	}
>  	/* Register new modify-header resource. */
> -	cache_resource = rte_calloc(__func__, 1, sizeof(*cache_resource),
> 0);
> +	cache_resource = rte_calloc(__func__, 1,
> +				    sizeof(*cache_resource) + actions_len, 0);
>  	if (!cache_resource)
>  		return rte_flow_error_set(error, ENOMEM,
> 
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
>  					  "cannot allocate resource
> memory");
>  	*cache_resource = *resource;
> +	rte_memcpy(cache_resource->actions, resource->actions,
> actions_len);
>  	cache_resource->verbs_action =
>  		mlx5_glue->dv_create_flow_action_modify_header
> -					(sh->ctx, cache_resource->ft_type,
> -					 ns, cache_resource->flags,
> -					 cache_resource->actions_num *
> -					 sizeof(cache_resource->actions[0]),
> +					(sh->ctx, cache_resource->ft_type,
> ns,
> +					 cache_resource->flags, actions_len,
>  					 (uint64_t *)cache_resource-
> >actions);
>  	if (!cache_resource->verbs_action) {
>  		rte_free(cache_resource);
> @@ -6739,10 +6742,13 @@ struct field_modify_info modify_tcp[] = {
>  	};
>  	int actions_n = 0;
>  	bool actions_end = false;
> -	struct mlx5_flow_dv_modify_hdr_resource mhdr_res = {
> -		.ft_type = attr->egress ?
> MLX5DV_FLOW_TABLE_TYPE_NIC_TX :
> -
> MLX5DV_FLOW_TABLE_TYPE_NIC_RX
> -	};
> +	union {
> +		struct mlx5_flow_dv_modify_hdr_resource res;
> +		uint8_t len[sizeof(struct
> mlx5_flow_dv_modify_hdr_resource) +
> +			    sizeof(struct mlx5_modification_cmd) *
> +			    (MLX5_MAX_MODIFY_NUM + 1)];
> +	} mhdr_dummy;
> +	struct mlx5_flow_dv_modify_hdr_resource *mhdr_res =
> &mhdr_dummy.res;
>  	union flow_dv_attr flow_attr = { .attr = 0 };
>  	uint32_t tag_be;
>  	union mlx5_flow_tbl_key tbl_key;
> @@ -6754,15 +6760,19 @@ struct field_modify_info modify_tcp[] = {
>  	uint32_t table;
>  	int ret = 0;
> 
> +	mhdr_res->ft_type = attr->egress ?
> MLX5DV_FLOW_TABLE_TYPE_NIC_TX :
> +
> MLX5DV_FLOW_TABLE_TYPE_NIC_RX;
>  	ret = mlx5_flow_group_to_table(attr, dev_flow->external, attr-
> >group,
>  				       &table, error);
>  	if (ret)
>  		return ret;
>  	dev_flow->group = table;
>  	if (attr->transfer)
> -		mhdr_res.ft_type = MLX5DV_FLOW_TABLE_TYPE_FDB;
> +		mhdr_res->ft_type = MLX5DV_FLOW_TABLE_TYPE_FDB;
>  	if (priority == MLX5_FLOW_PRIO_RSVD)
>  		priority = dev_conf->flow_prio - 1;
> +	/* number of actions must be set to 0 in case of dirty stack. */
> +	mhdr_res->actions_num = 0;
>  	for (; !actions_end ; actions++) {
>  		const struct rte_flow_action_queue *queue;
>  		const struct rte_flow_action_rss *rss;
> @@ -6800,7 +6810,7 @@ struct field_modify_info modify_tcp[] = {
>  				};
> 
>  				if (flow_dv_convert_action_mark(dev,
> &mark,
> -								&mhdr_res,
> +								mhdr_res,
>  								error))
>  					return -rte_errno;
>  				action_flags |=
> MLX5_FLOW_ACTION_MARK_EXT;
> @@ -6822,7 +6832,7 @@ struct field_modify_info modify_tcp[] = {
>  						actions->conf;
> 
>  				if (flow_dv_convert_action_mark(dev, mark,
> -								&mhdr_res,
> +								mhdr_res,
>  								error))
>  					return -rte_errno;
>  				action_flags |=
> MLX5_FLOW_ACTION_MARK_EXT;
> @@ -6843,7 +6853,7 @@ struct field_modify_info modify_tcp[] = {
>  			break;
>  		case RTE_FLOW_ACTION_TYPE_SET_META:
>  			if (flow_dv_convert_action_set_meta
> -				(dev, &mhdr_res, attr,
> +				(dev, mhdr_res, attr,
>  				 (const struct rte_flow_action_set_meta *)
>  				  actions->conf, error))
>  				return -rte_errno;
> @@ -6851,7 +6861,7 @@ struct field_modify_info modify_tcp[] = {
>  			break;
>  		case RTE_FLOW_ACTION_TYPE_SET_TAG:
>  			if (flow_dv_convert_action_set_tag
> -				(dev, &mhdr_res,
> +				(dev, mhdr_res,
>  				 (const struct rte_flow_action_set_tag *)
>  				  actions->conf, error))
>  				return -rte_errno;
> @@ -6951,7 +6961,7 @@ struct field_modify_info modify_tcp[] = {
>  			mlx5_update_vlan_vid_pcp(actions, &vlan);
>  			/* If no VLAN push - this is a modify header action */
>  			if (flow_dv_convert_action_modify_vlan_vid
> -						(&mhdr_res, actions, error))
> +						(mhdr_res, actions, error))
>  				return -rte_errno;
>  			action_flags |=
> MLX5_FLOW_ACTION_OF_SET_VLAN_VID;
>  			break;
> @@ -7050,7 +7060,7 @@ struct field_modify_info modify_tcp[] = {
>  		case RTE_FLOW_ACTION_TYPE_SET_MAC_SRC:
>  		case RTE_FLOW_ACTION_TYPE_SET_MAC_DST:
>  			if (flow_dv_convert_action_modify_mac
> -					(&mhdr_res, actions, error))
> +					(mhdr_res, actions, error))
>  				return -rte_errno;
>  			action_flags |= actions->type ==
> 
> 	RTE_FLOW_ACTION_TYPE_SET_MAC_SRC ?
> @@ -7060,7 +7070,7 @@ struct field_modify_info modify_tcp[] = {
>  		case RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC:
>  		case RTE_FLOW_ACTION_TYPE_SET_IPV4_DST:
>  			if (flow_dv_convert_action_modify_ipv4
> -					(&mhdr_res, actions, error))
> +					(mhdr_res, actions, error))
>  				return -rte_errno;
>  			action_flags |= actions->type ==
> 
> 	RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC ?
> @@ -7070,7 +7080,7 @@ struct field_modify_info modify_tcp[] = {
>  		case RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC:
>  		case RTE_FLOW_ACTION_TYPE_SET_IPV6_DST:
>  			if (flow_dv_convert_action_modify_ipv6
> -					(&mhdr_res, actions, error))
> +					(mhdr_res, actions, error))
>  				return -rte_errno;
>  			action_flags |= actions->type ==
> 
> 	RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC ?
> @@ -7080,7 +7090,7 @@ struct field_modify_info modify_tcp[] = {
>  		case RTE_FLOW_ACTION_TYPE_SET_TP_SRC:
>  		case RTE_FLOW_ACTION_TYPE_SET_TP_DST:
>  			if (flow_dv_convert_action_modify_tp
> -					(&mhdr_res, actions, items,
> +					(mhdr_res, actions, items,
>  					 &flow_attr, error))
>  				return -rte_errno;
>  			action_flags |= actions->type ==
> @@ -7090,13 +7100,13 @@ struct field_modify_info modify_tcp[] = {
>  			break;
>  		case RTE_FLOW_ACTION_TYPE_DEC_TTL:
>  			if (flow_dv_convert_action_modify_dec_ttl
> -					(&mhdr_res, items, &flow_attr,
> error))
> +					(mhdr_res, items, &flow_attr, error))
>  				return -rte_errno;
>  			action_flags |= MLX5_FLOW_ACTION_DEC_TTL;
>  			break;
>  		case RTE_FLOW_ACTION_TYPE_SET_TTL:
>  			if (flow_dv_convert_action_modify_ttl
> -					(&mhdr_res, actions, items,
> +					(mhdr_res, actions, items,
>  					 &flow_attr, error))
>  				return -rte_errno;
>  			action_flags |= MLX5_FLOW_ACTION_SET_TTL;
> @@ -7104,7 +7114,7 @@ struct field_modify_info modify_tcp[] = {
>  		case RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ:
>  		case RTE_FLOW_ACTION_TYPE_DEC_TCP_SEQ:
>  			if (flow_dv_convert_action_modify_tcp_seq
> -					(&mhdr_res, actions, error))
> +					(mhdr_res, actions, error))
>  				return -rte_errno;
>  			action_flags |= actions->type ==
> 
> 	RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ ?
> @@ -7115,7 +7125,7 @@ struct field_modify_info modify_tcp[] = {
>  		case RTE_FLOW_ACTION_TYPE_INC_TCP_ACK:
>  		case RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK:
>  			if (flow_dv_convert_action_modify_tcp_ack
> -					(&mhdr_res, actions, error))
> +					(mhdr_res, actions, error))
>  				return -rte_errno;
>  			action_flags |= actions->type ==
> 
> 	RTE_FLOW_ACTION_TYPE_INC_TCP_ACK ?
> @@ -7124,13 +7134,13 @@ struct field_modify_info modify_tcp[] = {
>  			break;
>  		case MLX5_RTE_FLOW_ACTION_TYPE_TAG:
>  			if (flow_dv_convert_action_set_reg
> -					(&mhdr_res, actions, error))
> +					(mhdr_res, actions, error))
>  				return -rte_errno;
>  			action_flags |= MLX5_FLOW_ACTION_SET_TAG;
>  			break;
>  		case MLX5_RTE_FLOW_ACTION_TYPE_COPY_MREG:
>  			if (flow_dv_convert_action_copy_mreg
> -					(dev, &mhdr_res, actions, error))
> +					(dev, mhdr_res, actions, error))
>  				return -rte_errno;
>  			action_flags |= MLX5_FLOW_ACTION_SET_TAG;
>  			break;
> @@ -7155,10 +7165,10 @@ struct field_modify_info modify_tcp[] = {
>  			break;
>  		case RTE_FLOW_ACTION_TYPE_END:
>  			actions_end = true;
> -			if (mhdr_res.actions_num) {
> +			if (mhdr_res->actions_num) {
>  				/* create modify action if needed. */
>  				if (flow_dv_modify_hdr_resource_register
> -					(dev, &mhdr_res, dev_flow, error))
> +					(dev, mhdr_res, dev_flow, error))
>  					return -rte_errno;
>  				dev_flow-
> >dv.actions[modify_action_position] =
>  					dev_flow->dv.modify_hdr-
> >verbs_action;
> @@ -7167,7 +7177,7 @@ struct field_modify_info modify_tcp[] = {
>  		default:
>  			break;
>  		}
> -		if (mhdr_res.actions_num &&
> +		if (mhdr_res->actions_num &&
>  		    modify_action_position == UINT32_MAX)
>  			modify_action_position = actions_n++;
>  	}
> --
> 1.8.3.1



More information about the dev mailing list