patch 'service: fix stats race condition for MT safe service' has been queued to stable release 20.11.7

[luca.boccassi at gmail.com]

Hi,

FYI, your patch has been queued to stable release 20.11.7

Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet.
It will be pushed if I get no objections before 11/05/22. 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://github.com/kevintraynor/dpdk-stable

This queued commit can be viewed at:
https://github.com/kevintraynor/dpdk-stable/commit/027b9bcee35794f6cf27ad8399fbcbaf37d9b2c0

Thanks.

Luca Boccassi

---
>From 027b9bcee35794f6cf27ad8399fbcbaf37d9b2c0 Mon Sep 17 00:00:00 2001
From: Harry van Haaren <harry.van.haaren at intel.com>
Date: Mon, 11 Jul 2022 13:18:25 +0000
Subject: [PATCH] service: fix stats race condition for MT safe service
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

[ upstream commit 99e4e840479ab44ffb35177e22c9999fec64b848 ]

This commit fixes a potential racey-add that could occur if
multiple service-lcores were executing the same MT-safe service
at the same time, with service statistics collection enabled.

Because multiple threads can run and execute the service, the
stats values can have multiple writer threads, resulting in the
requirement of using atomic addition for correctness.

Note that when a MT unsafe service is executed, a spinlock is
held, so the stats increments are protected. This fact is used
to avoid executing atomic add instructions when not required.
Regular reads and increments are used, and only the store is
specified as atomic, reducing perf impact on e.g. x86 arch.

This patch causes a 1.25x increase in cycle-cost for polling a
MT safe service when statistics are enabled. No change was seen
for MT unsafe services, or when statistics are disabled.

Fixes: 21698354c832 ("service: introduce service cores concept")

Reported-by: Mattias Rönnblom <mattias.ronnblom at ericsson.com>
Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli at arm.com>
Suggested-by: Morten Brørup <mb at smartsharesystems.com>
Suggested-by: Bruce Richardson <bruce.richardson at intel.com>
Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
---
 lib/librte_eal/common/rte_service.c | 31 +++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index e76c2baffc..d5e3574ec7 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -56,10 +56,17 @@ struct rte_service_spec_impl {
 	 * on currently.
 	 */
 	uint32_t num_mapped_cores;
-	uint64_t calls;
-	uint64_t cycles_spent;
+
+	/* 32-bit builds won't naturally align a uint64_t, so force alignment,
+	 * allowing regular reads to be atomic.
+	 */
+	uint64_t calls __rte_aligned(8);
+	uint64_t cycles_spent __rte_aligned(8);
 } __rte_cache_aligned;
 
+/* Mask used to ensure uint64_t 8 byte vars are naturally aligned. */
+#define RTE_SERVICE_STAT_ALIGN_MASK (8 - 1)
+
 /* the internal values of a service core */
 struct core_state {
 	/* map of services IDs are run on this core */
@@ -365,13 +372,29 @@ service_runner_do_callback(struct rte_service_spec_impl *s,
 {
 	void *userdata = s->spec.callback_userdata;
 
+	/* Ensure the atomically stored variables are naturally aligned,
+	 * as required for regular loads to be atomic.
+	 */
+	RTE_BUILD_BUG_ON((offsetof(struct rte_service_spec_impl, calls)
+		& RTE_SERVICE_STAT_ALIGN_MASK) != 0);
+	RTE_BUILD_BUG_ON((offsetof(struct rte_service_spec_impl, cycles_spent)
+		& RTE_SERVICE_STAT_ALIGN_MASK) != 0);
+
 	if (service_stats_enabled(s)) {
 		uint64_t start = rte_rdtsc();
 		s->spec.callback(userdata);
 		uint64_t end = rte_rdtsc();
-		s->cycles_spent += end - start;
+		uint64_t cycles = end - start;
 		cs->calls_per_service[service_idx]++;
-		s->calls++;
+		if (service_mt_safe(s)) {
+			__atomic_fetch_add(&s->cycles_spent, cycles, __ATOMIC_RELAXED);
+			__atomic_fetch_add(&s->calls, 1, __ATOMIC_RELAXED);
+		} else {
+			uint64_t cycles_new = s->cycles_spent + cycles;
+			uint64_t calls_new = s->calls++;
+			__atomic_store_n(&s->cycles_spent, cycles_new, __ATOMIC_RELAXED);
+			__atomic_store_n(&s->calls, calls_new, __ATOMIC_RELAXED);
+		}
 	} else
 		s->spec.callback(userdata);
 }
-- 
2.34.1

---
  Diff of the applied patch vs upstream commit (please double-check if non-empty:
---
--- -	2022-11-03 09:27:29.365045661 +0000
+++ 0066-service-fix-stats-race-condition-for-MT-safe-service.patch	2022-11-03 09:27:25.485424608 +0000
@@ -1 +1 @@
-From 99e4e840479ab44ffb35177e22c9999fec64b848 Mon Sep 17 00:00:00 2001
+From 027b9bcee35794f6cf27ad8399fbcbaf37d9b2c0 Mon Sep 17 00:00:00 2001
@@ -8,0 +9,2 @@
+[ upstream commit 99e4e840479ab44ffb35177e22c9999fec64b848 ]
+
@@ -35 +37 @@
- lib/eal/common/rte_service.c | 31 +++++++++++++++++++++++++++----
+ lib/librte_eal/common/rte_service.c | 31 +++++++++++++++++++++++++----
@@ -38,5 +40,5 @@
-diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c
-index d2b7275ac0..94cb056196 100644
---- a/lib/eal/common/rte_service.c
-+++ b/lib/eal/common/rte_service.c
-@@ -50,10 +50,17 @@ struct rte_service_spec_impl {
+diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
+index e76c2baffc..d5e3574ec7 100644
+--- a/lib/librte_eal/common/rte_service.c
++++ b/lib/librte_eal/common/rte_service.c
+@@ -56,10 +56,17 @@ struct rte_service_spec_impl {
@@ -62 +64 @@
-@@ -359,13 +366,29 @@ service_runner_do_callback(struct rte_service_spec_impl *s,
+@@ -365,13 +372,29 @@ service_runner_do_callback(struct rte_service_spec_impl *s,