patches for DPDK stable branches
 help / color / Atom feed
* [dpdk-stable] [PATCH] net/mlx5: fix modify actions support limitation
@ 2020-01-06  6:31 Bing Zhao
  2020-01-12 15:56 ` [dpdk-stable] [PATCH v2] " Bing Zhao
  0 siblings, 1 reply; 8+ messages in thread
From: Bing Zhao @ 2020-01-06  6:31 UTC (permalink / raw)
  To: orika, viacheslavo, rasland, matan; +Cc: dev, stable

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@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 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 4c16281..586b56e 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;
 }
 
@@ -2248,7 +2244,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 &&
@@ -3359,21 +3354,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;
 }
 
 /**
@@ -3458,8 +3459,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");
@@ -3469,17 +3474,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));
@@ -3489,18 +3492,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);
@@ -6687,10 +6690,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;
@@ -6702,15 +6708,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;
@@ -6748,7 +6758,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;
@@ -6770,7 +6780,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;
@@ -6791,7 +6801,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;
@@ -6799,7 +6809,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;
@@ -6899,7 +6909,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;
@@ -6998,7 +7008,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 ?
@@ -7008,7 +7018,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 ?
@@ -7018,7 +7028,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 ?
@@ -7028,7 +7038,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 ==
@@ -7038,13 +7048,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;
@@ -7052,7 +7062,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 ?
@@ -7063,7 +7073,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 ?
@@ -7072,13 +7082,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;
@@ -7103,10 +7113,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;
@@ -7115,7 +7125,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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-stable] [PATCH v2] net/mlx5: fix modify actions support limitation
  2020-01-06  6:31 [dpdk-stable] [PATCH] net/mlx5: fix modify actions support limitation Bing Zhao
@ 2020-01-12 15:56 ` " Bing Zhao
  2020-01-16 14:34   ` Ori Kam
                     ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Bing Zhao @ 2020-01-12 15:56 UTC (permalink / raw)
  To: orika, viacheslavo, rasland, matan; +Cc: dev, stable

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix modify actions support limitation
  2020-01-12 15:56 ` [dpdk-stable] [PATCH v2] " Bing Zhao
@ 2020-01-16 14:34   ` Ori Kam
  2020-01-19 16:21   ` Raslan Darawsheh
  2020-01-20  9:43   ` [dpdk-stable] [PATCH v3] " Bing Zhao
  2 siblings, 0 replies; 8+ messages in thread
From: Ori Kam @ 2020-01-16 14:34 UTC (permalink / raw)
  To: Bing Zhao, Slava Ovsiienko, Raslan Darawsheh, Matan Azrad; +Cc: dev, stable



> -----Original Message-----
> From: Bing Zhao <bingz@mellanox.com>
> Sent: Sunday, January 12, 2020 5:57 PM
> To: Ori Kam <orika@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>;
> Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; stable@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@mellanox.com>
> ---
Acked-by: Ori Kam <orika@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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix modify actions support limitation
  2020-01-12 15:56 ` [dpdk-stable] [PATCH v2] " Bing Zhao
  2020-01-16 14:34   ` Ori Kam
@ 2020-01-19 16:21   ` Raslan Darawsheh
  2020-01-20  9:43   ` [dpdk-stable] [PATCH v3] " Bing Zhao
  2 siblings, 0 replies; 8+ messages in thread
From: Raslan Darawsheh @ 2020-01-19 16:21 UTC (permalink / raw)
  To: Bing Zhao, Ori Kam, Slava Ovsiienko, Matan Azrad; +Cc: dev, stable

Hi Bing,

I see that your patch is causing some build failure on aarch64 with meson build.

-c ../../root/dpdk/drivers/net/mlx5/mlx5_flow_dv.c
../../root/dpdk/drivers/net/mlx5/mlx5_flow_dv.c: In function '__flow_dv_translate':
../../root/dpdk/drivers/net/mlx5/mlx5_flow_dv.c:7339:48: error: passing argument 1 of 'flow_dv_convert_action_modify_ipv4_dscp' from incompatible pointer type [-Werror=incompatible-pointer-types]
    if (flow_dv_convert_action_modify_ipv4_dscp(&mhdr_res,
                                                ^
../../root/dpdk/drivers/net/mlx5/mlx5_flow_dv.c:1213:1: note: expected 'struct mlx5_flow_dv_modify_hdr_resource *' but argument is of type 'struct mlx5_flow_dv_modify_hdr_resource **'
 flow_dv_convert_action_modify_ipv4_dscp
 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../root/dpdk/drivers/net/mlx5/mlx5_flow_dv.c:7345:48: error: passing argument 1 of 'flow_dv_convert_action_modify_ipv6_dscp' from incompatible pointer type [-Werror=incompatible-pointer-types]
    if (flow_dv_convert_action_modify_ipv6_dscp(&mhdr_res,
                                                ^
../../root/dpdk/drivers/net/mlx5/mlx5_flow_dv.c:1248:1: note: expected 'struct mlx5_flow_dv_modify_hdr_resource *' but argument is of type 'struct mlx5_flow_dv_modify_hdr_resource **'
 flow_dv_convert_action_modify_ipv6_dscp
 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
ninja: build stopped: subcommand failed.


Can you please fix that and send a V3 for it ?

Kindest regards,
Raslan Darawsheh

> -----Original Message-----
> From: Bing Zhao <bingz@mellanox.com>
> Sent: Sunday, January 12, 2020 5:57 PM
> To: Ori Kam <orika@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>;
> Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; stable@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@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 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-stable] [PATCH v3] net/mlx5: fix modify actions support limitation
  2020-01-12 15:56 ` [dpdk-stable] [PATCH v2] " Bing Zhao
  2020-01-16 14:34   ` Ori Kam
  2020-01-19 16:21   ` Raslan Darawsheh
