patch 'net/mlx5: fix counter query loop getting stuck' has been queued to stable release 23.11.3

[Xueming Li <xuemingl@nvidia.com>]

Hi,

FYI, your patch has been queued to stable release 23.11.3

Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet.
It will be pushed if I get no objections before 12/10/24. So please
shout if anyone has objections.

Also note that after the patch there's a diff of the upstream commit vs the
patch applied to the branch. This will indicate if there was any rebasing
needed to apply to the stable branch. If there were code changes for rebasing
(ie: not only metadata diffs), please double check that the rebase was
correctly done.

Queued patches are on a temporary branch at:
https://git.dpdk.org/dpdk-stable/log/?h=23.11-staging

This queued commit can be viewed at:
https://git.dpdk.org/dpdk-stable/commit/?h=23.11-staging&id=833fd897b154ab2f3d278426453a7f1514635a66

Thanks.

Xueming Li <xuemingl@nvidia.com>

---
From 833fd897b154ab2f3d278426453a7f1514635a66 Mon Sep 17 00:00:00 2001
From: Dariusz Sosnowski <dsosnowski@nvidia.com>
Date: Wed, 30 Oct 2024 17:30:46 +0100
Subject: [PATCH] net/mlx5: fix counter query loop getting stuck
Cc: Xueming Li <xuemingl@nvidia.com>

[ upstream commit c0e29968294c92ca15fdb34ce63fbba01c4562a6 ]

Counter service thread, responsible for refreshing counter values
stored in host memory, is running an "infinite loop" with the following
logic:

- For each port:
    - Refresh port's counter pool - call to __mlx5_hws_cnt_svc().
    - Perform aging checks.
- Go to sleep if time left in current cycle.
- Repeat.

__mlx5_hws_cnt_svc() used to perform counter value refresh
implemented the following logic:

1. Store number of counters waiting for reset.
2. Issue ASO WQEs to refresh all counters values.
3. Move counters from reset to reuse list.
   Number of moved counters is limited by number stored in step 1 or
   step 4.
4. Store number of counters waiting for reset.
5. If number of counters waiting for reset > 0, go to step 2.

Now, if an application constantly creates/destroys flow rules with
counters and even a single counter is added to reset list during step 2,
counter service thread might end up issuing ASO WQEs endlessly,
without going to sleep and respecting the configured cycle time.

This patch fixes that by remove the loop inside __mlx5_hws_cnt_svc().

As a drawback of this fix, the application must allocate enough counters
to accommodate for the cycle time. This number if roughly equal to the
expected counter release rate.

This patch also:

- Ensures that proper counter related error code is returned,
  when flow rule create failed due to counter allocation problem.
- Adds debug logging to counter service thread.
- Adds documentation for counter service thread.

Fixes: 4d368e1da3a4 ("net/mlx5: support flow counter action for HWS")

Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 doc/guides/nics/mlx5.rst        | 71 +++++++++++++++++++++++++++++++++
 drivers/net/mlx5/mlx5_flow_hw.c | 17 +++++---
 drivers/net/mlx5/mlx5_hws_cnt.c | 46 ++++++++++++---------
 3 files changed, 110 insertions(+), 24 deletions(-)

diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
index 2c59b24d78..d1c3284ca1 100644
--- a/doc/guides/nics/mlx5.rst
+++ b/doc/guides/nics/mlx5.rst
@@ -1874,6 +1874,77 @@ directly but neither destroyed nor flushed.
 The application should re-create the flows as required after the port restart.


+Notes for flow counters
+-----------------------
+
+mlx5 PMD supports the ``COUNT`` flow action,
+which provides an ability to count packets (and bytes)
+matched against a given flow rule.
+This section describes the high level overview of
+how this support is implemented and limitations.
+
+HW steering flow engine
+~~~~~~~~~~~~~~~~~~~~~~~
+
+Flow counters are allocated from HW in bulks.
+A set of bulks forms a flow counter pool managed by PMD.
+When flow counters are queried from HW,
+each counter is identified by an offset in a given bulk.
+Querying HW flow counter requires sending a request to HW,
+which will request a read of counter values for given offsets.
+HW will asynchronously provide these values through a DMA write.
+
+In order to optimize HW to SW communication,
+these requests are handled in a separate counter service thread
+spawned by mlx5 PMD.
+This service thread will refresh the counter values stored in memory,
+in cycles, each spanning ``svc_cycle_time`` milliseconds.
+By default, ``svc_cycle_time`` is set to 500.
+When applications query the ``COUNT`` flow action,
+PMD returns the values stored in host memory.
+
+mlx5 PMD manages 3 global rings of allocated counter offsets:
+
+- ``free`` ring - Counters which were not used at all.
+- ``wait_reset`` ring - Counters which were used in some flow rules,
+  but were recently freed (flow rule was destroyed
+  or an indirect action was destroyed).
+  Since the count value might have changed
+  between the last counter service thread cycle and the moment it was freed,
+  the value in host memory might be stale.
+  During the next service thread cycle,
+  such counters will be moved to ``reuse`` ring.
+- ``reuse`` ring - Counters which were used at least once
+  and can be reused in new flow rules.
+
+When counters are assigned to a flow rule (or allocated to indirect action),
+the PMD first tries to fetch a counter from ``reuse`` ring.
+If it's empty, the PMD fetches a counter from ``free`` ring.
+
+The counter service thread works as follows:
+
+#. Record counters stored in ``wait_reset`` ring.
+#. Read values of all counters which were used at least once
+   or are currently in use.
+#. Move recorded counters from ``wait_reset`` to ``reuse`` ring.
+#. Sleep for ``(query time) - svc_cycle_time`` milliseconds
+#. Repeat.
+
+Because freeing a counter (by destroying a flow rule or destroying indirect action)
+does not immediately make it available for the application,
+the PMD might return:
+
+- ``ENOENT`` if no counter is available in ``free``, ``reuse``
+  or ``wait_reset`` rings.
+  No counter will be available until the application releases some of them.
+- ``EAGAIN`` if no counter is available in ``free`` and ``reuse`` rings,
+  but there are counters in ``wait_reset`` ring.
+  This means that after the next service thread cycle new counters will be available.
+
+The application has to be aware that flow rule create or indirect action create
+might need be retried.
+
+
 Notes for hairpin
 -----------------

diff --git a/drivers/net/mlx5/mlx5_flow_hw.c b/drivers/net/mlx5/mlx5_flow_hw.c
index 2437e44051..31e11763db 100644
--- a/drivers/net/mlx5/mlx5_flow_hw.c
+++ b/drivers/net/mlx5/mlx5_flow_hw.c
@@ -3186,8 +3186,11 @@ flow_hw_actions_construct(struct rte_eth_dev *dev,
 		case RTE_FLOW_ACTION_TYPE_COUNT:
 			cnt_queue = mlx5_hws_cnt_get_queue(priv, &queue);
 			ret = mlx5_hws_cnt_pool_get(priv->hws_cpool, cnt_queue, &cnt_id, age_idx);
-			if (ret != 0)
+			if (ret != 0) {
+				rte_flow_error_set(error, -ret, RTE_FLOW_ERROR_TYPE_ACTION,
+						action, "Failed to allocate flow counter");
 				goto error;
+			}
 			ret = mlx5_hws_cnt_pool_get_action_offset
 				(priv->hws_cpool,
 				 cnt_id,
@@ -3381,6 +3384,7 @@ flow_hw_async_flow_create(struct rte_eth_dev *dev,
 	struct rte_flow_hw *flow = NULL;
 	struct mlx5_hw_q_job *job = NULL;
 	const struct rte_flow_item *rule_items;
+	struct rte_flow_error sub_error = { 0 };
 	uint32_t flow_idx = 0;
 	uint32_t res_idx = 0;
 	int ret;
@@ -3429,7 +3433,7 @@ flow_hw_async_flow_create(struct rte_eth_dev *dev,
 	if (flow_hw_actions_construct(dev, job,
 				      &table->ats[action_template_index],
 				      pattern_template_index, actions,
-				      rule_acts, queue, error))
+				      rule_acts, queue, &sub_error))
 		goto error;
 	rule_items = flow_hw_get_rule_items(dev, table, items,
 					    pattern_template_index, job);
@@ -3448,9 +3452,12 @@ error:
 		mlx5_ipool_free(table->flow, flow_idx);
 	if (res_idx)
 		mlx5_ipool_free(table->resource, res_idx);
-	rte_flow_error_set(error, rte_errno,
-			   RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
-			   "fail to create rte flow");
+	if (sub_error.cause != RTE_FLOW_ERROR_TYPE_NONE && error != NULL)
+		*error = sub_error;
+	else
+		rte_flow_error_set(error, rte_errno,
+				   RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
+				   "fail to create rte flow");
 	return NULL;
 }

diff --git a/drivers/net/mlx5/mlx5_hws_cnt.c b/drivers/net/mlx5/mlx5_hws_cnt.c
index 41edd19bb8..7a88a4001a 100644
--- a/drivers/net/mlx5/mlx5_hws_cnt.c
+++ b/drivers/net/mlx5/mlx5_hws_cnt.c
@@ -56,26 +56,29 @@ __mlx5_hws_cnt_svc(struct mlx5_dev_ctx_shared *sh,
 	uint32_t ret __rte_unused;

 	reset_cnt_num = rte_ring_count(reset_list);
-	do {
-		cpool->query_gen++;
-		mlx5_aso_cnt_query(sh, cpool);
-		zcdr.n1 = 0;
-		zcdu.n1 = 0;
-		ret = rte_ring_enqueue_zc_burst_elem_start(reuse_list,
-							   sizeof(cnt_id_t),
-							   reset_cnt_num, &zcdu,
-							   NULL);
-		MLX5_ASSERT(ret == reset_cnt_num);
-		ret = rte_ring_dequeue_zc_burst_elem_start(reset_list,
-							   sizeof(cnt_id_t),
-							   reset_cnt_num, &zcdr,
-							   NULL);
-		MLX5_ASSERT(ret == reset_cnt_num);
-		__hws_cnt_r2rcpy(&zcdu, &zcdr, reset_cnt_num);
-		rte_ring_dequeue_zc_elem_finish(reset_list, reset_cnt_num);
-		rte_ring_enqueue_zc_elem_finish(reuse_list, reset_cnt_num);
+	cpool->query_gen++;
+	mlx5_aso_cnt_query(sh, cpool);
+	zcdr.n1 = 0;
+	zcdu.n1 = 0;
+	ret = rte_ring_enqueue_zc_burst_elem_start(reuse_list,
+						   sizeof(cnt_id_t),
+						   reset_cnt_num, &zcdu,
+						   NULL);
+	MLX5_ASSERT(ret == reset_cnt_num);
+	ret = rte_ring_dequeue_zc_burst_elem_start(reset_list,
+						   sizeof(cnt_id_t),
+						   reset_cnt_num, &zcdr,
+						   NULL);
+	MLX5_ASSERT(ret == reset_cnt_num);
+	__hws_cnt_r2rcpy(&zcdu, &zcdr, reset_cnt_num);
+	rte_ring_dequeue_zc_elem_finish(reset_list, reset_cnt_num);
+	rte_ring_enqueue_zc_elem_finish(reuse_list, reset_cnt_num);
+
+	if (rte_log_can_log(mlx5_logtype, RTE_LOG_DEBUG)) {
 		reset_cnt_num = rte_ring_count(reset_list);
-	} while (reset_cnt_num > 0);
+		DRV_LOG(DEBUG, "ibdev %s cpool %p wait_reset_cnt=%" PRIu32,
+			       sh->ibdev_name, (void *)cpool, reset_cnt_num);
+	}
 }

 /**
@@ -315,6 +318,11 @@ mlx5_hws_cnt_svc(void *opaque)
 		rte_spinlock_unlock(&sh->cpool_lock);
 		query_us = query_cycle / (rte_get_timer_hz() / US_PER_S);
 		sleep_us = interval - query_us;
+		DRV_LOG(DEBUG, "ibdev %s counter service thread: "
+			       "interval_us=%" PRIu64 " query_us=%" PRIu64 " "
+			       "sleep_us=%" PRIu64,
+			sh->ibdev_name, interval, query_us,
+			interval > query_us ? sleep_us : 0);
 		if (interval > query_us)
 			rte_delay_us_sleep(sleep_us);
 	}
--
2.34.1

---
  Diff of the applied patch vs upstream commit (please double-check if non-empty:
---
--- -	2024-12-06 23:26:46.403270900 +0800
+++ 0070-net-mlx5-fix-counter-query-loop-getting-stuck.patch	2024-12-06 23:26:44.023044827 +0800
@@ -1 +1 @@
-From c0e29968294c92ca15fdb34ce63fbba01c4562a6 Mon Sep 17 00:00:00 2001
+From 833fd897b154ab2f3d278426453a7f1514635a66 Mon Sep 17 00:00:00 2001
@@ -4,0 +5,3 @@
+Cc: Xueming Li <xuemingl@nvidia.com>
+
+[ upstream commit c0e29968294c92ca15fdb34ce63fbba01c4562a6 ]
@@ -46 +48,0 @@
-Cc: stable@dpdk.org
@@ -57 +59 @@
-index b1d6863f36..145c01fbda 100644
+index 2c59b24d78..d1c3284ca1 100644
@@ -60 +62 @@
-@@ -2021,6 +2021,77 @@ directly but neither destroyed nor flushed.
+@@ -1874,6 +1874,77 @@ directly but neither destroyed nor flushed.
@@ -139 +141 @@
-index 488ef4ce3c..6ad98d40f7 100644
+index 2437e44051..31e11763db 100644
@@ -142 +144 @@
-@@ -3734,8 +3734,11 @@ flow_hw_actions_construct(struct rte_eth_dev *dev,
+@@ -3186,8 +3186,11 @@ flow_hw_actions_construct(struct rte_eth_dev *dev,
@@ -155,2 +157 @@
-@@ -3980,6 +3983,7 @@ flow_hw_async_flow_create_generic(struct rte_eth_dev *dev,
- 	struct mlx5dr_rule_action *rule_acts;
+@@ -3381,6 +3384,7 @@ flow_hw_async_flow_create(struct rte_eth_dev *dev,
@@ -157,0 +159 @@
+ 	struct mlx5_hw_q_job *job = NULL;
@@ -163 +165,2 @@
-@@ -4037,7 +4041,7 @@ flow_hw_async_flow_create_generic(struct rte_eth_dev *dev,
+@@ -3429,7 +3433,7 @@ flow_hw_async_flow_create(struct rte_eth_dev *dev,
+ 	if (flow_hw_actions_construct(dev, job,
@@ -165,2 +168 @@
- 				      table->its[pattern_template_index]->item_flags,
- 				      flow->table, actions,
+ 				      pattern_template_index, actions,
@@ -171,4 +173,2 @@
- 					    pattern_template_index, &priv->hw_q[queue].pp);
-@@ -4074,9 +4078,12 @@ error:
- 		mlx5_ipool_free(table->resource, res_idx);
- 	if (flow_idx)
+ 					    pattern_template_index, job);
+@@ -3448,9 +3452,12 @@ error:
@@ -175,0 +176,2 @@
+ 	if (res_idx)
+ 		mlx5_ipool_free(table->resource, res_idx);
@@ -189 +191 @@
-index def0b19deb..0197c098f6 100644
+index 41edd19bb8..7a88a4001a 100644
@@ -241 +243 @@
-@@ -325,6 +328,11 @@ mlx5_hws_cnt_svc(void *opaque)
+@@ -315,6 +318,11 @@ mlx5_hws_cnt_svc(void *opaque)