Skip to content

Commit

Permalink
[#16055] YSQL: lock-order-inversion observed in pgwrapper::PgSupervisor
Browse files Browse the repository at this point in the history
Summary:
TSAN detects this possible deadlock in PgWrapper::Supervisor:

Minimal stack trace:

```
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=8513)
[ts-2]   Cycle in lock order graph: M0 (0x7b5400002a48) => M1 (0x7b1800051f18) => M0

Mutex M1 acquired here while holding mutex M0 in main thread:
[ts-2]     #0 pthread_mutex_lock /opt/yb-build/llvm/yb-llvm-v15.0.3-yb-1-1667341687-0b8d1183-centos7-x86_64-build/src/llvm-project/compiler-rt/lib/tsan/rtl/../../sanitizer_common/sanitizer_common_interceptors.inc:4457:3 (yb-tserver+0xa4cfa)
[ts-2]     #3 yb::FlagsCallbackRegistry::RegisterCallback(void const*, string const&, std::function<void ()>) ${BUILD_ROOT}/../../src/yb/util/flags/flags_callback.cc:84:19 (libyb_util.so+0x24dcd9)
[ts-2]     #4 yb::RegisterFlagUpdateCallback(void const*, string const&, std::function<void ()>) ${BUILD_ROOT}/../../src/yb/util/flags/flags_callback.cc:184:24 (libyb_util.so+0x24f8c3)
[ts-2]     #5 yb::pgwrapper::PgSupervisor::RegisterReloadPgConfigCallback(void const*) ${BUILD_ROOT}/../../src/yb/yql/pgwrapper/pg_wrapper.cc:904:32 (libyb_pgwrapper.so+0x2f2f4)
[ts-2]     #6 yb::pgwrapper::PgSupervisor::RegisterPgFlagChangeNotifications() ${BUILD_ROOT}/../../src/yb/yql/pgwrapper/pg_wrapper.cc:919:7 (libyb_pgwrapper.so+0x2e69e)
[ts-2]     #7 yb::pgwrapper::PgSupervisor::Start() ${BUILD_ROOT}/../../src/yb/yql/pgwrapper/pg_wrapper.cc:749:3 (libyb_pgwrapper.so+0x2ce05)

Mutex M0 previously acquired by the same thread here:
[ts-2]     #0 pthread_mutex_lock /opt/yb-build/llvm/yb-llvm-v15.0.3-yb-1-1667341687-0b8d1183-centos7-x86_64-build/src/llvm-project/compiler-rt/lib/tsan/rtl/../../sanitizer_common/sanitizer_common_interceptors.inc:4457:3 (yb-tserver+0xa4cfa)
[ts-2]     #3 yb::pgwrapper::PgSupervisor::Start() ${BUILD_ROOT}/../../src/yb/yql/pgwrapper/pg_wrapper.cc:746:31 (libyb_pgwrapper.so+0x2cdaf)

Mutex M0 acquired here while holding mutex M1 in thread T66:
[ts-2]     #0 pthread_mutex_lock /opt/yb-build/llvm/yb-llvm-v15.0.3-yb-1-1667341687-0b8d1183-centos7-x86_64-build/src/llvm-project/compiler-rt/lib/tsan/rtl/../../sanitizer_common/sanitizer_common_interceptors.inc:4457:3 (yb-tserver+0xa4cfa)
[ts-2]     #3 yb::pgwrapper::PgSupervisor::UpdateAndReloadConfig() ${BUILD_ROOT}/../../src/yb/yql/pgwrapper/pg_wrapper.cc:894:31 (libyb_pgwrapper.so+0x2f1b7)

```
TSAN detects that there are two functions (Start() and UpdateAndReloadConfig)
each acquiring M0 and M1 in inverse order which may run into a deadlock. However, Start() is always
called first and will acquire M0 and M1 before it registers the callback that invokes
UpdateAndReloadConfig(). Start() will never be called again. Thus the deadlock
called out by TSAN is not possible.
Jira: DB-5450

Test Plan: Tested with TSAN build in Detective runs of D22913

Reviewers: hsunder, mbautin

Reviewed By: hsunder

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D25311
  • Loading branch information
deeps1991 committed Jun 8, 2023
1 parent 6320b2c commit 9408c04
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 0 deletions.
15 changes: 15 additions & 0 deletions src/yb/util/debug/sanitizer_scopes.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,5 +54,20 @@ class ScopedTSANIgnoreReadsAndWrites {
DISALLOW_COPY_AND_ASSIGN(ScopedTSANIgnoreReadsAndWrites);
};

// Scope guard which instructs TSAN to ignore all synchronization events
// on the current thread as long as it is alive. These may be safely
// nested.
class ScopedTSANIgnoreSync {
public:
ScopedTSANIgnoreSync() {
ANNOTATE_IGNORE_SYNC_BEGIN();
}
~ScopedTSANIgnoreSync() {
ANNOTATE_IGNORE_SYNC_END();
}
private:
DISALLOW_COPY_AND_ASSIGN(ScopedTSANIgnoreSync);
};

} // namespace debug
} // namespace yb
8 changes: 8 additions & 0 deletions src/yb/yql/pgwrapper/pg_wrapper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <boost/algorithm/string.hpp>

#include "yb/tserver/tablet_server_interface.h"
#include "yb/util/debug/sanitizer_scopes.h"
#include "yb/util/env_util.h"
#include "yb/util/errno.h"
#include "yb/util/flags.h"
Expand Down Expand Up @@ -988,6 +989,13 @@ Status PgSupervisor::ReloadConfig() {
}

Status PgSupervisor::UpdateAndReloadConfig() {
// See GHI #16055. TSAN detects that Start() and UpdateAndReloadConfig each acquire M0 and M1 in
// inverse order which may run into a deadlock. However, Start() is always called first and will
// acquire M0 and M1 before it registers the callback UpdateAndReloadConfig() and will never
// be called again. Thus the deadlock called out by TSAN is not possible. Silence TSAN warnings
// from this function to prevent spurious failures.
debug::ScopedTSANIgnoreSync ignore_sync;
debug::ScopedTSANIgnoreReadsAndWrites ignore_reads_and_writes;
std::lock_guard<std::mutex> lock(mtx_);
if (pg_wrapper_) {
return pg_wrapper_->UpdateAndReloadConfig();
Expand Down

0 comments on commit 9408c04

Please sign in to comment.