From 5eeb913d6cff9cfe9a6769d7efe4a7b9f23de0f4 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 8 Apr 2016 22:14:19 +0200 Subject: [PATCH] Clean up lockorder data of destroyed mutexes The lockorder potential deadlock detection works by remembering for each lock A that is acquired while holding another B the pair (A,B), and triggering a warning when (B,A) already exists in the table. A and B in the above text are represented by pointers to the CCriticalSection object that is acquired. This does mean however that we need to clean up the table entries that refer to any critical section which is destroyed, as it memory address can potentially be used for another unrelated lock in the future. Implement this clean up by remembering not only the pairs in forward direction, but also backward direction. This allows for fast iteration over all pairs that use a deleted CCriticalSection in either the first or the second position. --- src/sync.cpp | 55 +++++++++++++++++++++++++++++++++++++++++----------- src/sync.h | 33 +++++++++++++++++++------------ 2 files changed, 65 insertions(+), 23 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index 8df8ae43f4..641ed2c8ca 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -56,11 +56,24 @@ private: }; typedef std::vector > LockStack; +typedef std::map, LockStack> LockOrders; +typedef std::set > InvLockOrders; -static boost::mutex dd_mutex; -static std::map, LockStack> lockorders; -static boost::thread_specific_ptr lockstack; +struct LockData { + // Very ugly hack: as the global constructs and destructors run single + // threaded, we use this boolean to know whether LockData still exists, + // as DeleteLock can get called by global CCriticalSection destructors + // after LockData disappears. + bool available; + LockData() : available(true) {} + ~LockData() { available = false; } + LockOrders lockorders; + InvLockOrders invlockorders; + boost::mutex dd_mutex; +} static lockdata; + +boost::thread_specific_ptr lockstack; static void potential_deadlock_detected(const std::pair& mismatch, const LockStack& s1, const LockStack& s2) { @@ -117,7 +130,7 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) if (lockstack.get() == NULL) lockstack.reset(new LockStack); - dd_mutex.lock(); + boost::unique_lock lock(lockdata.dd_mutex); (*lockstack).push_back(std::make_pair(c, locklocation)); @@ -127,23 +140,21 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) break; std::pair p1 = std::make_pair(i.first, c); - if (lockorders.count(p1)) + if (lockdata.lockorders.count(p1)) continue; - lockorders[p1] = (*lockstack); + lockdata.lockorders[p1] = (*lockstack); std::pair p2 = std::make_pair(c, i.first); - if (lockorders.count(p2)) - potential_deadlock_detected(p1, lockorders[p2], lockorders[p1]); + lockdata.invlockorders.insert(p2); + if (lockdata.lockorders.count(p2)) + potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]); } } - dd_mutex.unlock(); } static void pop_lock() { - dd_mutex.lock(); (*lockstack).pop_back(); - dd_mutex.unlock(); } void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry) @@ -173,4 +184,26 @@ void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, abort(); } +void DeleteLock(void* cs) +{ + if (!lockdata.available) { + // We're already shutting down. + return; + } + boost::unique_lock lock(lockdata.dd_mutex); + std::pair item = std::make_pair(cs, (void*)0); + LockOrders::iterator it = lockdata.lockorders.lower_bound(item); + while (it != lockdata.lockorders.end() && it->first.first == cs) { + std::pair invitem = std::make_pair(it->first.second, it->first.first); + lockdata.invlockorders.erase(invitem); + lockdata.lockorders.erase(it++); + } + InvLockOrders::iterator invit = lockdata.invlockorders.lower_bound(item); + while (invit != lockdata.invlockorders.end() && invit->first == cs) { + std::pair invinvitem = std::make_pair(invit->second, invit->first); + lockdata.lockorders.erase(invinvitem); + lockdata.invlockorders.erase(invit++); + } +} + #endif /* DEBUG_LOCKORDER */ diff --git a/src/sync.h b/src/sync.h index 34dd8c228e..0c58fb6b4e 100644 --- a/src/sync.h +++ b/src/sync.h @@ -71,30 +71,39 @@ public: } }; -/** - * Wrapped boost mutex: supports recursive locking, but no waiting - * TODO: We should move away from using the recursive lock by default. - */ -typedef AnnotatedMixin CCriticalSection; - -/** Wrapped boost mutex: supports waiting but not recursive locking */ -typedef AnnotatedMixin CWaitableCriticalSection; - -/** Just a typedef for boost::condition_variable, can be wrapped later if desired */ -typedef boost::condition_variable CConditionVariable; - #ifdef DEBUG_LOCKORDER void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false); void LeaveCritical(); std::string LocksHeld(); void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs); +void DeleteLock(void* cs); #else void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {} void static inline LeaveCritical() {} void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {} +void static inline DeleteLock(void* cs) {} #endif #define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs) +/** + * Wrapped boost mutex: supports recursive locking, but no waiting + * TODO: We should move away from using the recursive lock by default. + */ +class CCriticalSection : public AnnotatedMixin +{ +public: + ~CCriticalSection() { + DeleteLock((void*)this); + } +}; + +typedef CCriticalSection CDynamicCriticalSection; +/** Wrapped boost mutex: supports waiting but not recursive locking */ +typedef AnnotatedMixin CWaitableCriticalSection; + +/** Just a typedef for boost::condition_variable, can be wrapped later if desired */ +typedef boost::condition_variable CConditionVariable; + #ifdef DEBUG_LOCKCONTENTION void PrintLockContention(const char* pszName, const char* pszFile, int nLine); #endif