From 3cf5eb1546ed83ee1b5c3fe8ca9d5274ffe4bd59 Mon Sep 17 00:00:00 2001 From: Harry van Haaren Date: Mon, 21 Aug 2017 13:58:09 +0100 Subject: [PATCH] service: fix and refactor atomic service accesses This commit fixes an issue in the service runner function, where the atomic value was not cleared on exiting the service function. This resulted in future attempts to run the service to appear like the function was running, however it was in reality deadlocked. This commit refactors the atomic handling to be more readable, by splitting the implementation code into a new static inline function. The remaining flow control of atomics in the existing function is refactored for readability. Fixes: 21698354c832 ("service: introduce service cores concept") Signed-off-by: Harry van Haaren Acked-by: Pavan Nikhilesh --- doc/guides/rel_notes/release_17_11.rst | 7 ++++ lib/librte_eal/common/rte_service.c | 45 +++++++++++++++----------- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/doc/guides/rel_notes/release_17_11.rst b/doc/guides/rel_notes/release_17_11.rst index 170f4f9165..6e6ba1c862 100644 --- a/doc/guides/rel_notes/release_17_11.rst +++ b/doc/guides/rel_notes/release_17_11.rst @@ -65,6 +65,13 @@ Resolved Issues EAL ~~~ +* **Service core fails to call service callback due to atomic lock** + + In a specific configuration of multi-thread unsafe services and service + cores, a service core previously did not correctly release the atomic lock + on the service. This would result in the cores polling the service, but it + looked like another thread was executing the service callback. The logic for + atomic locking of the services has been fixed and refactored for readability. Drivers ~~~~~~~ diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c index 06fe6ccfe4..b125859b1b 100644 --- a/lib/librte_eal/common/rte_service.c +++ b/lib/librte_eal/common/rte_service.c @@ -296,6 +296,23 @@ rte_service_runstate_get(uint32_t id) return (s->runstate == RUNSTATE_RUNNING) && (s->num_mapped_cores > 0); } +static inline void +rte_service_runner_do_callback(struct rte_service_spec_impl *s, + struct core_state *cs, uint32_t service_idx) +{ + void *userdata = s->spec.callback_userdata; + + if (service_stats_enabled(s)) { + uint64_t start = rte_rdtsc(); + s->spec.callback(userdata); + uint64_t end = rte_rdtsc(); + s->cycles_spent += end - start; + cs->calls_per_service[service_idx]++; + s->calls++; + } else + s->spec.callback(userdata); +} + static int32_t rte_service_runner_func(void *arg) { @@ -315,26 +332,16 @@ rte_service_runner_func(void *arg) /* check do we need cmpset, if MT safe or <= 1 core * mapped, atomic ops are not required. */ - const int need_cmpset = !((service_mt_safe(s) == 0) && - (s->num_mapped_cores > 1)); - uint32_t *lock = (uint32_t *)&s->execute_lock; - - if (need_cmpset || rte_atomic32_cmpset(lock, 0, 1)) { - void *userdata = s->spec.callback_userdata; - - if (service_stats_enabled(s)) { - uint64_t start = rte_rdtsc(); - s->spec.callback(userdata); - uint64_t end = rte_rdtsc(); - s->cycles_spent += end - start; - cs->calls_per_service[i]++; - s->calls++; - } else - s->spec.callback(userdata); - - if (need_cmpset) + const int use_atomics = (service_mt_safe(s) == 0) && + (s->num_mapped_cores > 1); + if (use_atomics) { + uint32_t *lock = (uint32_t *)&s->execute_lock; + if (rte_atomic32_cmpset(lock, 0, 1)) { + rte_service_runner_do_callback(s, cs, i); rte_atomic32_clear(&s->execute_lock); - } + } + } else + rte_service_runner_do_callback(s, cs, i); } rte_smp_rmb(); -- 2.20.1