Repository: kudu Updated Branches: refs/heads/master 2a5a12fbd -> 57fe0c3db
KUDU-2291 (part 8): fix a TSAN issue with libunwind initialization libunwind uses double-checked locking for initialization, which isn't technically safe. We previously tried to work around this by calling into the stack trace library before starting any kudu::Threads, but that still left us open to races in unit tests like rw_mutex-test which uses std::thread. This patch changes the forced single-threaded initialization to use GoogleOnce instead. Prior to this patch, looping rw_mutex-test on TSAN failed 12/1000 times. With the patch it passed 1000/1000. Change-Id: I522b6553e9cb9a30d7106ff55ad119f7df1f949c Reviewed-on: http://gerrit.cloudera.org:8080/9409 Tested-by: Kudu Jenkins Reviewed-by: Mike Percy <mpe...@apache.org> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/20ba3c7b Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/20ba3c7b Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/20ba3c7b Branch: refs/heads/master Commit: 20ba3c7ba6c64c4a1521e7aab06a99a93cc01d45 Parents: 2a5a12f Author: Todd Lipcon <t...@apache.org> Authored: Thu Feb 22 16:59:38 2018 -0800 Committer: Todd Lipcon <t...@apache.org> Committed: Fri Feb 23 20:42:19 2018 +0000 ---------------------------------------------------------------------- src/kudu/util/debug-util.cc | 26 ++++++++++++++++++++++++++ src/kudu/util/thread.cc | 5 ----- 2 files changed, 26 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/20ba3c7b/src/kudu/util/debug-util.cc ---------------------------------------------------------------------- diff --git a/src/kudu/util/debug-util.cc b/src/kudu/util/debug-util.cc index 0af7abe..5a142bc 100644 --- a/src/kudu/util/debug-util.cc +++ b/src/kudu/util/debug-util.cc @@ -44,10 +44,12 @@ #include <libunwind.h> #endif +#include "kudu/gutil/basictypes.h" #include "kudu/gutil/dynamic_annotations.h" #include "kudu/gutil/hash/city.h" #include "kudu/gutil/linux_syscall_support.h" #include "kudu/gutil/macros.h" +#include "kudu/gutil/once.h" #include "kudu/gutil/spinlock.h" #include "kudu/gutil/stringprintf.h" #include "kudu/gutil/strings/numbers.h" @@ -305,6 +307,19 @@ bool InitSignalHandlerUnlocked(int signum) { return state == INITIALIZED; } +#ifdef __linux__ +GoogleOnceType g_prime_libunwind_once; + +void PrimeLibunwind() { + // The first call into libunwind does some unsafe double-checked locking + // for initialization. So, we make sure that the first call is not concurrent + // with any other call. + unw_cursor_t cursor; + unw_context_t uc; + unw_getcontext(&uc); + RAW_CHECK(unw_init_local(&cursor, &uc) >= 0, "unw_init_local failed"); +} +#endif } // anonymous namespace Status SetStackTraceSignal(int signum) { @@ -392,6 +407,15 @@ Status StackTraceCollector::TriggerAsync(int64_t tid, StackTrace* stack) { return Status::NotSupported("unable to take thread stack: signal handler unavailable"); } } + // Ensure that libunwind is primed for use before we send any signals. Otherwise + // we can hit a deadlock with the following stack: + // GoogleOnceInit() [waits on the 'once' to finish, but will never finish] + // StackTrace::Collect() + // <signal handler> + // PrimeLibUnwind + // GoogleOnceInit() [not yet initted, so starts initializing] + // StackTrace::Collect() + GoogleOnceInit(&g_prime_libunwind_once, &PrimeLibunwind); std::unique_ptr<SignalData> data(new SignalData()); // Set the target TID in our communication structure, so if we end up with any @@ -545,6 +569,8 @@ void StackTrace::Collect(int skip_frames) { const int kMaxDepth = arraysize(frames_); #ifdef __linux__ + GoogleOnceInit(&g_prime_libunwind_once, &PrimeLibunwind); + unw_cursor_t cursor; unw_context_t uc; unw_getcontext(&uc); http://git-wip-us.apache.org/repos/asf/kudu/blob/20ba3c7b/src/kudu/util/thread.cc ---------------------------------------------------------------------- diff --git a/src/kudu/util/thread.cc b/src/kudu/util/thread.cc index b5db351..3275e6c 100644 --- a/src/kudu/util/thread.cc +++ b/src/kudu/util/thread.cc @@ -41,7 +41,6 @@ #include <glog/logging.h> #include "kudu/gutil/atomicops.h" -#include "kudu/gutil/basictypes.h" #include "kudu/gutil/bind.h" #include "kudu/gutil/bind_helpers.h" #include "kudu/gutil/dynamic_annotations.h" @@ -49,7 +48,6 @@ #include "kudu/gutil/once.h" #include "kudu/gutil/port.h" #include "kudu/gutil/strings/substitute.h" -#include "kudu/util/debug-util.h" #include "kudu/util/flag_tags.h" #include "kudu/util/kernel_stack_watchdog.h" #include "kudu/util/logging.h" @@ -407,9 +405,6 @@ void ThreadMgr::ThreadPathHandler(const WebCallbackRegistry::WebRequest& req, } static void InitThreading() { - // Warm up the stack trace library. This avoids a race in libunwind initialization - // by making sure we initialize it before we start any other threads. - ignore_result(GetStackTraceHex()); thread_manager.reset(new ThreadMgr()); }
