MB-41510: Fix TSAN failure due to lock order inversion

Fix a TSAN failure due to a lock order inversion observed on the
mad-hatter TSAN cv machine. This was due to a potential deadlock while
acquiring locks on this and other histogram in
HdrHistogram::operator+=(). To fix this use folly::acquireLocked() to
safely take write lock and read lock on this->histogram and
other.histogram respectively. We also remove the nullptr for
this->histogram as we don't guard any other access to the histograms
pointer, in case cb_calloc() fails.

TSAN failure:
    WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=11208)
    Cycle in lock order graph: M296533874179548048 (0x000000000000) => M174936684240544688 (0x000000000000) => M296533874179548048

    Mutex M174936684240544688 acquired here while holding mutex M296533874179548048 in main thread:
    #0 AnnotateRWLockAcquired <null> (libtsan.so.0+0x00000005b63d)
    #1 folly::detail::annotate_rwlock_acquired_impl(void const volatile*, folly::annotate_rwlock_level, char const*, int) /home/couchbase/jenkins/workspace/cbdeps-platform-build-old/deps/packages/build/folly/follytsan-prefix/src/follytsan/folly/synchronization/SanitizeThread.cpp:99 (ep-engine_ep_unit_tests+0x00000139555e)
    #2 annotate_rwlock_acquired tlm/deps/folly.exploded/include/folly/synchronization/SanitizeThread.h:111 (ep-engine_ep_unit_tests+0x000001362a06)
    #3 folly::SharedMutexImpl<false, void, std::atomic, false, true>::annotateAcquired(folly::annotate_rwlock_level) tlm/deps/folly.exploded/include/folly/SharedMutex.h:726 (ep-engine_ep_unit_tests+0x000001362a06)
    #4 folly::SharedMutexImpl<false, void, std::atomic, false, true>::lock_shared() tlm/deps/folly.exploded/include/folly/SharedMutex.h:400 (ep-engine_ep_unit_tests+0x000001362a06)
    #5 folly::detail::LockTraitsImpl<folly::SharedMutexImpl<false, void, std::atomic, false, true>, (folly::detail::MutexLevel)1, false>::lock_shared(folly::SharedMutexImpl<false, void, std::atomic, false, true>&) tlm/deps/folly.exploded/include/folly/LockTraits.h:157 (ep-engine_ep_unit_tests+0x000001362a06)
    #6 std::integral_constant<bool, true> folly::LockPolicyShared::lock<folly::SharedMutexImpl<false, void, std::atomic, false, true> >(folly::SharedMutexImpl<false, void, std::atomic, false, true>&) tlm/deps/folly.exploded/include/folly/LockTraits.h:499 (ep-engine_ep_unit_tests+0x000001362a06)
    #7 folly::LockedPtrBase<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const, folly::SharedMutexImpl<false, void, std::atomic, false, true>, folly::LockPolicyShared>::LockedPtrBase(folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const*) tlm/deps/folly.exploded/include/folly/Synchronized.h:1089 (ep-engine_ep_unit_tests+0x000001362a06)
    #8 folly::LockedPtr<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const, folly::LockPolicyShared>::LockedPtr(folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const*) tlm/deps/folly.exploded/include/folly/Synchronized.h:1394 (ep-engine_ep_unit_tests+0x000001360063)
    #9 folly::SynchronizedBase<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >, (folly::detail::MutexLevel)1>::rlock() tlm/deps/folly.exploded/include/folly/Synchronized.h:140 (ep-engine_ep_unit_tests+0x000001360063)
    #10 HdrHistogram::operator+=(HdrHistogram const&) ../kv_engine/utilities/hdrhistogram.cc:83 (ep-engine_ep_unit_tests+0x000001360063)
    #11 HdrHistogramTest_aggregationTest_Test::TestBody() ../kv_engine/engines/ep/tests/module_tests/hdrhistogram_test.cc:317 (ep-engine_ep_unit_tests+0x00000106945c)
    #12 void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../googletest/googletest/src/gtest.cc:2402 (ep-engine_ep_unit_tests+0x0000013276cb)
    #13 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../googletest/googletest/src/gtest.cc:2438 (ep-engine_ep_unit_tests+0x00000132e814)
    #14 testing::Test::Run() ../googletest/googletest/src/gtest.cc:2474 (ep-engine_ep_unit_tests+0x00000131d649)
    #15 testing::TestInfo::Run() ../googletest/googletest/src/gtest.cc:2656 (ep-engine_ep_unit_tests+0x00000131d91d)
    #16 testing::TestCase::Run() ../googletest/googletest/src/gtest.cc:2774 (ep-engine_ep_unit_tests+0x00000131dac5)
    #17 testing::internal::UnitTestImpl::RunAllTests() ../googletest/googletest/src/gtest.cc:4649 (ep-engine_ep_unit_tests+0x00000131fe3e)
    #18 bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../googletest/googletest/src/gtest.cc:2402 (ep-engine_ep_unit_tests+0x000001327972)
    #19 bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../googletest/googletest/src/gtest.cc:2438 (ep-engine_ep_unit_tests+0x00000132edc8)
    #20 testing::UnitTest::Run() ../googletest/googletest/src/gtest.cc:4257 (ep-engine_ep_unit_tests+0x00000131d741)
    #21 RUN_ALL_TESTS() ../googletest/googletest/include/gtest/gtest.h:2237 (ep-engine_ep_unit_tests+0x000000d30e6a)
    #22 main ../kv_engine/engines/ep/tests/module_tests/ep_unit_tests_main.cc:144 (ep-engine_ep_unit_tests+0x000000d30e6a)

Change-Id: I7f7448627d20c753add8c92eaf1186fb350aaab0
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/137548
Reviewed-by: Dave Rigby <[email protected]>
Tested-by: Build Bot <[email protected]>

Author: rdemellow

utilities/hdrhistogram.cc

@@ -56,31 +56,43 @@ HdrHistogram::HdrHistogram(uint64_t lowestTrackableValue,
 }
 
 HdrHistogram& HdrHistogram::operator+=(const HdrHistogram& other) {
-    if (other.histogram.rlock()->get() != nullptr &&
-        other.getValueCount() > 0) {
+    /*
+     * Note: folly::acquireLockedPair() takes an exclusive lock on
+     * this->histogram as we pass it a non-const ref. Where as it takes a shared
+     * lock on other.histogram as we pass it a const ref. We can do this as we
+     * only need to write to the ptr of this->histogram. (see
+     * folly::acquireLocked() source code comments for more information regard
+     * how it takes locks)
+     */
+    auto lockPair = folly::acquireLockedPair(this->histogram, other.histogram);
+    // Don't use structured binding as this messes up clang-analyzer, analyzing
+    // the destruction of folly's locks.
+    WHistoLockedPtr& thisLock = lockPair.first;
+    ConstRHistoLockedPtr& otherLock = lockPair.second;
+    if (otherLock->get()->total_count > 0) {
         // work out if we need to resize the receiving histogram
-        uint64_t newMin = this->getMinTrackableValue();
-        uint64_t newMax = this->getMaxTrackableValue();
-        int32_t newSigfig = this->getSigFigAccuracy();
+        auto newMin = getMinTrackableValue(thisLock);
+        auto newMax = getMaxTrackableValue(thisLock);
+        auto newSigfig = getSigFigAccuracy(thisLock);
         bool resize = false;
 
-        if (other.getMinTrackableValue() < this->getMinTrackableValue()) {
-            newMin = other.getMinTrackableValue();
+        if (getMinTrackableValue(otherLock) < newMin) {
+            newMin = getMinTrackableValue(otherLock);
             resize = true;
         }
-        if (other.getMaxTrackableValue() > this->getMaxTrackableValue()) {
-            newMax = other.getMaxTrackableValue();
+        if (getMaxTrackableValue(otherLock) > newMax) {
+            newMax = getMaxTrackableValue(otherLock);
             resize = true;
         }
 
-        if (other.getSigFigAccuracy() > this->getSigFigAccuracy()) {
-            newSigfig = other.getSigFigAccuracy();
+        if (getSigFigAccuracy(otherLock) > newSigfig) {
+            newSigfig = getSigFigAccuracy(otherLock);
             resize = true;
         }
         if (resize) {
-            this->resize(newMin, newMax, newSigfig);
+            this->resize(thisLock, newMin, newMax, newSigfig);
         }
-        hdr_add(this->histogram.rlock()->get(), other.histogram.rlock()->get());
+        hdr_add(thisLock->get(), otherLock->get());
     }
     return *this;
 }
@@ -323,7 +335,8 @@ double HdrHistogram::getMean() const {
     return hdr_mean(histogram.rlock()->get()) - 1;
 }
 
-void HdrHistogram::resize(uint64_t lowestTrackableValue,
+void HdrHistogram::resize(WHistoLockedPtr& histoLockPtr,
+                          uint64_t lowestTrackableValue,
                           uint64_t highestTrackableValue,
                           int significantFigures) {
     if (lowestTrackableValue >= highestTrackableValue ||
@@ -345,6 +358,6 @@ void HdrHistogram::resize(uint64_t lowestTrackableValue,
                 &hist, // Pointer to initialise
                 cb_calloc);
 
-    hdr_add(hist, histogram.rlock()->get());
-    histogram.wlock()->reset(hist);
+    hdr_add(hist, histoLockPtr->get());
+    histoLockPtr->reset(hist);
 }

utilities/hdrhistogram.h

@@ -52,7 +52,8 @@ class HdrHistogram {
 
     using SyncHdrHistogramPtr = folly::Synchronized<
             std::unique_ptr<struct hdr_histogram, HdrDeleter>>;
-    using HistoLockedPtr = SyncHdrHistogramPtr::ConstLockedPtr;
+    using ConstRHistoLockedPtr = SyncHdrHistogramPtr::ConstRLockedPtr;
+    using WHistoLockedPtr = SyncHdrHistogramPtr::WLockedPtr;
 
 public:
     struct Iterator : public hdr_iter {
@@ -142,7 +143,7 @@ class HdrHistogram {
          * it being resized or reset while we're reading from the underlying
          * data structure.
          */
-        HistoLockedPtr histoRLockPtr;
+        ConstRHistoLockedPtr histoRLockPtr;
     };
 
     /**
@@ -320,25 +321,15 @@ class HdrHistogram {
      * @return minimum trackable value
      */
     uint64_t getMinTrackableValue() const {
-        // We subtract one from the lowest value as we have added a one offset
-        // as the underlying hdr_histogram cannot store 0 and
-        // therefore the value must be greater than or equal to 1.
-        return static_cast<uint64_t>(
-                       histogram.rlock()->get()->lowest_trackable_value) -
-               1;
+        return getMinTrackableValue(histogram.rlock());
     }
 
     /**
      * Method to get the maximum trackable value of this histogram
      * @return maximum trackable value
      */
     uint64_t getMaxTrackableValue() const {
-        // We subtract one from the lowest value as we have added a one offset
-        // as the underlying hdr_histogram cannot store 0 and
-        // therefore the value must be greater than or equal to 1.
-        return static_cast<uint64_t>(
-                       histogram.rlock()->get()->highest_trackable_value) -
-               1;
+        return getMaxTrackableValue(histogram.rlock());
     }
 
     /**
@@ -348,7 +339,7 @@ class HdrHistogram {
      * figures bing used
      */
     int getSigFigAccuracy() const {
-        return histogram.rlock()->get()->significant_figures;
+        return getSigFigAccuracy(histogram.rlock());
     }
 
     /**
@@ -358,10 +349,36 @@ class HdrHistogram {
     double getMean() const;
 
 private:
-    void resize(uint64_t lowestTrackableValue,
+    void resize(WHistoLockedPtr& histoLockPtr,
+                uint64_t lowestTrackableValue,
                 uint64_t highestTrackableValue,
                 int significantFigures);
 
+    template <class LockType>
+    static uint64_t getMinTrackableValue(const LockType& histoLockPtr) {
+        // We subtract one from the lowest value as we have added a one offset
+        // as the underlying hdr_histogram cannot store 0 and
+        // therefore the value must be greater than or equal to 1.
+        return static_cast<uint64_t>(
+                       histoLockPtr->get()->lowest_trackable_value) -
+               1;
+    }
+
+    template <class LockType>
+    static uint64_t getMaxTrackableValue(const LockType& histoLockPtr) {
+        // We subtract one from the lowest value as we have added a one offset
+        // as the underlying hdr_histogram cannot store 0 and
+        // therefore the value must be greater than or equal to 1.
+        return static_cast<uint64_t>(
+                       histoLockPtr->get()->highest_trackable_value) -
+               1;
+    }
+
+    template <class LockType>
+    static int getSigFigAccuracy(const LockType& histoLockPtr) {
+        return histoLockPtr->get()->significant_figures;
+    }
+
     /**
      * Private method used to create an iterator for the specified mode
      * @param mode the mode of the iterator to be created