DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/mlx5: fix incorrect group value of sample suffix flow
@ 2020-10-28  7:41 Jiawei Wang
  2020-10-30  7:26 ` [dpdk-dev] [PATCH v2] " Jiawei Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Jiawei Wang @ 2020-10-28  7:41 UTC (permalink / raw)
  To: viacheslavo, matan, shahafs, orika; +Cc: dev, rasland

While creating the suffix flow, MLX5 PMD fetch the next flow
group from the sample resource and create the suffix flow
base on it, and the next group value already be scaled but
still go to scale with table factor during suffix flow
translation.

The fix introduce a 'skip_scale' flag that will be set to 1
for the sample suffix flow, and will skip the scale with table
factor in the translation function.

Redmine: 2337417
Fixes: dc67aa65c698 ("net/mlx5: implement tunnel offload API")

Signed-off-by: Jiawei Wang <jiaweiw@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow.c    | 155 ++++++++++++++++++----------------------
 drivers/net/mlx5/mlx5_flow.h    |  15 +++-
 drivers/net/mlx5/mlx5_flow_dv.c |   2 +
 3 files changed, 85 insertions(+), 87 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 2cf15f0..451eab9 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -4398,20 +4398,14 @@ struct tunnel_default_miss_ctx {
  *   Parent flow structure pointer.
  * @param[in, out] sub_flow
  *   Pointer to return the created subflow, may be NULL.
- * @param[in] prefix_layers
- *   Prefix subflow layers, may be 0.
- * @param[in] prefix_mark
- *   Prefix subflow mark flag, may be 0.
  * @param[in] attr
  *   Flow rule attributes.
  * @param[in] items
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -4421,22 +4415,21 @@ struct tunnel_default_miss_ctx {
 flow_create_split_inner(struct rte_eth_dev *dev,
 			struct rte_flow *flow,
 			struct mlx5_flow **sub_flow,
-			uint64_t prefix_layers,
-			uint32_t prefix_mark,
 			const struct rte_flow_attr *attr,
 			const struct rte_flow_item items[],
 			const struct rte_flow_action actions[],
-			bool external, uint32_t flow_idx,
+			struct mlx5_flow_split_info *flow_split_info,
 			struct rte_flow_error *error)
 {
 	struct mlx5_flow *dev_flow;
 
 	dev_flow = flow_drv_prepare(dev, flow, attr, items, actions,
-		flow_idx, error);
+				    flow_split_info->flow_idx, error);
 	if (!dev_flow)
 		return -rte_errno;
 	dev_flow->flow = flow;
-	dev_flow->external = external;
+	dev_flow->external = flow_split_info->external;
+	dev_flow->skip_scale = flow_split_info->skip_scale;
 	/* Subflow object was created, we must include one in the list. */
 	SILIST_INSERT(&flow->dev_handles, dev_flow->handle_idx,
 		      dev_flow->handle, next);
@@ -4445,9 +4438,9 @@ struct tunnel_default_miss_ctx {
 	 * flow may need some user defined item layer flags, and pass the
 	 * Metadate rxq mark flag to suffix flow as well.
 	 */
-	if (prefix_layers)
-		dev_flow->handle->layers = prefix_layers;
-	if (prefix_mark)
+	if (flow_split_info->prefix_layers)
+		dev_flow->handle->layers = flow_split_info->prefix_layers;
+	if (flow_split_info->prefix_mark)
 		dev_flow->handle->mark = 1;
 	if (sub_flow)
 		*sub_flow = dev_flow;
@@ -4989,20 +4982,14 @@ struct tunnel_default_miss_ctx {
  *   Pointer to Ethernet device.
  * @param[in] flow
  *   Parent flow structure pointer.
- * @param[in] prefix_layers
- *   Prefix flow layer flags.
- * @param[in] prefix_mark
- *   Prefix subflow mark flag, may be 0.
  * @param[in] attr
  *   Flow rule attributes.
  * @param[in] items
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -5011,12 +4998,10 @@ struct tunnel_default_miss_ctx {
 static int
 flow_create_split_metadata(struct rte_eth_dev *dev,
 			   struct rte_flow *flow,
-			   uint64_t prefix_layers,
-			   uint32_t prefix_mark,
 			   const struct rte_flow_attr *attr,
 			   const struct rte_flow_item items[],
 			   const struct rte_flow_action actions[],
-			   bool external, uint32_t flow_idx,
+			   struct mlx5_flow_split_info *flow_split_info,
 			   struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
@@ -5035,10 +5020,8 @@ struct tunnel_default_miss_ctx {
 	if (!config->dv_flow_en ||
 	    config->dv_xmeta_en == MLX5_XMETA_MODE_LEGACY ||
 	    !mlx5_flow_ext_mreg_supported(dev))
-		return flow_create_split_inner(dev, flow, NULL, prefix_layers,
-					       prefix_mark, attr, items,
-					       actions, external, flow_idx,
-					       error);
+		return flow_create_split_inner(dev, flow, NULL, attr, items,
+					       actions, flow_split_info, error);
 	actions_n = flow_parse_metadata_split_actions_info(actions, &qrss,
 							   &encap_idx);
 	if (qrss) {
@@ -5123,10 +5106,9 @@ struct tunnel_default_miss_ctx {
 			goto exit;
 	}
 	/* Add the unmodified original or prefix subflow. */
-	ret = flow_create_split_inner(dev, flow, &dev_flow, prefix_layers,
-				      prefix_mark, attr,
+	ret = flow_create_split_inner(dev, flow, &dev_flow, attr,
 				      items, ext_actions ? ext_actions :
-				      actions, external, flow_idx, error);
+				      actions, flow_split_info, error);
 	if (ret < 0)
 		goto exit;
 	MLX5_ASSERT(dev_flow);
@@ -5187,10 +5169,12 @@ struct tunnel_default_miss_ctx {
 		}
 		dev_flow = NULL;
 		/* Add suffix subflow to execute Q/RSS. */
-		ret = flow_create_split_inner(dev, flow, &dev_flow, layers, 0,
+		flow_split_info->prefix_layers = layers;
+		flow_split_info->prefix_mark = 0;
+		ret = flow_create_split_inner(dev, flow, &dev_flow,
 					      &q_attr, mtr_sfx ? items :
 					      q_items, q_actions,
-					      external, flow_idx, error);
+					      flow_split_info, error);
 		if (ret < 0)
 			goto exit;
 		/* qrss ID should be freed if failed. */
@@ -5223,20 +5207,14 @@ struct tunnel_default_miss_ctx {
  *   Pointer to Ethernet device.
  * @param[in] flow
  *   Parent flow structure pointer.
- * @param[in] prefix_layers
- *   Prefix subflow layers, may be 0.
- * @param[in] prefix_mark
- *   Prefix subflow mark flag, may be 0.
  * @param[in] attr
  *   Flow rule attributes.
  * @param[in] items
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -5245,12 +5223,10 @@ struct tunnel_default_miss_ctx {
 static int
 flow_create_split_meter(struct rte_eth_dev *dev,
 			struct rte_flow *flow,
-			uint64_t prefix_layers,
-			uint32_t prefix_mark,
 			const struct rte_flow_attr *attr,
 			const struct rte_flow_item items[],
 			const struct rte_flow_action actions[],
-			bool external, uint32_t flow_idx,
+			struct mlx5_flow_split_info *flow_split_info,
 			struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
@@ -5294,11 +5270,10 @@ struct tunnel_default_miss_ctx {
 			goto exit;
 		}
 		/* Add the prefix subflow. */
+		flow_split_info->prefix_mark = 0;
 		ret = flow_create_split_inner(dev, flow, &dev_flow,
-					      prefix_layers, 0,
-					      attr, items,
-					      pre_actions, external,
-					      flow_idx, error);
+					      attr, items, pre_actions,
+					      flow_split_info, error);
 		if (ret) {
 			ret = -rte_errno;
 			goto exit;
@@ -5308,16 +5283,16 @@ struct tunnel_default_miss_ctx {
 		sfx_attr.group = sfx_attr.transfer ?
 				(MLX5_FLOW_TABLE_LEVEL_SUFFIX - 1) :
 				 MLX5_FLOW_TABLE_LEVEL_SUFFIX;
+		flow_split_info->prefix_layers =
+				flow_get_prefix_layer_flags(dev_flow);
+		flow_split_info->prefix_mark = dev_flow->handle->mark;
 	}
 	/* Add the prefix subflow. */
-	ret = flow_create_split_metadata(dev, flow, dev_flow ?
-					 flow_get_prefix_layer_flags(dev_flow) :
-					 prefix_layers, dev_flow ?
-					 dev_flow->handle->mark : prefix_mark,
+	ret = flow_create_split_metadata(dev, flow,
 					 &sfx_attr, sfx_items ?
 					 sfx_items : items,
 					 sfx_actions ? sfx_actions : actions,
-					 external, flow_idx, error);
+					 flow_split_info, error);
 exit:
 	if (sfx_actions)
 		mlx5_free(sfx_actions);
@@ -5349,10 +5324,8 @@ struct tunnel_default_miss_ctx {
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -5364,7 +5337,7 @@ struct tunnel_default_miss_ctx {
 			 const struct rte_flow_attr *attr,
 			 const struct rte_flow_item items[],
 			 const struct rte_flow_action actions[],
-			 bool external, uint32_t flow_idx,
+			 struct mlx5_flow_split_info *flow_split_info,
 			 struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
@@ -5421,9 +5394,9 @@ struct tunnel_default_miss_ctx {
 			goto exit;
 		}
 		/* Add the prefix subflow. */
-		ret = flow_create_split_inner(dev, flow, &dev_flow, 0, 0, attr,
-					      items, pre_actions, external,
-					      flow_idx, error);
+		ret = flow_create_split_inner(dev, flow, &dev_flow, attr,
+					      items, pre_actions,
+					      flow_split_info, error);
 		if (ret) {
 			ret = -rte_errno;
 			goto exit;
@@ -5441,15 +5414,20 @@ struct tunnel_default_miss_ctx {
 		sfx_attr.group = sfx_attr.transfer ?
 					(sfx_table_key.table_id - 1) :
 					 sfx_table_key.table_id;
+		flow_split_info->prefix_layers =
+				flow_get_prefix_layer_flags(dev_flow);
+		flow_split_info->prefix_mark = dev_flow->handle->mark;
+		/* Suffix group level already be scaled with factor, set
+		 * skip_scale to 1 to avoid scale again in translation.
+		 */
+		flow_split_info->skip_scale = 1;
 #endif
 	}
 	/* Add the suffix subflow. */
-	ret = flow_create_split_meter(dev, flow, dev_flow ?
-				 flow_get_prefix_layer_flags(dev_flow) : 0,
-				 dev_flow ? dev_flow->handle->mark : 0,
-				 &sfx_attr, sfx_items ? sfx_items : items,
-				 sfx_actions ? sfx_actions : actions,
-				 external, flow_idx, error);
+	ret = flow_create_split_meter(dev, flow, &sfx_attr,
+				      sfx_items ? sfx_items : items,
+				      sfx_actions ? sfx_actions : actions,
+				      flow_split_info, error);
 exit:
 	if (sfx_actions)
 		mlx5_free(sfx_actions);
@@ -5484,10 +5462,8 @@ struct tunnel_default_miss_ctx {
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -5499,13 +5475,13 @@ struct tunnel_default_miss_ctx {
 			const struct rte_flow_attr *attr,
 			const struct rte_flow_item items[],
 			const struct rte_flow_action actions[],
-			bool external, uint32_t flow_idx,
+			struct mlx5_flow_split_info *flow_split_info,
 			struct rte_flow_error *error)
 {
 	int ret;
 
 	ret = flow_create_split_sample(dev, flow, attr, items,
-				       actions, external, flow_idx, error);
+				       actions, flow_split_info, error);
 	MLX5_ASSERT(ret <= 0);
 	return ret;
 }
@@ -5594,11 +5570,17 @@ struct tunnel_default_miss_ctx {
 	int hairpin_flow;
 	uint32_t hairpin_id = 0;
 	struct rte_flow_attr attr_tx = { .priority = 0 };
-	struct rte_flow_attr attr_factor = {0};
 	const struct rte_flow_action *actions;
 	struct rte_flow_action *translated_actions = NULL;
 	struct mlx5_flow_tunnel *tunnel;
 	struct tunnel_default_miss_ctx default_miss_ctx = { 0, };
+	struct mlx5_flow_split_info flow_split_info = {
+		.external = !!external,
+		.skip_scale = 0,
+		.flow_idx = 0,
+		.prefix_mark = 0,
+		.prefix_layers = 0
+	};
 	int ret = flow_shared_actions_translate(original_actions,
 						shared_actions,
 						&shared_actions_n,
@@ -5609,10 +5591,9 @@ struct tunnel_default_miss_ctx {
 		return 0;
 	}
 	actions = translated_actions ? translated_actions : original_actions;
-	memcpy((void *)&attr_factor, (const void *)attr, sizeof(*attr));
 	p_actions_rx = actions;
-	hairpin_flow = flow_check_hairpin_split(dev, &attr_factor, actions);
-	ret = flow_drv_validate(dev, &attr_factor, items, p_actions_rx,
+	hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
+	ret = flow_drv_validate(dev, attr, items, p_actions_rx,
 				external, hairpin_flow, error);
 	if (ret < 0)
 		goto error_before_hairpin_split;
@@ -5631,7 +5612,8 @@ struct tunnel_default_miss_ctx {
 		rte_errno = ENOMEM;
 		goto error_before_flow;
 	}
-	flow->drv_type = flow_get_drv_type(dev, &attr_factor);
+	flow_split_info.flow_idx = idx;
+	flow->drv_type = flow_get_drv_type(dev, attr);
 	if (hairpin_id != 0)
 		flow->hairpin_flow_id = hairpin_id;
 	MLX5_ASSERT(flow->drv_type > MLX5_FLOW_TYPE_MIN &&
@@ -5678,9 +5660,9 @@ struct tunnel_default_miss_ctx {
 		 * depending on configuration. In the simplest
 		 * case it just creates unmodified original flow.
 		 */
-		ret = flow_create_split_outer(dev, flow, &attr_factor,
+		ret = flow_create_split_outer(dev, flow, attr,
 					      buf->entry[i].pattern,
-					      p_actions_rx, external, idx,
+					      p_actions_rx, &flow_split_info,
 					      error);
 		if (ret < 0)
 			goto error;
@@ -5728,8 +5710,8 @@ struct tunnel_default_miss_ctx {
 	 * the egress Flows belong to the different device and
 	 * copy table should be updated in peer NIC Rx domain.
 	 */
-	if (attr_factor.ingress &&
-	    (external || attr_factor.group != MLX5_FLOW_MREG_CP_TABLE_GROUP)) {
+	if (attr->ingress &&
+	    (external || attr->group != MLX5_FLOW_MREG_CP_TABLE_GROUP)) {
 		ret = flow_mreg_update_copy_table(dev, flow, actions, error);
 		if (ret)
 			goto error;
@@ -7622,7 +7604,8 @@ struct mlx5_meter_domains_infos *
 	int ret;
 	bool standard_translation;
 
-	if (grp_info.external && group < MLX5_MAX_TABLES_EXTERNAL)
+	if (!grp_info.skip_scale && grp_info.external &&
+	    group < MLX5_MAX_TABLES_EXTERNAL)
 		group *= MLX5_FLOW_TABLE_FACTOR;
 	if (is_tunnel_offload_active(dev)) {
 		standard_translation = !grp_info.external ||
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index 06db440..ccd6745 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -759,6 +759,7 @@ struct mlx5_flow_verbs_workspace {
 #define MLX5_NUM_MAX_DEV_FLOWS 32
 
 /** Device flow structure. */
+__extension__
 struct mlx5_flow {
 	struct rte_flow *flow; /**< Pointer to the main flow. */
 	uint32_t flow_idx; /**< The memory pool index to the main flow. */
@@ -766,7 +767,9 @@ struct mlx5_flow {
 	uint64_t act_flags;
 	/**< Bit-fields of detected actions, see MLX5_FLOW_ACTION_*. */
 	bool external; /**< true if the flow is created external to PMD. */
-	uint8_t ingress; /**< 1 if the flow is ingress. */
+	uint8_t ingress:1; /**< 1 if the flow is ingress. */
+	uint8_t skip_scale:1;
+	/**< 1 if skip the scale the table with factor. */
 	union {
 #ifdef HAVE_IBV_FLOW_DV_SUPPORT
 		struct mlx5_flow_dv_workspace dv;
@@ -1102,6 +1105,15 @@ struct rte_flow_shared_action {
 	};
 };
 
+struct mlx5_flow_split_info {
+	bool external;
+	/**< True if flow is created by request external to PMD. */
+	uint8_t skip_scale; /**< Skip the scale the table with factor. */
+	uint32_t flow_idx; /**< This memory pool index to the flow. */
+	uint32_t prefix_mark; /**< Prefix subflow mark flag. */
+	uint64_t prefix_layers; /**< Prefix subflow layers. */
+};
+
 typedef int (*mlx5_flow_validate_t)(struct rte_eth_dev *dev,
 				    const struct rte_flow_attr *attr,
 				    const struct rte_flow_item items[],
@@ -1216,6 +1228,7 @@ struct flow_grp_info {
 	uint64_t fdb_def_rule:1;
 	/* force standard group translation */
 	uint64_t std_tbl_fix:1;
+	uint64_t skip_scale:1;
 };
 
 static inline bool
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 4a35010..08d17f8 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -9295,6 +9295,7 @@ struct field_modify_info modify_tcp[] = {
 		.external = !!dev_flow->external,
 		.transfer = !!attr->transfer,
 		.fdb_def_rule = !!priv->fdb_def_rule,
+		.skip_scale = !!dev_flow->skip_scale,
 	};
 
 	memset(&mdest_res, 0, sizeof(struct mlx5_flow_dv_dest_array_resource));
@@ -9629,6 +9630,7 @@ struct field_modify_info modify_tcp[] = {
 			jump_group = ((const struct rte_flow_action_jump *)
 							action->conf)->group;
 			grp_info.std_tbl_fix = 0;
+			grp_info.skip_scale = 0;
 			ret = mlx5_flow_group_to_table(dev, tunnel,
 						       jump_group,
 						       &table,
-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v2] fix incorrect group value of sample suffix flow
  2020-10-28  7:41 [dpdk-dev] [PATCH] net/mlx5: fix incorrect group value of sample suffix flow Jiawei Wang
@ 2020-10-30  7:26 ` Jiawei Wang
  2020-10-30  7:26   ` [dpdk-dev] [PATCH v2] net/mlx5: " Jiawei Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Jiawei Wang @ 2020-10-30  7:26 UTC (permalink / raw)
  To: viacheslavo, matan, orika; +Cc: dev, rasland

This patch fix the wrong flow group configuration for sample suffix flow in mlx5 pmd driver.

v2:
* Update commit message
* Rebase

Jiawei Wang (1):
  net/mlx5: fix incorrect group value of sample suffix flow

 drivers/net/mlx5/mlx5_flow.c    | 155 ++++++++++++++++++----------------------
 drivers/net/mlx5/mlx5_flow.h    |  15 +++-
 drivers/net/mlx5/mlx5_flow_dv.c |   2 +
 drivers/net/mlx5/mlx5_rxq.c     |   2 +
 4 files changed, 87 insertions(+), 87 deletions(-)

-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v2] net/mlx5: fix incorrect group value of sample suffix flow
  2020-10-30  7:26 ` [dpdk-dev] [PATCH v2] " Jiawei Wang
@ 2020-10-30  7:26   ` Jiawei Wang
  2020-11-01  6:50     ` [dpdk-dev] [PATCH v3] " Jiawei Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Jiawei Wang @ 2020-10-30  7:26 UTC (permalink / raw)
  To: viacheslavo, matan, orika; +Cc: dev, rasland

mlx5 PMD splited the sampling flow into prefix flow and suffix
flow. On the sample action translation function, the scaled
group value of suffix flow be attached into sample object and
saved into sample resource.

mlx5 PMD fetched the group value from the sample resource to
create the suffix flow. On the mlx5_flow_group_to_table
function the group value of suffix flow was scaled with table
factor again and translated into HW table. That caused the
incorrect group value of sample suffix flow.

The fix introduces a 'skip_scale' flag and sets it to 1 for the
sample suffix flow creation. On the mlx5_flow_group_to_table
function skips the scale with table factor to use the correct
group value.

Redmine: 2337417
Fixes: dc67aa6 ("net/mlx5: implement tunnel offload API")

Signed-off-by: Jiawei Wang <jiaweiw@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow.c    | 155 ++++++++++++++++++----------------------
 drivers/net/mlx5/mlx5_flow.h    |  15 +++-
 drivers/net/mlx5/mlx5_flow_dv.c |   2 +
 drivers/net/mlx5/mlx5_rxq.c     |   2 +
 4 files changed, 87 insertions(+), 87 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index a6e60af..dc7cb95 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -4288,20 +4288,14 @@ struct tunnel_default_miss_ctx {
  *   Parent flow structure pointer.
  * @param[in, out] sub_flow
  *   Pointer to return the created subflow, may be NULL.
- * @param[in] prefix_layers
- *   Prefix subflow layers, may be 0.
- * @param[in] prefix_mark
- *   Prefix subflow mark flag, may be 0.
  * @param[in] attr
  *   Flow rule attributes.
  * @param[in] items
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -4311,22 +4305,21 @@ struct tunnel_default_miss_ctx {
 flow_create_split_inner(struct rte_eth_dev *dev,
 			struct rte_flow *flow,
 			struct mlx5_flow **sub_flow,
-			uint64_t prefix_layers,
-			uint32_t prefix_mark,
 			const struct rte_flow_attr *attr,
 			const struct rte_flow_item items[],
 			const struct rte_flow_action actions[],
-			bool external, uint32_t flow_idx,
+			struct mlx5_flow_split_info *flow_split_info,
 			struct rte_flow_error *error)
 {
 	struct mlx5_flow *dev_flow;
 
 	dev_flow = flow_drv_prepare(dev, flow, attr, items, actions,
-		flow_idx, error);
+				    flow_split_info->flow_idx, error);
 	if (!dev_flow)
 		return -rte_errno;
 	dev_flow->flow = flow;
-	dev_flow->external = external;
+	dev_flow->external = flow_split_info->external;
+	dev_flow->skip_scale = flow_split_info->skip_scale;
 	/* Subflow object was created, we must include one in the list. */
 	SILIST_INSERT(&flow->dev_handles, dev_flow->handle_idx,
 		      dev_flow->handle, next);
@@ -4335,9 +4328,9 @@ struct tunnel_default_miss_ctx {
 	 * flow may need some user defined item layer flags, and pass the
 	 * Metadate rxq mark flag to suffix flow as well.
 	 */
-	if (prefix_layers)
-		dev_flow->handle->layers = prefix_layers;
-	if (prefix_mark)
+	if (flow_split_info->prefix_layers)
+		dev_flow->handle->layers = flow_split_info->prefix_layers;
+	if (flow_split_info->prefix_mark)
 		dev_flow->handle->mark = 1;
 	if (sub_flow)
 		*sub_flow = dev_flow;
@@ -4891,20 +4884,14 @@ struct tunnel_default_miss_ctx {
  *   Pointer to Ethernet device.
  * @param[in] flow
  *   Parent flow structure pointer.
- * @param[in] prefix_layers
- *   Prefix flow layer flags.
- * @param[in] prefix_mark
- *   Prefix subflow mark flag, may be 0.
  * @param[in] attr
  *   Flow rule attributes.
  * @param[in] items
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -4913,12 +4900,10 @@ struct tunnel_default_miss_ctx {
 static int
 flow_create_split_metadata(struct rte_eth_dev *dev,
 			   struct rte_flow *flow,
-			   uint64_t prefix_layers,
-			   uint32_t prefix_mark,
 			   const struct rte_flow_attr *attr,
 			   const struct rte_flow_item items[],
 			   const struct rte_flow_action actions[],
-			   bool external, uint32_t flow_idx,
+			   struct mlx5_flow_split_info *flow_split_info,
 			   struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
@@ -4937,10 +4922,8 @@ struct tunnel_default_miss_ctx {
 	if (!config->dv_flow_en ||
 	    config->dv_xmeta_en == MLX5_XMETA_MODE_LEGACY ||
 	    !mlx5_flow_ext_mreg_supported(dev))
-		return flow_create_split_inner(dev, flow, NULL, prefix_layers,
-					       prefix_mark, attr, items,
-					       actions, external, flow_idx,
-					       error);
+		return flow_create_split_inner(dev, flow, NULL, attr, items,
+					       actions, flow_split_info, error);
 	actions_n = flow_parse_metadata_split_actions_info(actions, &qrss,
 							   &encap_idx);
 	if (qrss) {
@@ -5025,10 +5008,9 @@ struct tunnel_default_miss_ctx {
 			goto exit;
 	}
 	/* Add the unmodified original or prefix subflow. */
-	ret = flow_create_split_inner(dev, flow, &dev_flow, prefix_layers,
-				      prefix_mark, attr,
+	ret = flow_create_split_inner(dev, flow, &dev_flow, attr,
 				      items, ext_actions ? ext_actions :
-				      actions, external, flow_idx, error);
+				      actions, flow_split_info, error);
 	if (ret < 0)
 		goto exit;
 	MLX5_ASSERT(dev_flow);
@@ -5089,10 +5071,12 @@ struct tunnel_default_miss_ctx {
 		}
 		dev_flow = NULL;
 		/* Add suffix subflow to execute Q/RSS. */
-		ret = flow_create_split_inner(dev, flow, &dev_flow, layers, 0,
+		flow_split_info->prefix_layers = layers;
+		flow_split_info->prefix_mark = 0;
+		ret = flow_create_split_inner(dev, flow, &dev_flow,
 					      &q_attr, mtr_sfx ? items :
 					      q_items, q_actions,
-					      external, flow_idx, error);
+					      flow_split_info, error);
 		if (ret < 0)
 			goto exit;
 		/* qrss ID should be freed if failed. */
@@ -5126,20 +5110,14 @@ struct tunnel_default_miss_ctx {
  *   Pointer to Ethernet device.
  * @param[in] flow
  *   Parent flow structure pointer.
- * @param[in] prefix_layers
- *   Prefix subflow layers, may be 0.
- * @param[in] prefix_mark
- *   Prefix subflow mark flag, may be 0.
  * @param[in] attr
  *   Flow rule attributes.
  * @param[in] items
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -5148,12 +5126,10 @@ struct tunnel_default_miss_ctx {
 static int
 flow_create_split_meter(struct rte_eth_dev *dev,
 			struct rte_flow *flow,
-			uint64_t prefix_layers,
-			uint32_t prefix_mark,
 			const struct rte_flow_attr *attr,
 			const struct rte_flow_item items[],
 			const struct rte_flow_action actions[],
-			bool external, uint32_t flow_idx,
+			struct mlx5_flow_split_info *flow_split_info,
 			struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
@@ -5197,11 +5173,10 @@ struct tunnel_default_miss_ctx {
 			goto exit;
 		}
 		/* Add the prefix subflow. */
+		flow_split_info->prefix_mark = 0;
 		ret = flow_create_split_inner(dev, flow, &dev_flow,
-					      prefix_layers, 0,
-					      attr, items,
-					      pre_actions, external,
-					      flow_idx, error);
+					      attr, items, pre_actions,
+					      flow_split_info, error);
 		if (ret) {
 			ret = -rte_errno;
 			goto exit;
@@ -5211,16 +5186,16 @@ struct tunnel_default_miss_ctx {
 		sfx_attr.group = sfx_attr.transfer ?
 				(MLX5_FLOW_TABLE_LEVEL_SUFFIX - 1) :
 				 MLX5_FLOW_TABLE_LEVEL_SUFFIX;
+		flow_split_info->prefix_layers =
+				flow_get_prefix_layer_flags(dev_flow);
+		flow_split_info->prefix_mark = dev_flow->handle->mark;
 	}
 	/* Add the prefix subflow. */
-	ret = flow_create_split_metadata(dev, flow, dev_flow ?
-					 flow_get_prefix_layer_flags(dev_flow) :
-					 prefix_layers, dev_flow ?
-					 dev_flow->handle->mark : prefix_mark,
+	ret = flow_create_split_metadata(dev, flow,
 					 &sfx_attr, sfx_items ?
 					 sfx_items : items,
 					 sfx_actions ? sfx_actions : actions,
-					 external, flow_idx, error);
+					 flow_split_info, error);
 exit:
 	if (sfx_actions)
 		mlx5_free(sfx_actions);
@@ -5252,10 +5227,8 @@ struct tunnel_default_miss_ctx {
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -5267,7 +5240,7 @@ struct tunnel_default_miss_ctx {
 			 const struct rte_flow_attr *attr,
 			 const struct rte_flow_item items[],
 			 const struct rte_flow_action actions[],
-			 bool external, uint32_t flow_idx,
+			 struct mlx5_flow_split_info *flow_split_info,
 			 struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
@@ -5324,9 +5297,9 @@ struct tunnel_default_miss_ctx {
 			goto exit;
 		}
 		/* Add the prefix subflow. */
-		ret = flow_create_split_inner(dev, flow, &dev_flow, 0, 0, attr,
-					      items, pre_actions, external,
-					      flow_idx, error);
+		ret = flow_create_split_inner(dev, flow, &dev_flow, attr,
+					      items, pre_actions,
+					      flow_split_info, error);
 		if (ret) {
 			ret = -rte_errno;
 			goto exit;
@@ -5344,15 +5317,20 @@ struct tunnel_default_miss_ctx {
 		sfx_attr.group = sfx_attr.transfer ?
 					(sfx_table_key.table_id - 1) :
 					 sfx_table_key.table_id;
+		flow_split_info->prefix_layers =
+				flow_get_prefix_layer_flags(dev_flow);
+		flow_split_info->prefix_mark = dev_flow->handle->mark;
+		/* Suffix group level already be scaled with factor, set
+		 * skip_scale to 1 to avoid scale again in translation.
+		 */
+		flow_split_info->skip_scale = 1;
 #endif
 	}
 	/* Add the suffix subflow. */
-	ret = flow_create_split_meter(dev, flow, dev_flow ?
-				 flow_get_prefix_layer_flags(dev_flow) : 0,
-				 dev_flow ? dev_flow->handle->mark : 0,
-				 &sfx_attr, sfx_items ? sfx_items : items,
-				 sfx_actions ? sfx_actions : actions,
-				 external, flow_idx, error);
+	ret = flow_create_split_meter(dev, flow, &sfx_attr,
+				      sfx_items ? sfx_items : items,
+				      sfx_actions ? sfx_actions : actions,
+				      flow_split_info, error);
 exit:
 	if (sfx_actions)
 		mlx5_free(sfx_actions);
@@ -5387,10 +5365,8 @@ struct tunnel_default_miss_ctx {
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -5402,13 +5378,13 @@ struct tunnel_default_miss_ctx {
 			const struct rte_flow_attr *attr,
 			const struct rte_flow_item items[],
 			const struct rte_flow_action actions[],
-			bool external, uint32_t flow_idx,
+			struct mlx5_flow_split_info *flow_split_info,
 			struct rte_flow_error *error)
 {
 	int ret;
 
 	ret = flow_create_split_sample(dev, flow, attr, items,
-				       actions, external, flow_idx, error);
+				       actions, flow_split_info, error);
 	MLX5_ASSERT(ret <= 0);
 	return ret;
 }
@@ -5527,11 +5503,17 @@ struct tunnel_default_miss_ctx {
 	uint32_t idx = 0;
 	int hairpin_flow;
 	struct rte_flow_attr attr_tx = { .priority = 0 };
-	struct rte_flow_attr attr_factor = {0};
 	const struct rte_flow_action *actions;
 	struct rte_flow_action *translated_actions = NULL;
 	struct mlx5_flow_tunnel *tunnel;
 	struct tunnel_default_miss_ctx default_miss_ctx = { 0, };
+	struct mlx5_flow_split_info flow_split_info = {
+		.external = !!external,
+		.skip_scale = 0,
+		.flow_idx = 0,
+		.prefix_mark = 0,
+		.prefix_layers = 0
+	};
 	struct mlx5_flow_workspace *wks = mlx5_flow_get_thread_workspace();
 	bool fidx = !!wks->flow_idx;
 	int ret;
@@ -5547,10 +5529,9 @@ struct tunnel_default_miss_ctx {
 		return 0;
 	}
 	actions = translated_actions ? translated_actions : original_actions;
-	memcpy((void *)&attr_factor, (const void *)attr, sizeof(*attr));
 	p_actions_rx = actions;
-	hairpin_flow = flow_check_hairpin_split(dev, &attr_factor, actions);
-	ret = flow_drv_validate(dev, &attr_factor, items, p_actions_rx,
+	hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
+	ret = flow_drv_validate(dev, attr, items, p_actions_rx,
 				external, hairpin_flow, error);
 	if (ret < 0)
 		goto error_before_hairpin_split;
@@ -5569,7 +5550,8 @@ struct tunnel_default_miss_ctx {
 				   idx);
 		p_actions_rx = actions_rx.actions;
 	}
-	flow->drv_type = flow_get_drv_type(dev, &attr_factor);
+	flow_split_info.flow_idx = idx;
+	flow->drv_type = flow_get_drv_type(dev, attr);
 	MLX5_ASSERT(flow->drv_type > MLX5_FLOW_TYPE_MIN &&
 		    flow->drv_type < MLX5_FLOW_TYPE_MAX);
 	memset(rss_desc, 0, offsetof(struct mlx5_flow_rss_desc, queue));
@@ -5616,9 +5598,9 @@ struct tunnel_default_miss_ctx {
 		 * depending on configuration. In the simplest
 		 * case it just creates unmodified original flow.
 		 */
-		ret = flow_create_split_outer(dev, flow, &attr_factor,
+		ret = flow_create_split_outer(dev, flow, attr,
 					      buf->entry[i].pattern,
-					      p_actions_rx, external, idx,
+					      p_actions_rx, &flow_split_info,
 					      error);
 		if (ret < 0)
 			goto error;
@@ -5666,8 +5648,8 @@ struct tunnel_default_miss_ctx {
 	 * the egress Flows belong to the different device and
 	 * copy table should be updated in peer NIC Rx domain.
 	 */
-	if (attr_factor.ingress &&
-	    (external || attr_factor.group != MLX5_FLOW_MREG_CP_TABLE_GROUP)) {
+	if (attr->ingress &&
+	    (external || attr->group != MLX5_FLOW_MREG_CP_TABLE_GROUP)) {
 		ret = flow_mreg_update_copy_table(dev, flow, actions, error);
 		if (ret)
 			goto error;
@@ -7551,7 +7533,8 @@ struct mlx5_meter_domains_infos *
 	int ret;
 	bool standard_translation;
 
-	if (grp_info.external && group < MLX5_MAX_TABLES_EXTERNAL)
+	if (!grp_info.skip_scale && grp_info.external &&
+	    group < MLX5_MAX_TABLES_EXTERNAL)
 		group *= MLX5_FLOW_TABLE_FACTOR;
 	if (is_tunnel_offload_active(dev)) {
 		standard_translation = !grp_info.external ||
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index 8ef2a85..bb671e7 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -754,6 +754,7 @@ struct mlx5_flow_verbs_workspace {
 #define MLX5_NUM_MAX_DEV_FLOWS 32
 
 /** Device flow structure. */
+__extension__
 struct mlx5_flow {
 	struct rte_flow *flow; /**< Pointer to the main flow. */
 	uint32_t flow_idx; /**< The memory pool index to the main flow. */
@@ -761,7 +762,9 @@ struct mlx5_flow {
 	uint64_t act_flags;
 	/**< Bit-fields of detected actions, see MLX5_FLOW_ACTION_*. */
 	bool external; /**< true if the flow is created external to PMD. */
-	uint8_t ingress; /**< 1 if the flow is ingress. */
+	uint8_t ingress:1; /**< 1 if the flow is ingress. */
+	uint8_t skip_scale:1;
+	/**< 1 if skip the scale the table with factor. */
 	union {
 #ifdef HAVE_IBV_FLOW_DV_SUPPORT
 		struct mlx5_flow_dv_workspace dv;
@@ -1102,6 +1105,15 @@ struct mlx5_flow_workspace {
 	int flow_nested_idx; /* Intermediate device flow index, nested. */
 };
 
+struct mlx5_flow_split_info {
+	bool external;
+	/**< True if flow is created by request external to PMD. */
+	uint8_t skip_scale; /**< Skip the scale the table with factor. */
+	uint32_t flow_idx; /**< This memory pool index to the flow. */
+	uint32_t prefix_mark; /**< Prefix subflow mark flag. */
+	uint64_t prefix_layers; /**< Prefix subflow layers. */
+};
+
 typedef int (*mlx5_flow_validate_t)(struct rte_eth_dev *dev,
 				    const struct rte_flow_attr *attr,
 				    const struct rte_flow_item items[],
@@ -1212,6 +1224,7 @@ struct flow_grp_info {
 	uint64_t fdb_def_rule:1;
 	/* force standard group translation */
 	uint64_t std_tbl_fix:1;
+	uint64_t skip_scale:1;
 };
 
 static inline bool
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 01b6e7c..69635a4 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -9315,6 +9315,7 @@ struct mlx5_cache_entry *
 		.external = !!dev_flow->external,
 		.transfer = !!attr->transfer,
 		.fdb_def_rule = !!priv->fdb_def_rule,
+		.skip_scale = !!dev_flow->skip_scale,
 	};
 
 	MLX5_ASSERT(wks);
@@ -9651,6 +9652,7 @@ struct mlx5_cache_entry *
 			jump_group = ((const struct rte_flow_action_jump *)
 							action->conf)->group;
 			grp_info.std_tbl_fix = 0;
+			grp_info.skip_scale = 0;
 			ret = mlx5_flow_group_to_table(dev, tunnel,
 						       jump_group,
 						       &table,
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index a733eda..43d81cc 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -2309,6 +2309,8 @@ int mlx5_hrxq_release(struct rte_eth_dev *dev, uint32_t hrxq_idx)
 	struct mlx5_hrxq *hrxq;
 
 	hrxq = mlx5_ipool_get(priv->sh->ipool[MLX5_IPOOL_HRXQ], hrxq_idx);
+	if (!hrxq)
+		return 0;
 	if (!hrxq->standalone)
 		return mlx5_cache_unregister(&priv->hrxqs, &hrxq->entry);
 	__mlx5_hrxq_remove(dev, hrxq);
-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v3] net/mlx5: fix incorrect group value of sample suffix flow
  2020-10-30  7:26   ` [dpdk-dev] [PATCH v2] net/mlx5: " Jiawei Wang
@ 2020-11-01  6:50     ` Jiawei Wang
  2020-11-01 16:41       ` Slava Ovsiienko
  2020-11-04 13:29       ` [dpdk-dev] [PATCH v4] " Jiawei Wang
  0 siblings, 2 replies; 7+ messages in thread
From: Jiawei Wang @ 2020-11-01  6:50 UTC (permalink / raw)
  To: viacheslavo, matan, orika; +Cc: dev, rasland

mlx5 PMD splited the sampling flow into prefix flow and suffix
flow. On the sample action translation function, the scaled
group value of suffix flow be attached into sample object and
saved into sample resource.

mlx5 PMD fetched the group value from the sample resource to
create the suffix flow. On the mlx5_flow_group_to_table
function the group value of suffix flow was scaled with table
factor again and translated into HW table. That caused the
incorrect group value of sample suffix flow.

The fix introduces a 'skip_scale' flag and sets it to 1 for the
sample suffix flow creation. On the mlx5_flow_group_to_table
function skips the scale with table factor to use the correct
group value.

Fixes: dc67aa6 ("net/mlx5: implement tunnel offload API")

Signed-off-by: Jiawei Wang <jiaweiw@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow.c    | 155 ++++++++++++++++++----------------------
 drivers/net/mlx5/mlx5_flow.h    |  15 +++-
 drivers/net/mlx5/mlx5_flow_dv.c |   2 +
 drivers/net/mlx5/mlx5_rxq.c     |   2 +
 4 files changed, 87 insertions(+), 87 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index a6e60af..dc7cb95 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -4288,20 +4288,14 @@ struct tunnel_default_miss_ctx {
  *   Parent flow structure pointer.
  * @param[in, out] sub_flow
  *   Pointer to return the created subflow, may be NULL.
- * @param[in] prefix_layers
- *   Prefix subflow layers, may be 0.
- * @param[in] prefix_mark
- *   Prefix subflow mark flag, may be 0.
  * @param[in] attr
  *   Flow rule attributes.
  * @param[in] items
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -4311,22 +4305,21 @@ struct tunnel_default_miss_ctx {
 flow_create_split_inner(struct rte_eth_dev *dev,
 			struct rte_flow *flow,
 			struct mlx5_flow **sub_flow,
-			uint64_t prefix_layers,
-			uint32_t prefix_mark,
 			const struct rte_flow_attr *attr,
 			const struct rte_flow_item items[],
 			const struct rte_flow_action actions[],
-			bool external, uint32_t flow_idx,
+			struct mlx5_flow_split_info *flow_split_info,
 			struct rte_flow_error *error)
 {
 	struct mlx5_flow *dev_flow;
 
 	dev_flow = flow_drv_prepare(dev, flow, attr, items, actions,
-		flow_idx, error);
+				    flow_split_info->flow_idx, error);
 	if (!dev_flow)
 		return -rte_errno;
 	dev_flow->flow = flow;
-	dev_flow->external = external;
+	dev_flow->external = flow_split_info->external;
+	dev_flow->skip_scale = flow_split_info->skip_scale;
 	/* Subflow object was created, we must include one in the list. */
 	SILIST_INSERT(&flow->dev_handles, dev_flow->handle_idx,
 		      dev_flow->handle, next);
@@ -4335,9 +4328,9 @@ struct tunnel_default_miss_ctx {
 	 * flow may need some user defined item layer flags, and pass the
 	 * Metadate rxq mark flag to suffix flow as well.
 	 */
-	if (prefix_layers)
-		dev_flow->handle->layers = prefix_layers;
-	if (prefix_mark)
+	if (flow_split_info->prefix_layers)
+		dev_flow->handle->layers = flow_split_info->prefix_layers;
+	if (flow_split_info->prefix_mark)
 		dev_flow->handle->mark = 1;
 	if (sub_flow)
 		*sub_flow = dev_flow;
@@ -4891,20 +4884,14 @@ struct tunnel_default_miss_ctx {
  *   Pointer to Ethernet device.
  * @param[in] flow
  *   Parent flow structure pointer.
- * @param[in] prefix_layers
- *   Prefix flow layer flags.
- * @param[in] prefix_mark
- *   Prefix subflow mark flag, may be 0.
  * @param[in] attr
  *   Flow rule attributes.
  * @param[in] items
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -4913,12 +4900,10 @@ struct tunnel_default_miss_ctx {
 static int
 flow_create_split_metadata(struct rte_eth_dev *dev,
 			   struct rte_flow *flow,
-			   uint64_t prefix_layers,
-			   uint32_t prefix_mark,
 			   const struct rte_flow_attr *attr,
 			   const struct rte_flow_item items[],
 			   const struct rte_flow_action actions[],
-			   bool external, uint32_t flow_idx,
+			   struct mlx5_flow_split_info *flow_split_info,
 			   struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
@@ -4937,10 +4922,8 @@ struct tunnel_default_miss_ctx {
 	if (!config->dv_flow_en ||
 	    config->dv_xmeta_en == MLX5_XMETA_MODE_LEGACY ||
 	    !mlx5_flow_ext_mreg_supported(dev))
-		return flow_create_split_inner(dev, flow, NULL, prefix_layers,
-					       prefix_mark, attr, items,
-					       actions, external, flow_idx,
-					       error);
+		return flow_create_split_inner(dev, flow, NULL, attr, items,
+					       actions, flow_split_info, error);
 	actions_n = flow_parse_metadata_split_actions_info(actions, &qrss,
 							   &encap_idx);
 	if (qrss) {
@@ -5025,10 +5008,9 @@ struct tunnel_default_miss_ctx {
 			goto exit;
 	}
 	/* Add the unmodified original or prefix subflow. */
-	ret = flow_create_split_inner(dev, flow, &dev_flow, prefix_layers,
-				      prefix_mark, attr,
+	ret = flow_create_split_inner(dev, flow, &dev_flow, attr,
 				      items, ext_actions ? ext_actions :
-				      actions, external, flow_idx, error);
+				      actions, flow_split_info, error);
 	if (ret < 0)
 		goto exit;
 	MLX5_ASSERT(dev_flow);
@@ -5089,10 +5071,12 @@ struct tunnel_default_miss_ctx {
 		}
 		dev_flow = NULL;
 		/* Add suffix subflow to execute Q/RSS. */
-		ret = flow_create_split_inner(dev, flow, &dev_flow, layers, 0,
+		flow_split_info->prefix_layers = layers;
+		flow_split_info->prefix_mark = 0;
+		ret = flow_create_split_inner(dev, flow, &dev_flow,
 					      &q_attr, mtr_sfx ? items :
 					      q_items, q_actions,
-					      external, flow_idx, error);
+					      flow_split_info, error);
 		if (ret < 0)
 			goto exit;
 		/* qrss ID should be freed if failed. */
@@ -5126,20 +5110,14 @@ struct tunnel_default_miss_ctx {
  *   Pointer to Ethernet device.
  * @param[in] flow
  *   Parent flow structure pointer.
- * @param[in] prefix_layers
- *   Prefix subflow layers, may be 0.
- * @param[in] prefix_mark
- *   Prefix subflow mark flag, may be 0.
  * @param[in] attr
  *   Flow rule attributes.
  * @param[in] items
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -5148,12 +5126,10 @@ struct tunnel_default_miss_ctx {
 static int
 flow_create_split_meter(struct rte_eth_dev *dev,
 			struct rte_flow *flow,
-			uint64_t prefix_layers,
-			uint32_t prefix_mark,
 			const struct rte_flow_attr *attr,
 			const struct rte_flow_item items[],
 			const struct rte_flow_action actions[],
-			bool external, uint32_t flow_idx,
+			struct mlx5_flow_split_info *flow_split_info,
 			struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
@@ -5197,11 +5173,10 @@ struct tunnel_default_miss_ctx {
 			goto exit;
 		}
 		/* Add the prefix subflow. */
+		flow_split_info->prefix_mark = 0;
 		ret = flow_create_split_inner(dev, flow, &dev_flow,
-					      prefix_layers, 0,
-					      attr, items,
-					      pre_actions, external,
-					      flow_idx, error);
+					      attr, items, pre_actions,
+					      flow_split_info, error);
 		if (ret) {
 			ret = -rte_errno;
 			goto exit;
@@ -5211,16 +5186,16 @@ struct tunnel_default_miss_ctx {
 		sfx_attr.group = sfx_attr.transfer ?
 				(MLX5_FLOW_TABLE_LEVEL_SUFFIX - 1) :
 				 MLX5_FLOW_TABLE_LEVEL_SUFFIX;
+		flow_split_info->prefix_layers =
+				flow_get_prefix_layer_flags(dev_flow);
+		flow_split_info->prefix_mark = dev_flow->handle->mark;
 	}
 	/* Add the prefix subflow. */
-	ret = flow_create_split_metadata(dev, flow, dev_flow ?
-					 flow_get_prefix_layer_flags(dev_flow) :
-					 prefix_layers, dev_flow ?
-					 dev_flow->handle->mark : prefix_mark,
+	ret = flow_create_split_metadata(dev, flow,
 					 &sfx_attr, sfx_items ?
 					 sfx_items : items,
 					 sfx_actions ? sfx_actions : actions,
-					 external, flow_idx, error);
+					 flow_split_info, error);
 exit:
 	if (sfx_actions)
 		mlx5_free(sfx_actions);
@@ -5252,10 +5227,8 @@ struct tunnel_default_miss_ctx {
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -5267,7 +5240,7 @@ struct tunnel_default_miss_ctx {
 			 const struct rte_flow_attr *attr,
 			 const struct rte_flow_item items[],
 			 const struct rte_flow_action actions[],
-			 bool external, uint32_t flow_idx,
+			 struct mlx5_flow_split_info *flow_split_info,
 			 struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
@@ -5324,9 +5297,9 @@ struct tunnel_default_miss_ctx {
 			goto exit;
 		}
 		/* Add the prefix subflow. */
-		ret = flow_create_split_inner(dev, flow, &dev_flow, 0, 0, attr,
-					      items, pre_actions, external,
-					      flow_idx, error);
+		ret = flow_create_split_inner(dev, flow, &dev_flow, attr,
+					      items, pre_actions,
+					      flow_split_info, error);
 		if (ret) {
 			ret = -rte_errno;
 			goto exit;
@@ -5344,15 +5317,20 @@ struct tunnel_default_miss_ctx {
 		sfx_attr.group = sfx_attr.transfer ?
 					(sfx_table_key.table_id - 1) :
 					 sfx_table_key.table_id;
+		flow_split_info->prefix_layers =
+				flow_get_prefix_layer_flags(dev_flow);
+		flow_split_info->prefix_mark = dev_flow->handle->mark;
+		/* Suffix group level already be scaled with factor, set
+		 * skip_scale to 1 to avoid scale again in translation.
+		 */
+		flow_split_info->skip_scale = 1;
 #endif
 	}
 	/* Add the suffix subflow. */
-	ret = flow_create_split_meter(dev, flow, dev_flow ?
-				 flow_get_prefix_layer_flags(dev_flow) : 0,
-				 dev_flow ? dev_flow->handle->mark : 0,
-				 &sfx_attr, sfx_items ? sfx_items : items,
-				 sfx_actions ? sfx_actions : actions,
-				 external, flow_idx, error);
+	ret = flow_create_split_meter(dev, flow, &sfx_attr,
+				      sfx_items ? sfx_items : items,
+				      sfx_actions ? sfx_actions : actions,
+				      flow_split_info, error);
 exit:
 	if (sfx_actions)
 		mlx5_free(sfx_actions);
@@ -5387,10 +5365,8 @@ struct tunnel_default_miss_ctx {
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -5402,13 +5378,13 @@ struct tunnel_default_miss_ctx {
 			const struct rte_flow_attr *attr,
 			const struct rte_flow_item items[],
 			const struct rte_flow_action actions[],
-			bool external, uint32_t flow_idx,
+			struct mlx5_flow_split_info *flow_split_info,
 			struct rte_flow_error *error)
 {
 	int ret;
 
 	ret = flow_create_split_sample(dev, flow, attr, items,
-				       actions, external, flow_idx, error);
+				       actions, flow_split_info, error);
 	MLX5_ASSERT(ret <= 0);
 	return ret;
 }
@@ -5527,11 +5503,17 @@ struct tunnel_default_miss_ctx {
 	uint32_t idx = 0;
 	int hairpin_flow;
 	struct rte_flow_attr attr_tx = { .priority = 0 };
-	struct rte_flow_attr attr_factor = {0};
 	const struct rte_flow_action *actions;
 	struct rte_flow_action *translated_actions = NULL;
 	struct mlx5_flow_tunnel *tunnel;
 	struct tunnel_default_miss_ctx default_miss_ctx = { 0, };
+	struct mlx5_flow_split_info flow_split_info = {
+		.external = !!external,
+		.skip_scale = 0,
+		.flow_idx = 0,
+		.prefix_mark = 0,
+		.prefix_layers = 0
+	};
 	struct mlx5_flow_workspace *wks = mlx5_flow_get_thread_workspace();
 	bool fidx = !!wks->flow_idx;
 	int ret;
@@ -5547,10 +5529,9 @@ struct tunnel_default_miss_ctx {
 		return 0;
 	}
 	actions = translated_actions ? translated_actions : original_actions;
-	memcpy((void *)&attr_factor, (const void *)attr, sizeof(*attr));
 	p_actions_rx = actions;
-	hairpin_flow = flow_check_hairpin_split(dev, &attr_factor, actions);
-	ret = flow_drv_validate(dev, &attr_factor, items, p_actions_rx,
+	hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
+	ret = flow_drv_validate(dev, attr, items, p_actions_rx,
 				external, hairpin_flow, error);
 	if (ret < 0)
 		goto error_before_hairpin_split;
@@ -5569,7 +5550,8 @@ struct tunnel_default_miss_ctx {
 				   idx);
 		p_actions_rx = actions_rx.actions;
 	}
-	flow->drv_type = flow_get_drv_type(dev, &attr_factor);
+	flow_split_info.flow_idx = idx;
+	flow->drv_type = flow_get_drv_type(dev, attr);
 	MLX5_ASSERT(flow->drv_type > MLX5_FLOW_TYPE_MIN &&
 		    flow->drv_type < MLX5_FLOW_TYPE_MAX);
 	memset(rss_desc, 0, offsetof(struct mlx5_flow_rss_desc, queue));
@@ -5616,9 +5598,9 @@ struct tunnel_default_miss_ctx {
 		 * depending on configuration. In the simplest
 		 * case it just creates unmodified original flow.
 		 */
-		ret = flow_create_split_outer(dev, flow, &attr_factor,
+		ret = flow_create_split_outer(dev, flow, attr,
 					      buf->entry[i].pattern,
-					      p_actions_rx, external, idx,
+					      p_actions_rx, &flow_split_info,
 					      error);
 		if (ret < 0)
 			goto error;
@@ -5666,8 +5648,8 @@ struct tunnel_default_miss_ctx {
 	 * the egress Flows belong to the different device and
 	 * copy table should be updated in peer NIC Rx domain.
 	 */
-	if (attr_factor.ingress &&
-	    (external || attr_factor.group != MLX5_FLOW_MREG_CP_TABLE_GROUP)) {
+	if (attr->ingress &&
+	    (external || attr->group != MLX5_FLOW_MREG_CP_TABLE_GROUP)) {
 		ret = flow_mreg_update_copy_table(dev, flow, actions, error);
 		if (ret)
 			goto error;
@@ -7551,7 +7533,8 @@ struct mlx5_meter_domains_infos *
 	int ret;
 	bool standard_translation;
 
-	if (grp_info.external && group < MLX5_MAX_TABLES_EXTERNAL)
+	if (!grp_info.skip_scale && grp_info.external &&
+	    group < MLX5_MAX_TABLES_EXTERNAL)
 		group *= MLX5_FLOW_TABLE_FACTOR;
 	if (is_tunnel_offload_active(dev)) {
 		standard_translation = !grp_info.external ||
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index 8ef2a85..bb671e7 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -754,6 +754,7 @@ struct mlx5_flow_verbs_workspace {
 #define MLX5_NUM_MAX_DEV_FLOWS 32
 
 /** Device flow structure. */
+__extension__
 struct mlx5_flow {
 	struct rte_flow *flow; /**< Pointer to the main flow. */
 	uint32_t flow_idx; /**< The memory pool index to the main flow. */
@@ -761,7 +762,9 @@ struct mlx5_flow {
 	uint64_t act_flags;
 	/**< Bit-fields of detected actions, see MLX5_FLOW_ACTION_*. */
 	bool external; /**< true if the flow is created external to PMD. */
-	uint8_t ingress; /**< 1 if the flow is ingress. */
+	uint8_t ingress:1; /**< 1 if the flow is ingress. */
+	uint8_t skip_scale:1;
+	/**< 1 if skip the scale the table with factor. */
 	union {
 #ifdef HAVE_IBV_FLOW_DV_SUPPORT
 		struct mlx5_flow_dv_workspace dv;
@@ -1102,6 +1105,15 @@ struct mlx5_flow_workspace {
 	int flow_nested_idx; /* Intermediate device flow index, nested. */
 };
 
+struct mlx5_flow_split_info {
+	bool external;
+	/**< True if flow is created by request external to PMD. */
+	uint8_t skip_scale; /**< Skip the scale the table with factor. */
+	uint32_t flow_idx; /**< This memory pool index to the flow. */
+	uint32_t prefix_mark; /**< Prefix subflow mark flag. */
+	uint64_t prefix_layers; /**< Prefix subflow layers. */
+};
+
 typedef int (*mlx5_flow_validate_t)(struct rte_eth_dev *dev,
 				    const struct rte_flow_attr *attr,
 				    const struct rte_flow_item items[],
@@ -1212,6 +1224,7 @@ struct flow_grp_info {
 	uint64_t fdb_def_rule:1;
 	/* force standard group translation */
 	uint64_t std_tbl_fix:1;
+	uint64_t skip_scale:1;
 };
 
 static inline bool
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 01b6e7c..69635a4 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -9315,6 +9315,7 @@ struct mlx5_cache_entry *
 		.external = !!dev_flow->external,
 		.transfer = !!attr->transfer,
 		.fdb_def_rule = !!priv->fdb_def_rule,
+		.skip_scale = !!dev_flow->skip_scale,
 	};
 
 	MLX5_ASSERT(wks);
@@ -9651,6 +9652,7 @@ struct mlx5_cache_entry *
 			jump_group = ((const struct rte_flow_action_jump *)
 							action->conf)->group;
 			grp_info.std_tbl_fix = 0;
+			grp_info.skip_scale = 0;
 			ret = mlx5_flow_group_to_table(dev, tunnel,
 						       jump_group,
 						       &table,
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index a733eda..43d81cc 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -2309,6 +2309,8 @@ int mlx5_hrxq_release(struct rte_eth_dev *dev, uint32_t hrxq_idx)
 	struct mlx5_hrxq *hrxq;
 
 	hrxq = mlx5_ipool_get(priv->sh->ipool[MLX5_IPOOL_HRXQ], hrxq_idx);
+	if (!hrxq)
+		return 0;
 	if (!hrxq->standalone)
 		return mlx5_cache_unregister(&priv->hrxqs, &hrxq->entry);
 	__mlx5_hrxq_remove(dev, hrxq);
-- 
1.8.3.1


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

* Re: [dpdk-dev] [PATCH v3] net/mlx5: fix incorrect group value of sample suffix flow
  2020-11-01  6:50     ` [dpdk-dev] [PATCH v3] " Jiawei Wang
@ 2020-11-01 16:41       ` Slava Ovsiienko
  2020-11-04 13:29       ` [dpdk-dev] [PATCH v4] " Jiawei Wang
  1 sibling, 0 replies; 7+ messages in thread
From: Slava Ovsiienko @ 2020-11-01 16:41 UTC (permalink / raw)
  To: Jiawei(Jonny) Wang, Matan Azrad, Ori Kam; +Cc: dev, Raslan Darawsheh

> -----Original Message-----
> From: Jiawei Wang <jiaweiw@nvidia.com>
> Sent: Sunday, November 1, 2020 8:50
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; Ori Kam <orika@nvidia.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>
> Subject: [PATCH v3] net/mlx5: fix incorrect group value of sample suffix flow
> 
> mlx5 PMD splited the sampling flow into prefix flow and suffix flow. On the
> sample action translation function, the scaled group value of suffix flow be
> attached into sample object and saved into sample resource.
> 
> mlx5 PMD fetched the group value from the sample resource to create the
> suffix flow. On the mlx5_flow_group_to_table function the group value of
> suffix flow was scaled with table factor again and translated into HW table.
> That caused the incorrect group value of sample suffix flow.
> 
> The fix introduces a 'skip_scale' flag and sets it to 1 for the sample suffix flow
> creation. On the mlx5_flow_group_to_table function skips the scale with table
> factor to use the correct group value.
> 
> Fixes: dc67aa6 ("net/mlx5: implement tunnel offload API")
> 
> Signed-off-by: Jiawei Wang <jiaweiw@nvidia.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>


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

* [dpdk-dev] [PATCH v4] net/mlx5: fix incorrect group value of sample suffix flow
  2020-11-01  6:50     ` [dpdk-dev] [PATCH v3] " Jiawei Wang
  2020-11-01 16:41       ` Slava Ovsiienko
@ 2020-11-04 13:29       ` Jiawei Wang
  2020-11-08 11:13         ` Raslan Darawsheh
  1 sibling, 1 reply; 7+ messages in thread
From: Jiawei Wang @ 2020-11-04 13:29 UTC (permalink / raw)
  To: viacheslavo, matan, orika; +Cc: dev, rasland

mlx5 PMD splited the sampling flow into prefix flow and suffix
flow. On the sample action translation function, the scaled
group value of suffix flow be attached into sample object and
saved into sample resource.

mlx5 PMD fetched the group value from the sample resource to
create the suffix flow. On the mlx5_flow_group_to_table
function the group value of suffix flow was scaled with table
factor again and translated into HW table. That caused the
incorrect group value of sample suffix flow.

The fix introduces a 'skip_scale' flag and sets it to 1 for the
sample suffix flow creation. On the mlx5_flow_group_to_table
function skips the scale with table factor to use the correct
group value.

Fixes: b0c8d823552a ("net/mlx5: implement tunnel offload")

Signed-off-by: Jiawei Wang <jiaweiw@nvidia.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
v4:
* Init the flow split data for each dev_flow
* Rebase

v3:
* Rebase

v2:
* Update commit message
* Rebase

---
 drivers/net/mlx5/mlx5_flow.c    | 159 ++++++++++++++++++----------------------
 drivers/net/mlx5/mlx5_flow.h    |  15 +++-
 drivers/net/mlx5/mlx5_flow_dv.c |   2 +
 drivers/net/mlx5/mlx5_rxq.c     |   2 +
 4 files changed, 91 insertions(+), 87 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index f9420e7..658a9e7 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -4304,20 +4304,14 @@ struct tunnel_default_miss_ctx {
  *   Parent flow structure pointer.
  * @param[in, out] sub_flow
  *   Pointer to return the created subflow, may be NULL.
- * @param[in] prefix_layers
- *   Prefix subflow layers, may be 0.
- * @param[in] prefix_mark
- *   Prefix subflow mark flag, may be 0.
  * @param[in] attr
  *   Flow rule attributes.
  * @param[in] items
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -4327,22 +4321,21 @@ struct tunnel_default_miss_ctx {
 flow_create_split_inner(struct rte_eth_dev *dev,
 			struct rte_flow *flow,
 			struct mlx5_flow **sub_flow,
-			uint64_t prefix_layers,
-			uint32_t prefix_mark,
 			const struct rte_flow_attr *attr,
 			const struct rte_flow_item items[],
 			const struct rte_flow_action actions[],
-			bool external, uint32_t flow_idx,
+			struct mlx5_flow_split_info *flow_split_info,
 			struct rte_flow_error *error)
 {
 	struct mlx5_flow *dev_flow;
 
 	dev_flow = flow_drv_prepare(dev, flow, attr, items, actions,
-		flow_idx, error);
+				    flow_split_info->flow_idx, error);
 	if (!dev_flow)
 		return -rte_errno;
 	dev_flow->flow = flow;
-	dev_flow->external = external;
+	dev_flow->external = flow_split_info->external;
+	dev_flow->skip_scale = flow_split_info->skip_scale;
 	/* Subflow object was created, we must include one in the list. */
 	SILIST_INSERT(&flow->dev_handles, dev_flow->handle_idx,
 		      dev_flow->handle, next);
@@ -4351,9 +4344,9 @@ struct tunnel_default_miss_ctx {
 	 * flow may need some user defined item layer flags, and pass the
 	 * Metadate rxq mark flag to suffix flow as well.
 	 */
-	if (prefix_layers)
-		dev_flow->handle->layers = prefix_layers;
-	if (prefix_mark)
+	if (flow_split_info->prefix_layers)
+		dev_flow->handle->layers = flow_split_info->prefix_layers;
+	if (flow_split_info->prefix_mark)
 		dev_flow->handle->mark = 1;
 	if (sub_flow)
 		*sub_flow = dev_flow;
@@ -4907,20 +4900,14 @@ struct tunnel_default_miss_ctx {
  *   Pointer to Ethernet device.
  * @param[in] flow
  *   Parent flow structure pointer.
- * @param[in] prefix_layers
- *   Prefix flow layer flags.
- * @param[in] prefix_mark
- *   Prefix subflow mark flag, may be 0.
  * @param[in] attr
  *   Flow rule attributes.
  * @param[in] items
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -4929,12 +4916,10 @@ struct tunnel_default_miss_ctx {
 static int
 flow_create_split_metadata(struct rte_eth_dev *dev,
 			   struct rte_flow *flow,
-			   uint64_t prefix_layers,
-			   uint32_t prefix_mark,
 			   const struct rte_flow_attr *attr,
 			   const struct rte_flow_item items[],
 			   const struct rte_flow_action actions[],
-			   bool external, uint32_t flow_idx,
+			   struct mlx5_flow_split_info *flow_split_info,
 			   struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
@@ -4953,10 +4938,8 @@ struct tunnel_default_miss_ctx {
 	if (!config->dv_flow_en ||
 	    config->dv_xmeta_en == MLX5_XMETA_MODE_LEGACY ||
 	    !mlx5_flow_ext_mreg_supported(dev))
-		return flow_create_split_inner(dev, flow, NULL, prefix_layers,
-					       prefix_mark, attr, items,
-					       actions, external, flow_idx,
-					       error);
+		return flow_create_split_inner(dev, flow, NULL, attr, items,
+					       actions, flow_split_info, error);
 	actions_n = flow_parse_metadata_split_actions_info(actions, &qrss,
 							   &encap_idx);
 	if (qrss) {
@@ -5041,10 +5024,9 @@ struct tunnel_default_miss_ctx {
 			goto exit;
 	}
 	/* Add the unmodified original or prefix subflow. */
-	ret = flow_create_split_inner(dev, flow, &dev_flow, prefix_layers,
-				      prefix_mark, attr,
+	ret = flow_create_split_inner(dev, flow, &dev_flow, attr,
 				      items, ext_actions ? ext_actions :
-				      actions, external, flow_idx, error);
+				      actions, flow_split_info, error);
 	if (ret < 0)
 		goto exit;
 	MLX5_ASSERT(dev_flow);
@@ -5105,10 +5087,12 @@ struct tunnel_default_miss_ctx {
 		}
 		dev_flow = NULL;
 		/* Add suffix subflow to execute Q/RSS. */
-		ret = flow_create_split_inner(dev, flow, &dev_flow, layers, 0,
+		flow_split_info->prefix_layers = layers;
+		flow_split_info->prefix_mark = 0;
+		ret = flow_create_split_inner(dev, flow, &dev_flow,
 					      &q_attr, mtr_sfx ? items :
 					      q_items, q_actions,
-					      external, flow_idx, error);
+					      flow_split_info, error);
 		if (ret < 0)
 			goto exit;
 		/* qrss ID should be freed if failed. */
@@ -5142,20 +5126,14 @@ struct tunnel_default_miss_ctx {
  *   Pointer to Ethernet device.
  * @param[in] flow
  *   Parent flow structure pointer.
- * @param[in] prefix_layers
- *   Prefix subflow layers, may be 0.
- * @param[in] prefix_mark
- *   Prefix subflow mark flag, may be 0.
  * @param[in] attr
  *   Flow rule attributes.
  * @param[in] items
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -5164,12 +5142,10 @@ struct tunnel_default_miss_ctx {
 static int
 flow_create_split_meter(struct rte_eth_dev *dev,
 			struct rte_flow *flow,
-			uint64_t prefix_layers,
-			uint32_t prefix_mark,
 			const struct rte_flow_attr *attr,
 			const struct rte_flow_item items[],
 			const struct rte_flow_action actions[],
-			bool external, uint32_t flow_idx,
+			struct mlx5_flow_split_info *flow_split_info,
 			struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
@@ -5213,11 +5189,10 @@ struct tunnel_default_miss_ctx {
 			goto exit;
 		}
 		/* Add the prefix subflow. */
+		flow_split_info->prefix_mark = 0;
 		ret = flow_create_split_inner(dev, flow, &dev_flow,
-					      prefix_layers, 0,
-					      attr, items,
-					      pre_actions, external,
-					      flow_idx, error);
+					      attr, items, pre_actions,
+					      flow_split_info, error);
 		if (ret) {
 			ret = -rte_errno;
 			goto exit;
@@ -5227,16 +5202,16 @@ struct tunnel_default_miss_ctx {
 		sfx_attr.group = sfx_attr.transfer ?
 				(MLX5_FLOW_TABLE_LEVEL_SUFFIX - 1) :
 				 MLX5_FLOW_TABLE_LEVEL_SUFFIX;
+		flow_split_info->prefix_layers =
+				flow_get_prefix_layer_flags(dev_flow);
+		flow_split_info->prefix_mark = dev_flow->handle->mark;
 	}
 	/* Add the prefix subflow. */
-	ret = flow_create_split_metadata(dev, flow, dev_flow ?
-					 flow_get_prefix_layer_flags(dev_flow) :
-					 prefix_layers, dev_flow ?
-					 dev_flow->handle->mark : prefix_mark,
+	ret = flow_create_split_metadata(dev, flow,
 					 &sfx_attr, sfx_items ?
 					 sfx_items : items,
 					 sfx_actions ? sfx_actions : actions,
-					 external, flow_idx, error);
+					 flow_split_info, error);
 exit:
 	if (sfx_actions)
 		mlx5_free(sfx_actions);
@@ -5268,10 +5243,8 @@ struct tunnel_default_miss_ctx {
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -5283,7 +5256,7 @@ struct tunnel_default_miss_ctx {
 			 const struct rte_flow_attr *attr,
 			 const struct rte_flow_item items[],
 			 const struct rte_flow_action actions[],
-			 bool external, uint32_t flow_idx,
+			 struct mlx5_flow_split_info *flow_split_info,
 			 struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
@@ -5340,9 +5313,9 @@ struct tunnel_default_miss_ctx {
 			goto exit;
 		}
 		/* Add the prefix subflow. */
-		ret = flow_create_split_inner(dev, flow, &dev_flow, 0, 0, attr,
-					      items, pre_actions, external,
-					      flow_idx, error);
+		ret = flow_create_split_inner(dev, flow, &dev_flow, attr,
+					      items, pre_actions,
+					      flow_split_info, error);
 		if (ret) {
 			ret = -rte_errno;
 			goto exit;
@@ -5360,15 +5333,20 @@ struct tunnel_default_miss_ctx {
 		sfx_attr.group = sfx_attr.transfer ?
 					(sfx_table_key.table_id - 1) :
 					 sfx_table_key.table_id;
+		flow_split_info->prefix_layers =
+				flow_get_prefix_layer_flags(dev_flow);
+		flow_split_info->prefix_mark = dev_flow->handle->mark;
+		/* Suffix group level already be scaled with factor, set
+		 * skip_scale to 1 to avoid scale again in translation.
+		 */
+		flow_split_info->skip_scale = 1;
 #endif
 	}
 	/* Add the suffix subflow. */
-	ret = flow_create_split_meter(dev, flow, dev_flow ?
-				 flow_get_prefix_layer_flags(dev_flow) : 0,
-				 dev_flow ? dev_flow->handle->mark : 0,
-				 &sfx_attr, sfx_items ? sfx_items : items,
-				 sfx_actions ? sfx_actions : actions,
-				 external, flow_idx, error);
+	ret = flow_create_split_meter(dev, flow, &sfx_attr,
+				      sfx_items ? sfx_items : items,
+				      sfx_actions ? sfx_actions : actions,
+				      flow_split_info, error);
 exit:
 	if (sfx_actions)
 		mlx5_free(sfx_actions);
@@ -5403,10 +5381,8 @@ struct tunnel_default_miss_ctx {
  *   Pattern specification (list terminated by the END pattern item).
  * @param[in] actions
  *   Associated actions (list terminated by the END action).
- * @param[in] external
- *   This flow rule is created by request external to PMD.
- * @param[in] flow_idx
- *   This memory pool index to the flow.
+ * @param[in] flow_split_info
+ *   Pointer to flow split info structure.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  * @return
@@ -5418,13 +5394,13 @@ struct tunnel_default_miss_ctx {
 			const struct rte_flow_attr *attr,
 			const struct rte_flow_item items[],
 			const struct rte_flow_action actions[],
-			bool external, uint32_t flow_idx,
+			struct mlx5_flow_split_info *flow_split_info,
 			struct rte_flow_error *error)
 {
 	int ret;
 
 	ret = flow_create_split_sample(dev, flow, attr, items,
-				       actions, external, flow_idx, error);
+				       actions, flow_split_info, error);
 	MLX5_ASSERT(ret <= 0);
 	return ret;
 }
@@ -5543,11 +5519,17 @@ struct tunnel_default_miss_ctx {
 	uint32_t idx = 0;
 	int hairpin_flow;
 	struct rte_flow_attr attr_tx = { .priority = 0 };
-	struct rte_flow_attr attr_factor = {0};
 	const struct rte_flow_action *actions;
 	struct rte_flow_action *translated_actions = NULL;
 	struct mlx5_flow_tunnel *tunnel;
 	struct tunnel_default_miss_ctx default_miss_ctx = { 0, };
+	struct mlx5_flow_split_info flow_split_info = {
+		.external = !!external,
+		.skip_scale = 0,
+		.flow_idx = 0,
+		.prefix_mark = 0,
+		.prefix_layers = 0
+	};
 	struct mlx5_flow_workspace *wks = mlx5_flow_get_thread_workspace();
 	bool fidx = !!wks->flow_idx;
 	int ret;
@@ -5563,10 +5545,9 @@ struct tunnel_default_miss_ctx {
 		return 0;
 	}
 	actions = translated_actions ? translated_actions : original_actions;
-	memcpy((void *)&attr_factor, (const void *)attr, sizeof(*attr));
 	p_actions_rx = actions;
-	hairpin_flow = flow_check_hairpin_split(dev, &attr_factor, actions);
-	ret = flow_drv_validate(dev, &attr_factor, items, p_actions_rx,
+	hairpin_flow = flow_check_hairpin_split(dev, attr, actions);
+	ret = flow_drv_validate(dev, attr, items, p_actions_rx,
 				external, hairpin_flow, error);
 	if (ret < 0)
 		goto error_before_hairpin_split;
@@ -5585,7 +5566,8 @@ struct tunnel_default_miss_ctx {
 				   idx);
 		p_actions_rx = actions_rx.actions;
 	}
-	flow->drv_type = flow_get_drv_type(dev, &attr_factor);
+	flow_split_info.flow_idx = idx;
+	flow->drv_type = flow_get_drv_type(dev, attr);
 	MLX5_ASSERT(flow->drv_type > MLX5_FLOW_TYPE_MIN &&
 		    flow->drv_type < MLX5_FLOW_TYPE_MAX);
 	memset(rss_desc, 0, offsetof(struct mlx5_flow_rss_desc, queue));
@@ -5627,14 +5609,18 @@ struct tunnel_default_miss_ctx {
 		wks->flow_nested_idx = fidx;
 	}
 	for (i = 0; i < buf->entries; ++i) {
+		/* Initialize flow split data. */
+		flow_split_info.prefix_layers = 0;
+		flow_split_info.prefix_mark = 0;
+		flow_split_info.skip_scale = 0;
 		/*
 		 * The splitter may create multiple dev_flows,
 		 * depending on configuration. In the simplest
 		 * case it just creates unmodified original flow.
 		 */
-		ret = flow_create_split_outer(dev, flow, &attr_factor,
+		ret = flow_create_split_outer(dev, flow, attr,
 					      buf->entry[i].pattern,
-					      p_actions_rx, external, idx,
+					      p_actions_rx, &flow_split_info,
 					      error);
 		if (ret < 0)
 			goto error;
@@ -5682,8 +5668,8 @@ struct tunnel_default_miss_ctx {
 	 * the egress Flows belong to the different device and
 	 * copy table should be updated in peer NIC Rx domain.
 	 */
-	if (attr_factor.ingress &&
-	    (external || attr_factor.group != MLX5_FLOW_MREG_CP_TABLE_GROUP)) {
+	if (attr->ingress &&
+	    (external || attr->group != MLX5_FLOW_MREG_CP_TABLE_GROUP)) {
 		ret = flow_mreg_update_copy_table(dev, flow, actions, error);
 		if (ret)
 			goto error;
@@ -7068,7 +7054,8 @@ struct mlx5_meter_domains_infos *
 	int ret;
 	bool standard_translation;
 
-	if (grp_info.external && group < MLX5_MAX_TABLES_EXTERNAL)
+	if (!grp_info.skip_scale && grp_info.external &&
+	    group < MLX5_MAX_TABLES_EXTERNAL)
 		group *= MLX5_FLOW_TABLE_FACTOR;
 	if (is_tunnel_offload_active(dev)) {
 		standard_translation = !grp_info.external ||
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index 58185fb..9a0268a 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -761,6 +761,7 @@ struct mlx5_flow_verbs_workspace {
 #define MLX5_NUM_MAX_DEV_FLOWS 32
 
 /** Device flow structure. */
+__extension__
 struct mlx5_flow {
 	struct rte_flow *flow; /**< Pointer to the main flow. */
 	uint32_t flow_idx; /**< The memory pool index to the main flow. */
@@ -768,7 +769,9 @@ struct mlx5_flow {
 	uint64_t act_flags;
 	/**< Bit-fields of detected actions, see MLX5_FLOW_ACTION_*. */
 	bool external; /**< true if the flow is created external to PMD. */
-	uint8_t ingress; /**< 1 if the flow is ingress. */
+	uint8_t ingress:1; /**< 1 if the flow is ingress. */
+	uint8_t skip_scale:1;
+	/**< 1 if skip the scale the table with factor. */
 	union {
 #ifdef HAVE_IBV_FLOW_DV_SUPPORT
 		struct mlx5_flow_dv_workspace dv;
@@ -1094,6 +1097,15 @@ struct mlx5_flow_workspace {
 	int flow_nested_idx; /* Intermediate device flow index, nested. */
 };
 
+struct mlx5_flow_split_info {
+	bool external;
+	/**< True if flow is created by request external to PMD. */
+	uint8_t skip_scale; /**< Skip the scale the table with factor. */
+	uint32_t flow_idx; /**< This memory pool index to the flow. */
+	uint32_t prefix_mark; /**< Prefix subflow mark flag. */
+	uint64_t prefix_layers; /**< Prefix subflow layers. */
+};
+
 typedef int (*mlx5_flow_validate_t)(struct rte_eth_dev *dev,
 				    const struct rte_flow_attr *attr,
 				    const struct rte_flow_item items[],
@@ -1211,6 +1223,7 @@ struct flow_grp_info {
 	uint64_t fdb_def_rule:1;
 	/* force standard group translation */
 	uint64_t std_tbl_fix:1;
+	uint64_t skip_scale:1;
 };
 
 static inline bool
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index d7641e9..01ba7c6 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -9566,6 +9566,7 @@ struct mlx5_cache_entry *
 		.external = !!dev_flow->external,
 		.transfer = !!attr->transfer,
 		.fdb_def_rule = !!priv->fdb_def_rule,
+		.skip_scale = !!dev_flow->skip_scale,
 	};
 
 	MLX5_ASSERT(wks);
@@ -9927,6 +9928,7 @@ struct mlx5_cache_entry *
 			jump_group = ((const struct rte_flow_action_jump *)
 							action->conf)->group;
 			grp_info.std_tbl_fix = 0;
+			grp_info.skip_scale = 0;
 			ret = mlx5_flow_group_to_table(dev, tunnel,
 						       jump_group,
 						       &table,
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 4d3bd00..ce36b7e 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -2317,6 +2317,8 @@ int mlx5_hrxq_release(struct rte_eth_dev *dev, uint32_t hrxq_idx)
 	struct mlx5_hrxq *hrxq;
 
 	hrxq = mlx5_ipool_get(priv->sh->ipool[MLX5_IPOOL_HRXQ], hrxq_idx);
+	if (!hrxq)
+		return 0;
 	if (!hrxq->standalone)
 		return mlx5_cache_unregister(&priv->hrxqs, &hrxq->entry);
 	__mlx5_hrxq_remove(dev, hrxq);
-- 
1.8.3.1


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

* Re: [dpdk-dev] [PATCH v4] net/mlx5: fix incorrect group value of sample suffix flow
  2020-11-04 13:29       ` [dpdk-dev] [PATCH v4] " Jiawei Wang
@ 2020-11-08 11:13         ` Raslan Darawsheh
  0 siblings, 0 replies; 7+ messages in thread
From: Raslan Darawsheh @ 2020-11-08 11:13 UTC (permalink / raw)
  To: Jiawei(Jonny) Wang, Slava Ovsiienko, Matan Azrad, Ori Kam; +Cc: dev

Hi,

> -----Original Message-----
> From: Jiawei Wang <jiaweiw@nvidia.com>
> Sent: Wednesday, November 4, 2020 3:30 PM
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; Ori Kam <orika@nvidia.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>
> Subject: [PATCH v4] net/mlx5: fix incorrect group value of sample suffix flow
> 
> mlx5 PMD splited the sampling flow into prefix flow and suffix
> flow. On the sample action translation function, the scaled
> group value of suffix flow be attached into sample object and
> saved into sample resource.
> 
> mlx5 PMD fetched the group value from the sample resource to
> create the suffix flow. On the mlx5_flow_group_to_table
> function the group value of suffix flow was scaled with table
> factor again and translated into HW table. That caused the
> incorrect group value of sample suffix flow.
> 
> The fix introduces a 'skip_scale' flag and sets it to 1 for the
> sample suffix flow creation. On the mlx5_flow_group_to_table
> function skips the scale with table factor to use the correct
> group value.
> 
> Fixes: b0c8d823552a ("net/mlx5: implement tunnel offload")
> 
> Signed-off-by: Jiawei Wang <jiaweiw@nvidia.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
> v4:
> * Init the flow split data for each dev_flow
> * Rebase
> 
> v3:
> * Rebase
> 
> v2:
> * Update commit message
> * Rebase
> 
> ---
>  drivers/net/mlx5/mlx5_flow.c    | 159 ++++++++++++++++++------------------
> ----
>  drivers/net/mlx5/mlx5_flow.h    |  15 +++-
>  drivers/net/mlx5/mlx5_flow_dv.c |   2 +
>  drivers/net/mlx5/mlx5_rxq.c     |   2 +
>  4 files changed, 91 insertions(+), 87 deletions(-)
> 

Patch applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh

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

end of thread, other threads:[~2020-11-08 11:13 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-28  7:41 [dpdk-dev] [PATCH] net/mlx5: fix incorrect group value of sample suffix flow Jiawei Wang
2020-10-30  7:26 ` [dpdk-dev] [PATCH v2] " Jiawei Wang
2020-10-30  7:26   ` [dpdk-dev] [PATCH v2] net/mlx5: " Jiawei Wang
2020-11-01  6:50     ` [dpdk-dev] [PATCH v3] " Jiawei Wang
2020-11-01 16:41       ` Slava Ovsiienko
2020-11-04 13:29       ` [dpdk-dev] [PATCH v4] " Jiawei Wang
2020-11-08 11:13         ` Raslan Darawsheh

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/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 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


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