@ 2020-01-20  9:43   ` " Bing Zhao
  2020-01-20 13:03     ` Ori Kam
  2020-01-20 14:04     ` Raslan Darawsheh
  2 siblings, 2 replies; 8+ messages in thread
From: Bing Zhao @ 2020-01-20  9:43 UTC (permalink / raw)
  To: orika, viacheslavo, rasland, matan; +Cc: dev, stable

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@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow.h    |  15 +++---
 drivers/net/mlx5/mlx5_flow_dv.c | 108 ++++++++++++++++++++++------------------
 2 files changed, 68 insertions(+), 55 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index e42c98a..2e94371 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -389,11 +389,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 {
@@ -404,9 +407,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 c02517a..eec4c72 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -365,7 +365,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");
@@ -406,11 +406,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;
 }
 
@@ -571,7 +571,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");
@@ -904,7 +904,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");
@@ -916,10 +916,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;
 }
 
@@ -2334,7 +2330,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 &&
@@ -3445,21 +3440,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;
 }
 
 /**
@@ -3618,8 +3619,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");
@@ -3629,17 +3634,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));
@@ -3649,18 +3652,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);
@@ -6911,10 +6914,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;
@@ -6926,15 +6932,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;
@@ -6972,7 +6982,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;
@@ -6994,7 +7004,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;
@@ -7015,7 +7025,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;
@@ -7023,7 +7033,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;
@@ -7123,7 +7133,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;
@@ -7222,7 +7232,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 ?
@@ -7232,7 +7242,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 ?
@@ -7242,7 +7252,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 ?
@@ -7252,7 +7262,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 ==
@@ -7262,13 +7272,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;
@@ -7276,7 +7286,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 ?
@@ -7287,7 +7297,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 ?
@@ -7296,13 +7306,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;
@@ -7326,23 +7336,23 @@ struct field_modify_info modify_tcp[] = {
 			action_flags |= MLX5_FLOW_ACTION_METER;
 			break;
 		case RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP:
-			if (flow_dv_convert_action_modify_ipv4_dscp(&mhdr_res,
+			if (flow_dv_convert_action_modify_ipv4_dscp(mhdr_res,
 							      actions, error))
 				return -rte_errno;
 			action_flags |= MLX5_FLOW_ACTION_SET_IPV4_DSCP;
 			break;
 		case RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP:
-			if (flow_dv_convert_action_modify_ipv6_dscp(&mhdr_res,
+			if (flow_dv_convert_action_modify_ipv6_dscp(mhdr_res,
 							      actions, error))
 				return -rte_errno;
 			action_flags |= MLX5_FLOW_ACTION_SET_IPV6_DSCP;
 			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;
@@ -7351,7 +7361,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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-stable] [PATCH v3] net/mlx5: fix modify actions support limitation
  2020-01-20  9:43   ` [dpdk-stable] [PATCH v3] " Bing Zhao
@ 2020-01-20 13:03     ` Ori Kam
  2020-01-20 13:06       ` Slava Ovsiienko
  2020-01-20 14:04     ` Raslan Darawsheh
  1 sibling, 1 reply; 8+ messages in thread
From: Ori Kam @ 2020-01-20 13:03 UTC (permalink / raw)
  To: Bing Zhao, Slava Ovsiienko, Raslan Darawsheh, Matan Azrad; +Cc: dev, stable



> -----Original Message-----
> From: Bing Zhao <bingz@mellanox.com>
> Sent: Monday, January 20, 2020 11:43 AM
> To: Ori Kam <orika@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>;
> Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [PATCH v3] 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@mellanox.com>
> ---
Acked-by: Ori Kam <orika@mellanox.com>
Thanks,
Ori


>  drivers/net/mlx5/mlx5_flow.h    |  15 +++---
>  drivers/net/mlx5/mlx5_flow_dv.c | 108 ++++++++++++++++++++++---------
> ---------
>  2 files changed, 68 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> index e42c98a..2e94371 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h
> @@ -389,11 +389,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 {
> @@ -404,9 +407,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 c02517a..eec4c72 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -365,7 +365,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");
> @@ -406,11 +406,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;
>  }
> 
> @@ -571,7 +571,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");
> @@ -904,7 +904,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");
> @@ -916,10 +916,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;
>  }
> 
> @@ -2334,7 +2330,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 &&
> @@ -3445,21 +3440,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;
>  }
> 
>  /**
> @@ -3618,8 +3619,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");
> @@ -3629,17 +3634,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));
> @@ -3649,18 +3652,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);
> @@ -6911,10 +6914,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;
> @@ -6926,15 +6932,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;
> @@ -6972,7 +6982,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;
> @@ -6994,7 +7004,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;
> @@ -7015,7 +7025,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;
> @@ -7023,7 +7033,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;
> @@ -7123,7 +7133,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;
> @@ -7222,7 +7232,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 ?
> @@ -7232,7 +7242,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 ?
> @@ -7242,7 +7252,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 ?
> @@ -7252,7 +7262,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 ==
> @@ -7262,13 +7272,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;
> @@ -7276,7 +7286,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 ?
> @@ -7287,7 +7297,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 ?
> @@ -7296,13 +7306,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;
> @@ -7326,23 +7336,23 @@ struct field_modify_info modify_tcp[] = {
>  			action_flags |= MLX5_FLOW_ACTION_METER;
>  			break;
>  		case RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP:
> -			if
> (flow_dv_convert_action_modify_ipv4_dscp(&mhdr_res,
> +			if
> (flow_dv_convert_action_modify_ipv4_dscp(mhdr_res,
>  							      actions, error))
>  				return -rte_errno;
>  			action_flags |=
> MLX5_FLOW_ACTION_SET_IPV4_DSCP;
>  			break;
>  		case RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP:
> -			if
> (flow_dv_convert_action_modify_ipv6_dscp(&mhdr_res,
> +			if
> (flow_dv_convert_action_modify_ipv6_dscp(mhdr_res,
>  							      actions, error))
>  				return -rte_errno;
>  			action_flags |=
> MLX5_FLOW_ACTION_SET_IPV6_DSCP;
>  			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;
> @@ -7351,7 +7361,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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-stable] [PATCH v3] net/mlx5: fix modify actions support limitation
  2020-01-20 13:03     ` Ori Kam
