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 <harry.van.haaren@intel.com>
Acked-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

diff --git a/doc/guides/rel_notes/release_17_11.rst b/doc/guides/rel_notes/release_17_11.rst
index 170f4f9165bc271f85822ae55b0ef707c6da575c..6e6ba1c8620f46d7baa4592d628e5aa7b310362d 100644 (file)
--- 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 06fe6ccfe4b6499e7185e505d407f2a2d772dddc..b125859b1b8e4cae9f47c3dbc1ae2aacfb2a83e7 100644 (file)
--- 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();