DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ori Kam <orika@mellanox.com>
To: Bing Zhao <bingz@mellanox.com>,
	Slava Ovsiienko <viacheslavo@mellanox.com>,
	Raslan Darawsheh <rasland@mellanox.com>,
	Matan Azrad <matan@mellanox.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] net/mlx5: support more than 16 modify actions
Date: Mon, 25 Nov 2019 07:50:28 +0000	[thread overview]
Message-ID: <AM4PR05MB34250D08A35157F1E04C67BDDB4A0@AM4PR05MB3425.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <1574493996-48427-1-git-send-email-bingz@mellanox.com>



> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Bing Zhao
> Sent: Saturday, November 23, 2019 9:27 AM
> To: Ori Kam <orika@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>
> Cc: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] net/mlx5: support more than 16 modify actions
> 
> 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 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 are not supported.
> 
> Signed-off-by: Bing Zhao <bingz@mellanox.com>
> ---
>  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 3fff5dd..0ad0fc1 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h
> @@ -370,11 +370,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 {
> @@ -385,9 +388,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 02f20fb..75d28bd 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -356,7 +356,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");
> @@ -397,11 +397,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;
>  }
> 
> @@ -562,7 +562,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");
> @@ -895,7 +895,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");
> @@ -907,10 +907,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;
>  }
> 
> @@ -2241,7 +2237,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 &&
> @@ -3352,21 +3347,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;
>  }
> 
>  /**
> @@ -3451,8 +3452,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");
> @@ -3462,17 +3467,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));
> @@ -3482,18 +3485,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);
> @@ -6611,10 +6614,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;
> @@ -6626,15 +6632,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;
> @@ -6672,7 +6682,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;
> @@ -6694,7 +6704,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;
> @@ -6715,7 +6725,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;
> @@ -6723,7 +6733,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;
> @@ -6823,7 +6833,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;
> @@ -6922,7 +6932,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 ?
> @@ -6932,7 +6942,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 ?
> @@ -6942,7 +6952,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 ?
> @@ -6952,7 +6962,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 ==
> @@ -6962,13 +6972,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;
> @@ -6976,7 +6986,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 ?
> @@ -6987,7 +6997,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 ?
> @@ -6996,13 +7006,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;
> @@ -7027,10 +7037,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;
> @@ -7039,7 +7049,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

Acked-by: Ori Kam <orika@mellanox.com>
Thanks
Ori Kam

      reply	other threads:[~2019-11-25  7:50 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-23  7:26 Bing Zhao
2019-11-25  7:50 ` Ori Kam [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AM4PR05MB34250D08A35157F1E04C67BDDB4A0@AM4PR05MB3425.eurprd05.prod.outlook.com \
    --to=orika@mellanox.com \
    --cc=bingz@mellanox.com \
    --cc=dev@dpdk.org \
    --cc=matan@mellanox.com \
    --cc=rasland@mellanox.com \
    --cc=viacheslavo@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).