@ 2020-01-20 13:06       ` Slava Ovsiienko
  0 siblings, 0 replies; 8+ messages in thread
From: Slava Ovsiienko @ 2020-01-20 13:06 UTC (permalink / raw)
  To: Ori Kam, Bing Zhao, Raslan Darawsheh, Matan Azrad; +Cc: dev, stable

> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> Sent: Monday, January 20, 2020 15:03
> 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; stable@dpdk.org
> Subject: RE: [PATCH v3] net/mlx5: fix modify actions support limitation
> 
> 
> 
> > -----Original Message-----
> > From: Bing Zhao <bingz@mellanox.com>
> > Sent: Monday, January 20, 2020 11:43 AM
> > To: Ori Kam <orika@mellanox.com>; Slava Ovsiienko
> > <viacheslavo@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>;
> > Matan Azrad <matan@mellanox.com>
> > Cc: dev@dpdk.org; stable@dpdk.org
> > Subject: [PATCH v3] 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@mellanox.com>
> > ---
> Acked-by: Ori Kam <orika@mellanox.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

> Thanks,
> Ori
> 
> 
> >  drivers/net/mlx5/mlx5_flow.h    |  15 +++---
> >  drivers/net/mlx5/mlx5_flow_dv.c | 108 ++++++++++++++++++++++---------
> > ---------
> >  2 files changed, 68 insertions(+), 55 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_flow.h
> > b/drivers/net/mlx5/mlx5_flow.h index e42c98a..2e94371 100644
> > --- a/drivers/net/mlx5/mlx5_flow.h
> > +++ b/drivers/net/mlx5/mlx5_flow.h
> > @@ -389,11 +389,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 { @@ -404,9 +407,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 c02517a..eec4c72 100644
> > --- a/drivers/net/mlx5/mlx5_flow_dv.c
> > +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> > @@ -365,7 +365,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");
> > @@ -406,11 +406,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;
> >  }
> >
> > @@ -571,7 +571,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");
> > @@ -904,7 +904,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");
> > @@ -916,10 +916,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;
> >  }
> >
> > @@ -2334,7 +2330,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 &&
> > @@ -3445,21 +3440,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;
> >  }
> >
> >  /**
> > @@ -3618,8 +3619,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");
> @@ -3629,17 +3634,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));
> > @@ -3649,18 +3652,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);
> > @@ -6911,10 +6914,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;
> > @@ -6926,15 +6932,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; @@ -6972,7 +6982,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;
> > @@ -6994,7 +7004,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;
> > @@ -7015,7 +7025,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;
> > @@ -7023,7 +7033,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;
> > @@ -7123,7 +7133,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;
> > @@ -7222,7 +7232,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 ?
> > @@ -7232,7 +7242,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 ?
> > @@ -7242,7 +7252,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 ?
> > @@ -7252,7 +7262,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 ==
> > @@ -7262,13 +7272,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; @@ -
> 7276,7 +7286,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 ?
> > @@ -7287,7 +7297,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 ?
> > @@ -7296,13 +7306,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;
> > @@ -7326,23 +7336,23 @@ struct field_modify_info modify_tcp[] = {
> >  			action_flags |= MLX5_FLOW_ACTION_METER;
> >  			break;
> >  		case RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP:
> > -			if
> > (flow_dv_convert_action_modify_ipv4_dscp(&mhdr_res,
> > +			if
> > (flow_dv_convert_action_modify_ipv4_dscp(mhdr_res,
> >  							      actions, error))
> >  				return -rte_errno;
> >  			action_flags |=
> > MLX5_FLOW_ACTION_SET_IPV4_DSCP;
> >  			break;
> >  		case RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP:
> > -			if
> > (flow_dv_convert_action_modify_ipv6_dscp(&mhdr_res,
> > +			if
> > (flow_dv_convert_action_modify_ipv6_dscp(mhdr_res,
> >  							      actions, error))
> >  				return -rte_errno;
> >  			action_flags |=
> > MLX5_FLOW_ACTION_SET_IPV6_DSCP;
> >  			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;
> > @@ -7351,7 +7361,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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-stable] [PATCH v3] net/mlx5: fix modify actions support limitation
  2020-01-20  9:43   ` [dpdk-stable] [PATCH v3] " Bing Zhao
  2020-01-20 13:03     ` Ori Kam
@ 2020-01-20 14:04     ` Raslan Darawsheh
  1 sibling, 0 replies; 8+ messages in thread
From: Raslan Darawsheh @ 2020-01-20 14:04 UTC (permalink / raw)
  To: Bing Zhao, Ori Kam, Slava Ovsiienko, Matan Azrad; +Cc: dev, stable

Hi,
> -----Original Message-----
> From: Bing Zhao <bingz@mellanox.com>
> Sent: Monday, January 20, 2020 11:43 AM
> To: Ori Kam <orika@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>;
> Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [PATCH v3] 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@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_flow.h    |  15 +++---
>  drivers/net/mlx5/mlx5_flow_dv.c | 108 ++++++++++++++++++++++---------
> ---------
>  2 files changed, 68 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> index e42c98a..2e94371 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h


Added  Cc: stable@dpdk.org

Patch applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-06  6:31 [dpdk-stable] [PATCH] net/mlx5: fix modify actions support limitation Bing Zhao
2020-01-12 15:56 ` [dpdk-stable] [PATCH v2] " Bing Zhao
2020-01-16 14:34   ` Ori Kam
2020-01-19 16:21   ` Raslan Darawsheh
2020-01-20  9:43   ` [dpdk-stable] [PATCH v3] " Bing Zhao
2020-01-20 13:03     ` Ori Kam
2020-01-20 13:06       ` Slava Ovsiienko
2020-01-20 14:04     ` Raslan Darawsheh

patches for DPDK stable branches

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox