[dpdk-dev] [PATCH] net/mlx5: avoid invalid counter query

[dpdk-dev] [PATCH] net/mlx5: avoid invalid counter query

[Suanming Mou]

Currently, the counter query requires the counter id should start
with 4 aligned. In none-batch mode, the counter pool might have the
chance to get the counter id not 4 aligned. In this case, the counter
should be skipped, or the query will be failed.

Skip the counter with id not 4 aligned as the first counter in the
none-batch count pool to avoid invalid counter query. Once having
new min_dcs id in the poll less than the skipped counters, the
skipped counters will be returned to the pool free list to use.

Signed-off-by: Suanming Mou <suanmingm@mellanox.com>
---
 drivers/net/mlx5/mlx5.h         |  1 +
 drivers/net/mlx5/mlx5_flow.c    |  3 ++
 drivers/net/mlx5/mlx5_flow_dv.c | 73 +++++++++++++++++++++++++++++++++++++++--
 3 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 46e66eb..8d4dba1 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -391,6 +391,7 @@ struct mlx5_flow_counter_ext {
 struct mlx5_flow_counter_pool {
 	TAILQ_ENTRY(mlx5_flow_counter_pool) next;
 	struct mlx5_counters counters[2]; /* Free counter list. */
+	struct mlx5_counters skip_counters; /* Skipped counter list. */
 	union {
 		struct mlx5_devx_obj *min_dcs;
 		rte_atomic64_t a64_dcs;
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index ae5ccc2..09f414f 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -5976,6 +5976,9 @@ struct mlx5_meter_domains_infos *
 		goto set_alarm;
 	dcs = (struct mlx5_devx_obj *)(uintptr_t)rte_atomic64_read
 							      (&pool->a64_dcs);
+	if (dcs->id & 3)
+		/* Pool without valid counter. */
+		goto set_alarm;
 	offset = batch ? 0 : dcs->id % MLX5_COUNTERS_PER_POOL;
 	/*
 	 * Identify the counters released between query trigger and query
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 8b5b683..65e383a 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -19,6 +19,7 @@
 #endif
 
 #include <rte_common.h>
+#include <rte_tailq.h>
 #include <rte_ether.h>
 #include <rte_ethdev_driver.h>
 #include <rte_flow.h>
@@ -4384,6 +4385,7 @@ struct field_modify_info modify_tcp[] = {
 	rte_spinlock_init(&pool->sl);
 	TAILQ_INIT(&pool->counters[0]);
 	TAILQ_INIT(&pool->counters[1]);
+	TAILQ_INIT(&pool->skip_counters);
 	TAILQ_INSERT_HEAD(&cont->pool_list, pool, next);
 	pool->index = n_valid;
 	cont->pools[n_valid] = pool;
@@ -4403,6 +4405,67 @@ struct field_modify_info modify_tcp[] = {
 }
 
 /**
+ * Check counter pool min_dcs.
+ *
+ * If counter pool starts with min_dcs id not aligned with 4,
+ * the pool query will be invalid. The pool should retry to
+ * allocate a new min_dcs, the current dcs will be skipped.
+ *
+ * @param[in] pool
+ *   Current counter pool.
+ * @param[in] dcs
+ *   The devX counter handle.
+ *
+ * @return
+ *   0 on valid, -1 otherwise.
+ */
+static int
+flow_dv_counter_check_dcs_id(struct mlx5_flow_counter_pool *pool,
+			     struct mlx5_devx_obj *dcs)
+{
+	struct mlx5_flow_counter *cnt;
+	uint32_t idx;
+
+	if (!(pool->min_dcs->id & 0x3) && dcs->id >= pool->min_dcs->id)
+		return 0;
+	idx = dcs->id % MLX5_COUNTERS_PER_POOL;
+	cnt = MLX5_POOL_GET_CNT(pool, idx);
+	MLX5_GET_POOL_CNT_EXT(pool, idx)->dcs = dcs;
+	TAILQ_INSERT_HEAD(&pool->skip_counters, cnt, next);
+	return -1;
+}
+
+/**
+ * Check skipped counters in the pool.
+ *
+ * As counter pool query requires the first counter dcs
+ * id start with 4 alinged, if the pool counters with
+ * min_dcs id are not aligned with 4, the counters will
+ * be skipped.
+ * Once other min_dcs id less than these skipped counter
+ * dcs id appears, the skipped counters will be safe to
+ * use.
+ *
+ * @param[in] pool
+ *   Current counter pool.
+ */
+static void
+flow_dv_counter_check_skip_counter(struct mlx5_flow_counter_pool *pool)
+{
+	struct mlx5_flow_counter *cnt;
+	void *tmp;
+
+	TAILQ_FOREACH_SAFE(cnt, &pool->skip_counters, next, tmp) {
+		if (MLX5_CNT_TO_CNT_EXT(pool, cnt)->dcs->id >
+		    pool->min_dcs->id) {
+			TAILQ_REMOVE(&pool->skip_counters, cnt, next);
+			TAILQ_INSERT_TAIL(&pool->counters[pool->query_gen],
+					  cnt, next);
+		}
+	}
+}
+
+/**
  * Update the minimum dcs-id for aged or no-aged counter pool.
  *
  * @param[in] dev
@@ -4427,12 +4490,15 @@ struct field_modify_info modify_tcp[] = {
 	other = flow_dv_find_pool_by_id(cont, pool->min_dcs->id);
 	if (!other)
 		return;
-	if (pool->min_dcs->id < other->min_dcs->id) {
+	if (!(pool->min_dcs->id & 0x3) &&
+	    pool->min_dcs->id < other->min_dcs->id) {
 		rte_atomic64_set(&other->a64_dcs,
 			rte_atomic64_read(&pool->a64_dcs));
+		flow_dv_counter_check_skip_counter(other);
 	} else {
 		rte_atomic64_set(&pool->a64_dcs,
 			rte_atomic64_read(&other->a64_dcs));
+		flow_dv_counter_check_skip_counter(pool);
 	}
 }
 /**
@@ -4477,12 +4543,15 @@ struct field_modify_info modify_tcp[] = {
 				mlx5_devx_cmd_destroy(dcs);
 				return NULL;
 			}
-		} else if (dcs->id < pool->min_dcs->id) {
+		} else if (dcs->id < pool->min_dcs->id ||
+			  (pool->min_dcs->id & 0x3 && !(dcs->id & 0x3))) {
 			rte_atomic64_set(&pool->a64_dcs,
 					 (int64_t)(uintptr_t)dcs);
+			flow_dv_counter_check_skip_counter(pool);
 		}
 		flow_dv_counter_update_min_dcs(dev,
 						pool, batch, age);
+		flow_dv_counter_check_dcs_id(pool, dcs);
 		i = dcs->id % MLX5_COUNTERS_PER_POOL;
 		cnt = MLX5_POOL_GET_CNT(pool, i);
 		cnt->pool = pool;
-- 
1.8.3.1

[dpdk-dev] [PATCH v2 1/3] net/mlx5: separate aging counter pool range

[Suanming Mou]

Currently, when allocate the counter or counter based age from group 0,
counter and age may share the same counter dcs ID range. Both age and
pure counter need to sync up with each other's container to check if
the ID range exists and update the min_dcs.

It comes two disadvantages:
1. If the ID range is shared, this counter range will be queried twice
both from age and pure counter container in 1s.
2. The same range counter check between the two container makes the
counter allocate sync min_dcs time to time with extra min_dcs updating.

This patch avoid the same ID range to be shared when allocate the new
pool. If the same ID range exists in other container, just add the
counter to the other container until get new range which saves the
min_dcs sync up time to time.

Signed-off-by: Suanming Mou <suanmingm@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow_dv.c | 69 +++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 41 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index f0cc7ad..2fc4457 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -4408,39 +4408,6 @@ struct field_modify_info modify_tcp[] = {
 }
 
 /**
- * Update the minimum dcs-id for aged or no-aged counter pool.
- *
- * @param[in] dev
- *   Pointer to the Ethernet device structure.
- * @param[in] pool
- *   Current counter pool.
- * @param[in] batch
- *   Whether the pool is for counter that was allocated by batch command.
- * @param[in] age
- *   Whether the counter is for aging.
- */
-static void
-flow_dv_counter_update_min_dcs(struct rte_eth_dev *dev,
-			struct mlx5_flow_counter_pool *pool,
-			uint32_t batch, uint32_t age)
-{
-	struct mlx5_priv *priv = dev->data->dev_private;
-	struct mlx5_flow_counter_pool *other;
-	struct mlx5_pools_container *cont;
-
-	cont = MLX5_CNT_CONTAINER(priv->sh, batch, (age ^ 0x1));
-	other = flow_dv_find_pool_by_id(cont, pool->min_dcs->id);
-	if (!other)
-		return;
-	if (pool->min_dcs->id < other->min_dcs->id) {
-		rte_atomic64_set(&other->a64_dcs,
-			rte_atomic64_read(&pool->a64_dcs));
-	} else {
-		rte_atomic64_set(&pool->a64_dcs,
-			rte_atomic64_read(&other->a64_dcs));
-	}
-}
-/**
  * Prepare a new counter and/or a new counter pool.
  *
  * @param[in] dev
@@ -4467,31 +4434,50 @@ struct field_modify_info modify_tcp[] = {
 	struct mlx5_counters tmp_tq;
 	struct mlx5_devx_obj *dcs = NULL;
 	struct mlx5_flow_counter *cnt;
+	uint32_t add2other;
 	uint32_t i;
 
 	cont = MLX5_CNT_CONTAINER(priv->sh, batch, age);
 	if (!batch) {
+retry:
+		add2other = 0;
 		/* bulk_bitmap must be 0 for single counter allocation. */
 		dcs = mlx5_devx_cmd_flow_counter_alloc(priv->sh->ctx, 0);
 		if (!dcs)
 			return NULL;
 		pool = flow_dv_find_pool_by_id(cont, dcs->id);
+		/* Check if counter belongs to exist pool ID range. */
 		if (!pool) {
-			pool = flow_dv_pool_create(dev, dcs, batch, age);
-			if (!pool) {
-				mlx5_devx_cmd_destroy(dcs);
-				return NULL;
+			pool = flow_dv_find_pool_by_id
+			       (MLX5_CNT_CONTAINER
+			       (priv->sh, batch, (age ^ 0x1)), dcs->id);
+			/*
+			 * Pool eixsts, counter will be added to the other
+			 * container, need to reallocate it later.
+			 */
+			if (pool) {
+				add2other = 1;
+			} else {
+				pool = flow_dv_pool_create(dev, dcs, batch,
+							   age);
+				if (!pool) {
+					mlx5_devx_cmd_destroy(dcs);
+					return NULL;
+				}
 			}
-		} else if (dcs->id < pool->min_dcs->id) {
+		}
+		if (dcs->id < pool->min_dcs->id)
 			rte_atomic64_set(&pool->a64_dcs,
 					 (int64_t)(uintptr_t)dcs);
-		}
-		flow_dv_counter_update_min_dcs(dev,
-						pool, batch, age);
 		i = dcs->id % MLX5_COUNTERS_PER_POOL;
 		cnt = MLX5_POOL_GET_CNT(pool, i);
 		cnt->pool = pool;
 		MLX5_GET_POOL_CNT_EXT(pool, i)->dcs = dcs;
+		if (add2other) {
+			TAILQ_INSERT_TAIL(&pool->counters[pool->query_gen],
+					  cnt, next);
+			goto retry;
+		}
 		*cnt_free = cnt;
 		return pool;
 	}
@@ -9985,3 +9971,4 @@ struct field_modify_info modify_tcp[] = {
 };
 
 #endif /* HAVE_IBV_FLOW_DV_SUPPORT */
+
-- 
1.8.3.1

[dpdk-dev] [PATCH v2 2/3] common/mlx5: add counter batch query ID alignment define

[Suanming Mou]

The counter batch query requires ID to be aligned with 4.

Signed-off-by: Suanming Mou <suanmingm@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>
---
 drivers/common/mlx5/mlx5_prm.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/common/mlx5/mlx5_prm.h b/drivers/common/mlx5/mlx5_prm.h
index cb5f968..8565d25 100644
--- a/drivers/common/mlx5/mlx5_prm.h
+++ b/drivers/common/mlx5/mlx5_prm.h
@@ -818,6 +818,9 @@ enum {
  */
 #define MLX5_CNT_BATCH_OFFSET 0x800000
 
+/* The counter batch query requires ID align with 4. */
+#define MLX5_CNT_BATCH_QUERY_ID_ALIGNMENT 4
+
 /* Flow counters. */
 struct mlx5_ifc_alloc_flow_counter_out_bits {
 	u8         status[0x8];
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2 2/3] common/mlx5: add counter batch query ID alignment define

[Ferruh Yigit]

On 7/22/2020 8:58 AM, Suanming Mou wrote:
> The counter batch query requires ID to be aligned with 4.
> 
> Signed-off-by: Suanming Mou <suanmingm@mellanox.com>
> Acked-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/common/mlx5/mlx5_prm.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/common/mlx5/mlx5_prm.h b/drivers/common/mlx5/mlx5_prm.h
> index cb5f968..8565d25 100644
> --- a/drivers/common/mlx5/mlx5_prm.h
> +++ b/drivers/common/mlx5/mlx5_prm.h
> @@ -818,6 +818,9 @@ enum {
>   */
>  #define MLX5_CNT_BATCH_OFFSET 0x800000
>  
> +/* The counter batch query requires ID align with 4. */
> +#define MLX5_CNT_BATCH_QUERY_ID_ALIGNMENT 4
> +
>  /* Flow counters. */
>  struct mlx5_ifc_alloc_flow_counter_out_bits {
>  	u8         status[0x8];
> 

Having only macro separated from usage is not very helpful, squashing this
commit with 3/3 where the macro is used in 'next-net' repo.

[dpdk-dev] [PATCH v2 3/3] net/mlx5: fix invalid counter query

[Suanming Mou]

Currently, the counter query requires the counter ID should start
with 4 aligned. In none-batch mode, the counter pool might have the
chance to get the counter ID not 4 aligned. In this case, the counter
should be skipped, or the query will be failed.

Skip the counter with ID not 4 aligned as the first counter in the
none-batch count pool to avoid invalid counter query. Once having
new min_dcs ID in the poll less than the skipped counters, the
skipped counters will be returned to the pool free list to use.

Fixes: 5382d28c2110 ("net/mlx5: accelerate DV flow counter transactions")
Cc: stable@dpdk.org

Signed-off-by: Suanming Mou <suanmingm@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx5/mlx5.h         |  6 ++-
 drivers/net/mlx5/mlx5_flow.c    |  6 +++
 drivers/net/mlx5/mlx5_flow_dv.c | 94 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 103 insertions(+), 3 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 5d7d609..0ecfc76 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -366,8 +366,9 @@ struct mlx5_flow_counter {
 struct mlx5_flow_counter_ext {
 	uint32_t shared:1; /**< Share counter ID with other flow rules. */
 	uint32_t batch: 1;
+	uint32_t skipped:1; /* This counter is skipped or not. */
 	/**< Whether the counter was allocated by batch command. */
-	uint32_t ref_cnt:30; /**< Reference counter. */
+	uint32_t ref_cnt:29; /**< Reference counter. */
 	uint32_t id; /**< User counter ID. */
 	union {  /**< Holds the counters for the rule. */
 #if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42)
@@ -390,8 +391,9 @@ struct mlx5_flow_counter_pool {
 		rte_atomic64_t a64_dcs;
 	};
 	/* The devx object of the minimum counter ID. */
-	uint32_t index:29; /* Pool index in container. */
+	uint32_t index:28; /* Pool index in container. */
 	uint32_t type:2; /* Memory type behind the counter array. */
+	uint32_t skip_cnt:1; /* Pool contains skipped counter. */
 	volatile uint32_t query_gen:1; /* Query round. */
 	rte_spinlock_t sl; /* The pool lock. */
 	struct mlx5_counter_stats_raw *raw;
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index b56bee4..40a8575 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -5974,6 +5974,11 @@ struct mlx5_meter_domains_infos *
 		goto set_alarm;
 	dcs = (struct mlx5_devx_obj *)(uintptr_t)rte_atomic64_read
 							      (&pool->a64_dcs);
+	if (dcs->id & (MLX5_CNT_BATCH_QUERY_ID_ALIGNMENT - 1)) {
+		/* Pool without valid counter. */
+		pool->raw_hw = NULL;
+		goto next_pool;
+	}
 	offset = batch ? 0 : dcs->id % MLX5_COUNTERS_PER_POOL;
 	/*
 	 * Identify the counters released between query trigger and query
@@ -5998,6 +6003,7 @@ struct mlx5_meter_domains_infos *
 	pool->raw_hw->min_dcs_id = dcs->id;
 	LIST_REMOVE(pool->raw_hw, next);
 	sh->cmng.pending_queries++;
+next_pool:
 	pool_index++;
 	if (pool_index >= rte_atomic16_read(&cont->n_valid)) {
 		batch ^= 0x1;
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 2fc4457..a2b7329 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -4408,6 +4408,66 @@ struct field_modify_info modify_tcp[] = {
 }
 
 /**
+ * Restore skipped counters in the pool.
+ *
+ * As counter pool query requires the first counter dcs
+ * ID start with 4 alinged, if the pool counters with
+ * min_dcs ID are not aligned with 4, the counters will
+ * be skipped.
+ * Once other min_dcs ID less than these skipped counter
+ * dcs ID appears, the skipped counters will be safe to
+ * use.
+ * Should be called when min_dcs is updated.
+ *
+ * @param[in] pool
+ *   Current counter pool.
+ * @param[in] last_min_dcs
+ *   Last min_dcs.
+ */
+static void
+flow_dv_counter_restore(struct mlx5_flow_counter_pool *pool,
+			struct mlx5_devx_obj *last_min_dcs)
+{
+	struct mlx5_flow_counter_ext *cnt_ext;
+	uint32_t offset, new_offset;
+	uint32_t skip_cnt = 0;
+	uint32_t i;
+
+	if (!pool->skip_cnt)
+		return;
+	/*
+	 * If last min_dcs is not valid. The skipped counter may even after
+	 * last min_dcs, set the offset to the whole pool.
+	 */
+	if (last_min_dcs->id & (MLX5_CNT_BATCH_QUERY_ID_ALIGNMENT - 1))
+		offset = MLX5_COUNTERS_PER_POOL;
+	else
+		offset = last_min_dcs->id % MLX5_COUNTERS_PER_POOL;
+	new_offset = pool->min_dcs->id % MLX5_COUNTERS_PER_POOL;
+	/*
+	 * Check the counters from 1 to the last_min_dcs range. Counters
+	 * before new min_dcs indicates pool still has skipped counters.
+	 * Counters be skipped after new min_dcs will be ready to use.
+	 * Offset 0 counter must be empty or min_dcs, start from 1.
+	 */
+	for (i = 1; i < offset; i++) {
+		cnt_ext = MLX5_GET_POOL_CNT_EXT(pool, i);
+		if (cnt_ext->skipped) {
+			if (i > new_offset) {
+				cnt_ext->skipped = 0;
+				TAILQ_INSERT_TAIL
+					(&pool->counters[pool->query_gen],
+					 MLX5_POOL_GET_CNT(pool, i), next);
+			} else {
+				skip_cnt++;
+			}
+		}
+	}
+	if (!skip_cnt)
+		pool->skip_cnt = 0;
+}
+
+/**
  * Prepare a new counter and/or a new counter pool.
  *
  * @param[in] dev
@@ -4432,6 +4492,7 @@ struct field_modify_info modify_tcp[] = {
 	struct mlx5_pools_container *cont;
 	struct mlx5_flow_counter_pool *pool;
 	struct mlx5_counters tmp_tq;
+	struct mlx5_devx_obj *last_min_dcs;
 	struct mlx5_devx_obj *dcs = NULL;
 	struct mlx5_flow_counter *cnt;
 	uint32_t add2other;
@@ -4466,13 +4527,44 @@ struct field_modify_info modify_tcp[] = {
 				}
 			}
 		}
-		if (dcs->id < pool->min_dcs->id)
+		if ((dcs->id < pool->min_dcs->id ||
+		    pool->min_dcs->id &
+		    (MLX5_CNT_BATCH_QUERY_ID_ALIGNMENT - 1)) &&
+		    !(dcs->id & (MLX5_CNT_BATCH_QUERY_ID_ALIGNMENT - 1))) {
+			/*
+			 * Update the pool min_dcs only if current dcs is
+			 * valid and exist min_dcs is not valid or greater
+			 * than new dcs.
+			 */
+			last_min_dcs = pool->min_dcs;
 			rte_atomic64_set(&pool->a64_dcs,
 					 (int64_t)(uintptr_t)dcs);
+			/*
+			 * Restore any skipped counters if the new min_dcs
+			 * ID is smaller or min_dcs is not valid.
+			 */
+			if (dcs->id < last_min_dcs->id ||
+			    last_min_dcs->id &
+			    (MLX5_CNT_BATCH_QUERY_ID_ALIGNMENT - 1))
+				flow_dv_counter_restore(pool, last_min_dcs);
+		}
 		i = dcs->id % MLX5_COUNTERS_PER_POOL;
 		cnt = MLX5_POOL_GET_CNT(pool, i);
 		cnt->pool = pool;
 		MLX5_GET_POOL_CNT_EXT(pool, i)->dcs = dcs;
+		/*
+		 * If min_dcs is not valid, it means the new allocated dcs
+		 * also fail to become the valid min_dcs, just skip it.
+		 * Or if min_dcs is valid, and new dcs ID is smaller than
+		 * min_dcs, but not become the min_dcs, also skip it.
+		 */
+		if (pool->min_dcs->id &
+		    (MLX5_CNT_BATCH_QUERY_ID_ALIGNMENT - 1) ||
+		    dcs->id < pool->min_dcs->id) {
+			MLX5_GET_POOL_CNT_EXT(pool, i)->skipped = 1;
+			pool->skip_cnt = 1;
+			goto retry;
+		}
 		if (add2other) {
 			TAILQ_INSERT_TAIL(&pool->counters[pool->query_gen],
 					  cnt, next);
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2 1/3] net/mlx5: separate aging counter pool range

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: Suanming Mou <suanmingm@mellanox.com>
> Sent: Wednesday, July 22, 2020 10:59 AM
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; Matan Azrad
> <matan@mellanox.com>
> Cc: Raslan Darawsheh <rasland@mellanox.com>; dev@dpdk.org
> Subject: [PATCH v2 1/3] net/mlx5: separate aging counter pool range
> 
> Currently, when allocate the counter or counter based age from group 0,
> counter and age may share the same counter dcs ID range. Both age and
> pure counter need to sync up with each other's container to check if
> the ID range exists and update the min_dcs.
> 
> It comes two disadvantages:
> 1. If the ID range is shared, this counter range will be queried twice
> both from age and pure counter container in 1s.
> 2. The same range counter check between the two container makes the
> counter allocate sync min_dcs time to time with extra min_dcs updating.
> 
> This patch avoid the same ID range to be shared when allocate the new
> pool. If the same ID range exists in other container, just add the
> counter to the other container until get new range which saves the
> min_dcs sync up time to time.
> 
> Signed-off-by: Suanming Mou <suanmingm@mellanox.com>
> Acked-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_flow_dv.c | 69 +++++++++++++++++------------------
> ------
>  1 file changed, 28 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c
> index f0cc7ad..2fc4457 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -4408,39 +4408,6 @@ struct field_modify_info modify_tcp[] = {
>  }
> 
>  /**
> - * Update the minimum dcs-id for aged or no-aged counter pool.
> - *
> - * @param[in] dev
> - *   Pointer to the Ethernet device structure.
> - * @param[in] pool
> - *   Current counter pool.
> - * @param[in] batch
> - *   Whether the pool is for counter that was allocated by batch command.
> - * @param[in] age
> - *   Whether the counter is for aging.
> - */
> -static void
> -flow_dv_counter_update_min_dcs(struct rte_eth_dev *dev,
> -			struct mlx5_flow_counter_pool *pool,
> -			uint32_t batch, uint32_t age)
> -{
> -	struct mlx5_priv *priv = dev->data->dev_private;
> -	struct mlx5_flow_counter_pool *other;
> -	struct mlx5_pools_container *cont;
> -
> -	cont = MLX5_CNT_CONTAINER(priv->sh, batch, (age ^ 0x1));
> -	other = flow_dv_find_pool_by_id(cont, pool->min_dcs->id);
> -	if (!other)
> -		return;
> -	if (pool->min_dcs->id < other->min_dcs->id) {
> -		rte_atomic64_set(&other->a64_dcs,
> -			rte_atomic64_read(&pool->a64_dcs));
> -	} else {
> -		rte_atomic64_set(&pool->a64_dcs,
> -			rte_atomic64_read(&other->a64_dcs));
> -	}
> -}
> -/**
>   * Prepare a new counter and/or a new counter pool.
>   *
>   * @param[in] dev
> @@ -4467,31 +4434,50 @@ struct field_modify_info modify_tcp[] = {
>  	struct mlx5_counters tmp_tq;
>  	struct mlx5_devx_obj *dcs = NULL;
>  	struct mlx5_flow_counter *cnt;
> +	uint32_t add2other;
>  	uint32_t i;
> 
>  	cont = MLX5_CNT_CONTAINER(priv->sh, batch, age);
>  	if (!batch) {
> +retry:
> +		add2other = 0;
>  		/* bulk_bitmap must be 0 for single counter allocation. */
>  		dcs = mlx5_devx_cmd_flow_counter_alloc(priv->sh->ctx, 0);
>  		if (!dcs)
>  			return NULL;
>  		pool = flow_dv_find_pool_by_id(cont, dcs->id);
> +		/* Check if counter belongs to exist pool ID range. */
>  		if (!pool) {
> -			pool = flow_dv_pool_create(dev, dcs, batch, age);
> -			if (!pool) {
> -				mlx5_devx_cmd_destroy(dcs);
> -				return NULL;
> +			pool = flow_dv_find_pool_by_id
> +			       (MLX5_CNT_CONTAINER
> +			       (priv->sh, batch, (age ^ 0x1)), dcs->id);
> +			/*
> +			 * Pool eixsts, counter will be added to the other
> +			 * container, need to reallocate it later.
> +			 */
> +			if (pool) {
> +				add2other = 1;
> +			} else {
> +				pool = flow_dv_pool_create(dev, dcs, batch,
> +							   age);
> +				if (!pool) {
> +					mlx5_devx_cmd_destroy(dcs);
> +					return NULL;
> +				}
>  			}
> -		} else if (dcs->id < pool->min_dcs->id) {
> +		}
> +		if (dcs->id < pool->min_dcs->id)
>  			rte_atomic64_set(&pool->a64_dcs,
>  					 (int64_t)(uintptr_t)dcs);
> -		}
> -		flow_dv_counter_update_min_dcs(dev,
> -						pool, batch, age);
>  		i = dcs->id % MLX5_COUNTERS_PER_POOL;
>  		cnt = MLX5_POOL_GET_CNT(pool, i);
>  		cnt->pool = pool;
>  		MLX5_GET_POOL_CNT_EXT(pool, i)->dcs = dcs;
> +		if (add2other) {
> +			TAILQ_INSERT_TAIL(&pool->counters[pool-
> >query_gen],
> +					  cnt, next);
> +			goto retry;
> +		}
>  		*cnt_free = cnt;
>  		return pool;
>  	}
> @@ -9985,3 +9971,4 @@ struct field_modify_info modify_tcp[] = {
>  };
> 
>  #endif /* HAVE_IBV_FLOW_DV_SUPPORT */
> +
> --
> 1.8.3.1


Series applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh