[dpdk-dev] [PATCH 0/2] net/mlx5: fix flow indirect action reference counting

[dpdk-dev] [PATCH 0/2] net/mlx5: fix flow indirect action reference counting

[Dmitry Kozlyuk]

Dmitry Kozlyuk (2):
  net/mlx5: report error on indirect CT action destroy
  net/mlx5: fix flow indirect action reference counting

 drivers/net/mlx5/mlx5_flow_dv.c | 59 ++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 20 deletions(-)

-- 
2.25.1

[dpdk-dev] [PATCH 1/2] net/mlx5: report error on indirect CT action destroy

[Dmitry Kozlyuk]

When an indirect CT action of mlx5 PMD could not be destroyed,
rte_action_handle_destroy() was returning (-1), but the error
structure was not filled. This lead to a segfault in testpmd
on an attempt to print it. Fill the details for each possible
cause of this error.

Fixes: c5a49265fc23 ("net/mlx5: add ASO connection tracking destroy")
Cc: bingz@nvidia.com

Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow_dv.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 3f6f5dcfba..b769ae4007 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -12304,17 +12304,27 @@ flow_dv_aso_ct_dev_release(struct rte_eth_dev *dev, uint32_t idx)
 }
 
 static inline int
-flow_dv_aso_ct_release(struct rte_eth_dev *dev, uint32_t own_idx)
+flow_dv_aso_ct_release(struct rte_eth_dev *dev, uint32_t own_idx,
+		       struct rte_flow_error *error)
 {
 	uint16_t owner = (uint16_t)MLX5_INDIRECT_ACT_CT_GET_OWNER(own_idx);
 	uint32_t idx = MLX5_INDIRECT_ACT_CT_GET_IDX(own_idx);
 	struct rte_eth_dev *owndev = &rte_eth_devices[owner];
-	RTE_SET_USED(dev);
+	int ret;
 
 	MLX5_ASSERT(owner < RTE_MAX_ETHPORTS);
 	if (dev->data->dev_started != 1)
-		return -1;
-	return flow_dv_aso_ct_dev_release(owndev, idx);
+		return rte_flow_error_set(error, EAGAIN,
+					  RTE_FLOW_ERROR_TYPE_ACTION,
+					  NULL,
+					  "Indirect CT action cannot be destroyed when the port is stopped");
+	ret = flow_dv_aso_ct_dev_release(owndev, idx);
+	if (ret < 0)
+		return rte_flow_error_set(error, EAGAIN,
+					  RTE_FLOW_ERROR_TYPE_ACTION,
+					  NULL,
+					  "Current state prevents indirect CT action from being destroyed");
+	return ret;
 }
 
 /*
@@ -14365,7 +14375,7 @@ flow_dv_destroy(struct rte_eth_dev *dev, struct rte_flow *flow)
 	}
 	/* Keep the current age handling by default. */
 	if (flow->indirect_type == MLX5_INDIRECT_ACTION_TYPE_CT && flow->ct)
