From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id E289B7D0F for ; Mon, 21 Aug 2017 14:58:34 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga105.fm.intel.com with ESMTP; 21 Aug 2017 05:58:34 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,409,1498546800"; d="scan'208";a="1208383314" Received: from silpixa00398672.ir.intel.com ([10.237.223.128]) by fmsmga002.fm.intel.com with ESMTP; 21 Aug 2017 05:58:33 -0700 From: Harry van Haaren To: dev@dpdk.org Cc: Harry van Haaren Date: Mon, 21 Aug 2017 13:58:09 +0100 Message-Id: <1503320296-51122-9-git-send-email-harry.van.haaren@intel.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1503320296-51122-1-git-send-email-harry.van.haaren@intel.com> References: <1502800360-15782-1-git-send-email-harry.van.haaren@intel.com> <1503320296-51122-1-git-send-email-harry.van.haaren@intel.com> Subject: [dpdk-dev] [PATCH v2 08/15] service: fix and refactor atomic service accesses X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Aug 2017 12:58:35 -0000 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 --- 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 170f4f9..6e6ba1c 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 1bfd149..6291c0c 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.7.4