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.
Reported-by: Mattias Rönnblom <mattias.ronnb...@ericsson.com>
Suggested-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
Suggested-by: Morten Brørup <m...@smartsharesystems.com>
Suggested-by: Bruce Richardson <bruce.richard...@intel.com>
Signed-off-by: Harry van Haaren <harry.van.haa...@intel.com>
---
v2 (Thanks Honnappa, Morten, Bruce & Mattias for discussion):
- Improved handling of stat stores to ensure they're atomic by
using __atomic_store_n() with regular loads/increments.
- Added BUILD_BUG_ON alignment checks for the uint64_t stats
variables, tested with __rte_packed to ensure build breaks
if not aligned naturally.
---
lib/eal/common/rte_service.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c
index d2b7275ac0..90d12032f0 100644
--- a/lib/eal/common/rte_service.c
+++ b/lib/eal/common/rte_service.c
@@ -54,6 +54,9 @@ struct rte_service_spec_impl {
uint64_t cycles_spent;
} __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 */
@@ -359,13 +362,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.32.0