-		flow_dv_aso_ct_release(dev, flow->ct);
+		flow_dv_aso_ct_release(dev, flow->ct, NULL);
 	else if (flow->age)
 		flow_dv_aso_age_release(dev, flow->age);
 	if (flow->geneve_tlv_option) {
@@ -14900,7 +14910,7 @@ flow_dv_action_destroy(struct rte_eth_dev *dev,
 				" released with references %d.", idx, ret);
 		return 0;
 	case MLX5_INDIRECT_ACTION_TYPE_CT:
-		ret = flow_dv_aso_ct_release(dev, idx);
+		ret = flow_dv_aso_ct_release(dev, idx, error);
 		if (ret < 0)
 			return ret;
 		if (ret > 0)
-- 
2.25.1

[dpdk-dev] [PATCH 2/2] net/mlx5: fix flow indirect action reference counting

[Dmitry Kozlyuk]

When an indirect action is used in a flow rule with a pattern that
causes RSS expansion, each device flow generated by the expansion
incremented the reference counter of the action. When such a flow was
destroyed, its action reference counter had been decremented only once.
The action remained marked as being used and could not be destroyed.
COUNT, AGE, and CONNTRACK indirect actions have been affected
(for AGE the error was not immediately observable).
Increment action counter only once for the original flow rule.

Fixes: 81073e1f8ce1 ("net/mlx5: support shared age action")
Fixes: 2d084f69aa26 ("net/mlx5: add translation of connection tracking action")
Fixes: f3191849f2c2 ("net/mlx5: support flow count action handle")
Cc: matan@nvidia.com
Cc: bingz@nvidia.com
Cc: michaelba@nvidia.com
Cc: stable@dpdk.org

Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow_dv.c | 37 ++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index b769ae4007..626513ce56 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -12840,10 +12840,13 @@ flow_dv_translate(struct rte_eth_dev *dev,
 				MLX5_FLOW_FATE_QUEUE;
 			break;
 		case MLX5_RTE_FLOW_ACTION_TYPE_AGE:
-			flow->age = (uint32_t)(uintptr_t)(action->conf);
-			age_act = flow_aso_age_get_by_idx(dev, flow->age);
-			__atomic_fetch_add(&age_act->refcnt, 1,
-					   __ATOMIC_RELAXED);
+			owner_idx = (uint32_t)(uintptr_t)action->conf;
+			age_act = flow_aso_age_get_by_idx(dev, owner_idx);
+			if (flow->age == 0) {
+				flow->age = owner_idx;
+				__atomic_fetch_add(&age_act->refcnt, 1,
+						   __ATOMIC_RELAXED);
+			}
 			age_act_pos = actions_n++;
 			action_flags |= MLX5_FLOW_ACTION_AGE;
 			break;
@@ -12853,9 +12856,9 @@ flow_dv_translate(struct rte_eth_dev *dev,
 			action_flags |= MLX5_FLOW_ACTION_AGE;
 			break;
 		case MLX5_RTE_FLOW_ACTION_TYPE_COUNT:
-			cnt_act = flow_dv_counter_get_by_idx(dev,
-					(uint32_t)(uintptr_t)action->conf,
-					NULL);
+			owner_idx = (uint32_t)(uintptr_t)action->conf;
+			cnt_act = flow_dv_counter_get_by_idx(dev, owner_idx,
+							     NULL);
 			MLX5_ASSERT(cnt_act != NULL);
 			/**
 			 * When creating meter drop flow in drop table, the
@@ -12866,10 +12869,12 @@ flow_dv_translate(struct rte_eth_dev *dev,
 				dev_flow->dv.actions[actions_n++] =
 							cnt_act->action;
 			} else {
-				flow->counter =
-					(uint32_t)(uintptr_t)(action->conf);
-				__atomic_fetch_add(&cnt_act->shared_info.refcnt,
-						1, __ATOMIC_RELAXED);
+				if (flow->counter == 0) {
+					flow->counter = owner_idx;
+					__atomic_fetch_add
+						(&cnt_act->shared_info.refcnt,
+						 1, __ATOMIC_RELAXED);
+				}
 				/* Save information first, will apply later. */
 				action_flags |= MLX5_FLOW_ACTION_COUNT;
 			}
@@ -13192,9 +13197,13 @@ flow_dv_translate(struct rte_eth_dev *dev,
 			else
 				dev_flow->dv.actions[actions_n] =
 							ct->dr_action_rply;
-			flow->indirect_type = MLX5_INDIRECT_ACTION_TYPE_CT;
-			flow->ct = owner_idx;
-			__atomic_fetch_add(&ct->refcnt, 1, __ATOMIC_RELAXED);
+			if (flow->ct == 0) {
+				flow->indirect_type =
+						MLX5_INDIRECT_ACTION_TYPE_CT;
+				flow->ct = owner_idx;
+				__atomic_fetch_add(&ct->refcnt, 1,
+						   __ATOMIC_RELAXED);
+			}
 			actions_n++;
 			action_flags |= MLX5_FLOW_ACTION_CT;
 			break;
-- 
2.25.1

Re: [dpdk-dev] [PATCH 0/2] net/mlx5: fix flow indirect action reference counting

[Thomas Monjalon]

01/09/2021 10:19, Dmitry Kozlyuk:
> Dmitry Kozlyuk (2):
>   net/mlx5: report error on indirect CT action destroy
>   net/mlx5: fix flow indirect action reference counting

Applied in next-net-mlx, thanks.