[dpdk-dev] [PATCH v1 0/2] Fix Meter Hierarchy related issues

[dpdk-dev] [PATCH v1 0/2] Fix Meter Hierarchy related issues

[Shun Hao]

1. fix the wrong flow statistics when flow contains both count/meter.
2. fix the wrong meter domain access when meters in hierarchy support
different domains.

Shun Hao (2):
  net/mlx5: fix meter flow counter traslation
  net/mlx5: fix domains detection in meter hierarchy

 drivers/net/mlx5/mlx5_flow_dv.c | 43 ++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 11 deletions(-)

-- 
2.20.0

[dpdk-dev] [PATCH v1 1/2] net/mlx5: fix meter flow counter traslation

[Shun Hao]

When an RTE flow uses a meter without any modify packet action,
there will be an internal drop flow with meter counter created,
matching the same 5-tuple as the original flow.

In this case, the meter flow count action is wrongly reused as the
original flow counter, leading to wrong flow statistics.

Add a check in the count action translation to detect the meter case
and use the meter drop dedicated counter in the meter 5-tuple flow
only.

Fixes: f3191849f2c2 ("net/mlx5: support flow count action handle")
Cc: stable@dpdk.org

Signed-off-by: Shun Hao <shunh@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow_dv.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 4644ae46bd..f54440c6f5 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -12824,13 +12824,26 @@ flow_dv_translate(struct rte_eth_dev *dev,
 			action_flags |= MLX5_FLOW_ACTION_AGE;
 			break;
 		case MLX5_RTE_FLOW_ACTION_TYPE_COUNT:
-			flow->counter = (uint32_t)(uintptr_t)(action->conf);
-			cnt_act = flow_dv_counter_get_by_idx(dev, flow->counter,
-							     NULL);
-			__atomic_fetch_add(&cnt_act->shared_info.refcnt, 1,
-					   __ATOMIC_RELAXED);
-			/* Save information first, will apply later. */
-			action_flags |= MLX5_FLOW_ACTION_COUNT;
+			cnt_act = flow_dv_counter_get_by_idx(dev,
+					(uint32_t)(uintptr_t)action->conf,
+					NULL);
+			MLX5_ASSERT(cnt_act != NULL);
+			/**
+			 * When creating meter drop flow in drop table, the
+			 * counter should not overwrite the rte flow counter.
+			 */
+			if (attr->group == MLX5_FLOW_TABLE_LEVEL_METER &&
+			    dev_flow->dv.table_id == MLX5_MTR_TABLE_ID_DROP) {
+				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);
+				/* Save information first, will apply later. */
+				action_flags |= MLX5_FLOW_ACTION_COUNT;
+			}
 			break;
 		case RTE_FLOW_ACTION_TYPE_COUNT:
 			if (!dev_conf->devx) {
-- 
2.20.0

[dpdk-dev] [PATCH v1 2/2] net/mlx5: fix domains detection in meter hierarchy

[Shun Hao]

Meters in one hierarchy might support different domains. For
example, one meter may support ingress only, but the root meter
can support all the domains.

If the later meter in the meter hierarchy wrongly doesn't inherit
the first meter's domains, it will lead to invalid domain table
access.

Fix is when creating meter hierarchy, try to inherit the first meter
domains in the meter hierarchy.

Fixes: a3b7af90baba ("net/mlx5: validate meter action in policy")
Cc: stable@dpdk.org

Signed-off-by: Shun Hao <shunh@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow_dv.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index f54440c6f5..31d857030f 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -17527,6 +17527,7 @@ flow_dv_validate_policy_mtr_hierarchy(struct rte_eth_dev *dev,
 					RTE_MTR_ERROR_TYPE_POLICER_ACTION_GREEN,
 					NULL,
 					"Multiple fate actions not supported.");
+	*hierarchy_domain = 0;
 	while (true) {
 		fm = mlx5_flow_meter_find(priv, meter_id, NULL);
 		if (!fm)
@@ -17539,7 +17540,12 @@ flow_dv_validate_policy_mtr_hierarchy(struct rte_eth_dev *dev,
 			"Non termination meter not supported in hierarchy.");
 		policy = mlx5_flow_meter_policy_find(dev, fm->policy_id, NULL);
 		MLX5_ASSERT(policy);
-		if (!policy->is_hierarchy) {
+		/**
+		 * Only inherit the supported domains of the first meter in
+		 * hierarchy.
+		 * One meter supports at least one domain.
+		 */
+		if (!*hierarchy_domain) {
 			if (policy->transfer)
 				*hierarchy_domain |=
 						MLX5_MTR_DOMAIN_TRANSFER_BIT;
@@ -17548,6 +17554,8 @@ flow_dv_validate_policy_mtr_hierarchy(struct rte_eth_dev *dev,
 						MLX5_MTR_DOMAIN_INGRESS_BIT;
 			if (policy->egress)
 				*hierarchy_domain |= MLX5_MTR_DOMAIN_EGRESS_BIT;
+		}
+		if (!policy->is_hierarchy) {
 			*is_rss = policy->is_rss;
 			break;
 		}
@@ -17825,11 +17833,11 @@ flow_dv_validate_mtr_policy_acts(struct rte_eth_dev *dev,
 			 * so MARK action is only in ingress domain.
 			 */
 			domain_color[i] = MLX5_MTR_DOMAIN_INGRESS_BIT;
-		else if (action_flags[i] &
-			 MLX5_FLOW_ACTION_METER_WITH_TERMINATED_POLICY)
-			domain_color[i] = hierarchy_domain;
 		else
 			domain_color[i] = def_domain;
+		if (action_flags[i] &
+		    MLX5_FLOW_ACTION_METER_WITH_TERMINATED_POLICY)
+			domain_color[i] &= hierarchy_domain;
 		/*
 		 * Non-termination actions only support NIC Tx domain.
 		 * The adjustion should be skipped when there is no
-- 
2.20.0

Re: [dpdk-dev] [PATCH v1 0/2] Fix Meter Hierarchy related issues

[Thomas Monjalon]

04/08/2021 09:26, Shun Hao:
> 1. fix the wrong flow statistics when flow contains both count/meter.
> 2. fix the wrong meter domain access when meters in hierarchy support
> different domains.
> 
> Shun Hao (2):
>   net/mlx5: fix meter flow counter traslation
>   net/mlx5: fix domains detection in meter hierarchy

Applied, thanks.