From ddafceb6c2ecb83b7bdf6711ea1c30d97aeb3b8f Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Tue, 22 Apr 2014 17:26:21 -0700 Subject: [PATCH] Better port::Mutex::AssertHeld() and AssertNotHeld() Summary: Using ThreadLocalPtr as a flag to determine if a mutex is locked or not enables us to implement AssertNotHeld(). It also makes AssertHeld() actually correct. I had to remove port::Mutex as a dependency for util/thread_local.h, but that's fine since we can just use std::mutex :) Test Plan: make check Reviewers: ljin, dhruba, haobo, sdong, yhchiang Reviewed By: ljin CC: leveldb Differential Revision: https://reviews.facebook.net/D18171 --- db/db_test.cc | 2 +- port/port_posix.cc | 21 ++++++++++++++++----- port/port_posix.h | 19 ++++++++++++------- util/thread_local.cc | 33 ++++++++++++++++----------------- util/thread_local.h | 5 ++--- 5 files changed, 47 insertions(+), 33 deletions(-) diff --git a/db/db_test.cc b/db/db_test.cc index f2c665af3f..50cd2d1f4b 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -415,7 +415,7 @@ class DBTest { options.db_log_dir = test::TmpDir(); break; case kWalDir: - options.wal_dir = "/tmp/wal"; + options.wal_dir = test::TmpDir() + "/wal_dir"; break; case kManifestFileSize: options.max_manifest_file_size = 50; // 50 bytes diff --git a/port/port_posix.cc b/port/port_posix.cc index 911cebdf2d..59129eb709 100644 --- a/port/port_posix.cc +++ b/port/port_posix.cc @@ -9,11 +9,13 @@ #include "port/port_posix.h" +#include #include #include #include #include #include "util/logging.h" +#include "util/thread_local.h" namespace rocksdb { namespace port { @@ -26,6 +28,9 @@ static void PthreadCall(const char* label, int result) { } Mutex::Mutex(bool adaptive) { +#ifndef NDEBUG + locked_.reset(new ThreadLocalPtr()); +#endif #ifdef OS_LINUX if (!adaptive) { PthreadCall("init mutex", pthread_mutex_init(&mu_, NULL)); @@ -49,20 +54,26 @@ Mutex::~Mutex() { PthreadCall("destroy mutex", pthread_mutex_destroy(&mu_)); } void Mutex::Lock() { PthreadCall("lock", pthread_mutex_lock(&mu_)); #ifndef NDEBUG - locked_ = true; + locked_->Reset(this); #endif } void Mutex::Unlock() { #ifndef NDEBUG - locked_ = false; + locked_->Reset(nullptr); #endif PthreadCall("unlock", pthread_mutex_unlock(&mu_)); } void Mutex::AssertHeld() { #ifndef NDEBUG - assert(locked_); + assert(locked_->Get() == this); +#endif +} + +void Mutex::AssertNotHeld() { +#ifndef NDEBUG + assert(locked_->Get() == nullptr); #endif } @@ -75,11 +86,11 @@ CondVar::~CondVar() { PthreadCall("destroy cv", pthread_cond_destroy(&cv_)); } void CondVar::Wait() { #ifndef NDEBUG - mu_->locked_ = false; + mu_->locked_->Reset(nullptr); #endif PthreadCall("wait", pthread_cond_wait(&cv_, &mu_->mu_)); #ifndef NDEBUG - mu_->locked_ = true; + mu_->locked_->Reset(mu_); #endif } diff --git a/port/port_posix.h b/port/port_posix.h index b2d162468a..faef4d24ef 100644 --- a/port/port_posix.h +++ b/port/port_posix.h @@ -9,8 +9,7 @@ // // See port_example.h for documentation for the following types/functions. -#ifndef STORAGE_LEVELDB_PORT_PORT_POSIX_H_ -#define STORAGE_LEVELDB_PORT_PORT_POSIX_H_ +#pragma once #undef PLATFORM_IS_LITTLE_ENDIAN #if defined(OS_MACOSX) @@ -51,6 +50,7 @@ #include #endif +#include #include #include #include @@ -83,6 +83,9 @@ #endif namespace rocksdb { + +class ThreadLocalPtr; + namespace port { static const bool kLittleEndian = PLATFORM_IS_LITTLE_ENDIAN; @@ -90,6 +93,10 @@ static const bool kLittleEndian = PLATFORM_IS_LITTLE_ENDIAN; class CondVar; +// DO NOT declare this Mutex as static ever. Inside it depends on ThreadLocalPtr +// and its Lock() and Unlock() function depend on ThreadLocalPtr::StaticMeta, +// which is also declared static. We can't really control static +// deinitialization order. class Mutex { public: /* implicit */ Mutex(bool adaptive = false); @@ -97,15 +104,15 @@ class Mutex { void Lock(); void Unlock(); - // this will assert if the mutex is not locked - // it does NOT verify that mutex is held by a calling thread + void AssertHeld(); + void AssertNotHeld(); private: friend class CondVar; pthread_mutex_t mu_; #ifndef NDEBUG - bool locked_; + std::unique_ptr locked_; #endif // No copying @@ -480,5 +487,3 @@ inline bool LZ4HC_Compress(const CompressionOptions &opts, const char* input, } // namespace port } // namespace rocksdb - -#endif // STORAGE_LEVELDB_PORT_PORT_POSIX_H_ diff --git a/util/thread_local.cc b/util/thread_local.cc index 1b4220b8f8..73d0bf06a3 100644 --- a/util/thread_local.cc +++ b/util/thread_local.cc @@ -8,21 +8,23 @@ // found in the LICENSE file. See the AUTHORS file for names of contributors. #include "util/thread_local.h" + +#include + #include "util/mutexlock.h" #include "port/likely.h" - namespace rocksdb { std::unique_ptr ThreadLocalPtr::StaticMeta::inst_; -port::Mutex ThreadLocalPtr::StaticMeta::mutex_; +std::mutex ThreadLocalPtr::StaticMeta::mutex_; #if !defined(OS_MACOSX) __thread ThreadLocalPtr::ThreadData* ThreadLocalPtr::StaticMeta::tls_ = nullptr; #endif ThreadLocalPtr::StaticMeta* ThreadLocalPtr::StaticMeta::Instance() { if (UNLIKELY(inst_ == nullptr)) { - MutexLock l(&mutex_); + std::lock_guard l(mutex_); if (inst_ == nullptr) { inst_.reset(new StaticMeta()); } @@ -37,7 +39,7 @@ void ThreadLocalPtr::StaticMeta::OnThreadExit(void* ptr) { auto* inst = Instance(); pthread_setspecific(inst->pthread_key_, nullptr); - MutexLock l(&mutex_); + std::lock_guard l(mutex_); inst->RemoveThreadData(tls); // Unref stored pointers of current thread from all instances uint32_t id = 0; @@ -64,7 +66,6 @@ ThreadLocalPtr::StaticMeta::StaticMeta() : next_instance_id_(0) { } void ThreadLocalPtr::StaticMeta::AddThreadData(ThreadLocalPtr::ThreadData* d) { - mutex_.AssertHeld(); d->next = &head_; d->prev = head_.prev; head_.prev->next = d; @@ -73,7 +74,6 @@ void ThreadLocalPtr::StaticMeta::AddThreadData(ThreadLocalPtr::ThreadData* d) { void ThreadLocalPtr::StaticMeta::RemoveThreadData( ThreadLocalPtr::ThreadData* d) { - mutex_.AssertHeld(); d->next->prev = d->prev; d->prev->next = d->next; d->next = d->prev = d; @@ -93,14 +93,14 @@ ThreadLocalPtr::ThreadData* ThreadLocalPtr::StaticMeta::GetThreadLocal() { { // Register it in the global chain, needs to be done before thread exit // handler registration - MutexLock l(&mutex_); + std::lock_guard l(mutex_); inst->AddThreadData(tls_); } // Even it is not OS_MACOSX, need to register value for pthread_key_ so that // its exit handler will be triggered. if (pthread_setspecific(inst->pthread_key_, tls_) != 0) { { - MutexLock l(&mutex_); + std::lock_guard l(mutex_); inst->RemoveThreadData(tls_); } delete tls_; @@ -122,7 +122,7 @@ void ThreadLocalPtr::StaticMeta::Reset(uint32_t id, void* ptr) { auto* tls = GetThreadLocal(); if (UNLIKELY(id >= tls->entries.size())) { // Need mutex to protect entries access within ReclaimId - MutexLock l(&mutex_); + std::lock_guard l(mutex_); tls->entries.resize(id + 1); } tls->entries[id].ptr.store(ptr, std::memory_order_relaxed); @@ -132,7 +132,7 @@ void* ThreadLocalPtr::StaticMeta::Swap(uint32_t id, void* ptr) { auto* tls = GetThreadLocal(); if (UNLIKELY(id >= tls->entries.size())) { // Need mutex to protect entries access within ReclaimId - MutexLock l(&mutex_); + std::lock_guard l(mutex_); tls->entries.resize(id + 1); } return tls->entries[id].ptr.exchange(ptr, std::memory_order_relaxed); @@ -143,7 +143,7 @@ bool ThreadLocalPtr::StaticMeta::CompareAndSwap(uint32_t id, void* ptr, auto* tls = GetThreadLocal(); if (UNLIKELY(id >= tls->entries.size())) { // Need mutex to protect entries access within ReclaimId - MutexLock l(&mutex_); + std::lock_guard l(mutex_); tls->entries.resize(id + 1); } return tls->entries[id].ptr.compare_exchange_strong(expected, ptr, @@ -152,7 +152,7 @@ bool ThreadLocalPtr::StaticMeta::CompareAndSwap(uint32_t id, void* ptr, void ThreadLocalPtr::StaticMeta::Scrape(uint32_t id, autovector* ptrs, void* const replacement) { - MutexLock l(&mutex_); + std::lock_guard l(mutex_); for (ThreadData* t = head_.next; t != &head_; t = t->next) { if (id < t->entries.size()) { void* ptr = @@ -165,12 +165,11 @@ void ThreadLocalPtr::StaticMeta::Scrape(uint32_t id, autovector* ptrs, } void ThreadLocalPtr::StaticMeta::SetHandler(uint32_t id, UnrefHandler handler) { - MutexLock l(&mutex_); + std::lock_guard l(mutex_); handler_map_[id] = handler; } UnrefHandler ThreadLocalPtr::StaticMeta::GetHandler(uint32_t id) { - mutex_.AssertHeld(); auto iter = handler_map_.find(id); if (iter == handler_map_.end()) { return nullptr; @@ -179,7 +178,7 @@ UnrefHandler ThreadLocalPtr::StaticMeta::GetHandler(uint32_t id) { } uint32_t ThreadLocalPtr::StaticMeta::GetId() { - MutexLock l(&mutex_); + std::lock_guard l(mutex_); if (free_instance_ids_.empty()) { return next_instance_id_++; } @@ -190,7 +189,7 @@ uint32_t ThreadLocalPtr::StaticMeta::GetId() { } uint32_t ThreadLocalPtr::StaticMeta::PeekId() const { - MutexLock l(&mutex_); + std::lock_guard l(mutex_); if (!free_instance_ids_.empty()) { return free_instance_ids_.back(); } @@ -200,7 +199,7 @@ uint32_t ThreadLocalPtr::StaticMeta::PeekId() const { void ThreadLocalPtr::StaticMeta::ReclaimId(uint32_t id) { // This id is not used, go through all thread local data and release // corresponding value - MutexLock l(&mutex_); + std::lock_guard l(mutex_); auto unref = GetHandler(id); for (ThreadData* t = head_.next; t != &head_; t = t->next) { if (id < t->entries.size()) { diff --git a/util/thread_local.h b/util/thread_local.h index a7728ed640..25862d2d8f 100644 --- a/util/thread_local.h +++ b/util/thread_local.h @@ -10,13 +10,12 @@ #pragma once #include +#include #include #include #include #include "util/autovector.h" -#include "port/port_posix.h" -#include "util/thread_local.h" namespace rocksdb { @@ -153,7 +152,7 @@ class ThreadLocalPtr { // protect inst, next_instance_id_, free_instance_ids_, head_, // ThreadData.entries - static port::Mutex mutex_; + static std::mutex mutex_; #if !defined(OS_MACOSX) // Thread local storage static __thread ThreadData* tls_;