MB-41510: Fix infinite loop due to HdrHistogram being reset

Currently if a HdrHistogram has its reset() method called by one thread
(T1) and is being read by another (T2) using an HdrHistoram::Iterator.
There is the race that could cause the HdrHistoram::Iterator() to enter
an infinite loop.

This occurs as HdrHistogram's getNextValue() methods use HdrHistogam_c's
hdr_iter_next() which intern uses the following function:
static bool has_next(struct hdr_iter* iter)
{
        return iter->cumulative_count < iter->total_count;
}
Which logically checks that the total count the Iterator was
initialize with is > than the sum of all the counts its read from the
buckets in the histogram. The trouble is, that the reset() method
changes the bucket counts back to 0. Thus, meaning the condition
cumulative_count >= total_count may never be met.

To fix this wrap the std::unique_ptr<struct hdr_histogram> with a
folly::Synchronized<>. This allows us to take an exclusive lock on the
pointer when performing a reset() of the histogram and allows us to
ensure that no iterator is reading from the histogram, as the iterator
takes a non exclusive read lock for the lifetime of the
HdrHistogram::Iterator struct. This also ensures that
HdrHistogram::resize() is thread safe as we now take a write lock.

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

Author: rdemellow

engines/ep/src/item_eviction.cc

@@ -69,9 +69,5 @@ uint8_t ItemEviction::convertFreqCountToNRUValue(uint8_t probCounter) {
 }
 
 void ItemEviction::copyFreqHistogram(HdrHistogram& hist) {
-    HdrHistogram::Iterator iter{
-            freqHistogram.makeLinearIterator(valueUnitsPerBucket)};
-    while (auto result = freqHistogram.getNextValueAndCount(iter)) {
-        hist.addValueAndCount(result->first, result->second);
-    }
+    hist = freqHistogram;
 }

engines/ep/tests/module_tests/hdrhistogram_test.cc

@@ -18,12 +18,9 @@
 #include "thread_gate.h"
 
 #include <folly/portability/GTest.h>
-#include <hdr_histogram.h>
-#include <optional>
-
 #include <cmath>
-#include <iomanip>
 #include <memory>
+#include <optional>
 #include <random>
 #include <thread>
 #include <utility>
@@ -468,3 +465,60 @@ TEST(HdrHistogramTest, int64MaxSizeTest) {
     // Testing any higher than this gives us garbage results back but
     // unfortunately with no way of knowing that they're garbage.
 }
+
+static std::vector<std::optional<std::pair<uint64_t, double>>> getAllValues(
+        const HdrHistogram& histo, HdrHistogram::Iterator& iter) {
+    std::vector<std::optional<std::pair<uint64_t, double>>> values;
+    while (auto pair = histo.getNextValueAndPercentile(iter)) {
+        if (!pair.has_value())
+            break;
+        values.push_back(pair);
+    }
+    return values;
+}
+
+void resetThread(HdrHistogram& histo, ThreadGate& tg) {
+    EXPECT_EQ(histo.getValueCount(), 10);
+    tg.threadUp();
+    histo.reset();
+    EXPECT_EQ(histo.getValueCount(), 0);
+}
+
+/**
+ * Test to check that if you create an iterator on a HdrHistogram object, then
+ * call reset() on it in another thread and use the iterator that the iterator
+ * doesn't end up in an infinite loop.
+ */
+TEST(HdrHistogramTest, ResetItoratorInfLoop) {
+    Hdr2sfMicroSecHistogram histogram;
+    for (int i = 0; i < 10; i++) {
+        histogram.addValue(i);
+    }
+    {
+        auto iter = histogram.getHistogramsIterator();
+        auto values = getAllValues(histogram, iter);
+        EXPECT_EQ(values.size(), 20);
+    }
+
+    std::thread thread;
+    { // Scope that holds read lock for iterator
+        auto iter2 = histogram.getHistogramsIterator();
+        /**
+         * Create thread this should start running resetThread() at some point
+         * int time will be blocked at HdrHistogram::reset() till this scope is
+         * exited and iter2 is destroyed (releasing the read lock).
+         * We will also create a ThreadGat to ensure the reset thread is runing
+         * and is about to try and get an exclusive lock before reading values
+         * from the histogram.
+         */
+        ThreadGate tg(2);
+        thread = std::thread(resetThread, std::ref(histogram), std::ref(tg));
+        tg.threadUp();
+        auto values = getAllValues(histogram, iter2);
+        EXPECT_EQ(values.size(), 20);
+    } // iter2 read lock released
+
+    // wait for resetThread() to complete
+    thread.join();
+    EXPECT_EQ(histogram.getValueCount(), 0);
+}

utilities/hdrhistogram.cc

@@ -16,8 +16,8 @@
  */
 
 #include "hdrhistogram.h"
+
 #include <folly/lang/Assume.h>
-#include <hdr_histogram.h>
 #include <nlohmann/json.hpp>
 #include <platform/cb_malloc.h>
 #include <iostream>
@@ -51,17 +51,17 @@ HdrHistogram::HdrHistogram(uint64_t lowestTrackableValue,
                 significantFigures,
                 &hist, // Pointer to initialise
                 cb_calloc);
-
-    histogram.reset(hist);
+    histogram.wlock()->reset(hist);
     defaultIterationMode = iterMode;
 }
 
 HdrHistogram& HdrHistogram::operator+=(const HdrHistogram& other) {
-    if (other.histogram != nullptr && other.getValueCount() > 0) {
+    if (other.histogram.rlock()->get() != nullptr &&
+        other.getValueCount() > 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->histogram->significant_figures;
+        int32_t newSigfig = this->getSigFigAccuracy();
         bool resize = false;
 
         if (other.getMinTrackableValue() < this->getMinTrackableValue()) {
@@ -73,92 +73,93 @@ HdrHistogram& HdrHistogram::operator+=(const HdrHistogram& other) {
             resize = true;
         }
 
-        if (other.histogram->significant_figures >
-            this->histogram->significant_figures) {
-            newSigfig = other.histogram->significant_figures;
+        if (other.getSigFigAccuracy() > this->getSigFigAccuracy()) {
+            newSigfig = other.getSigFigAccuracy();
             resize = true;
         }
         if (resize) {
             this->resize(newMin, newMax, newSigfig);
         }
-        hdr_add(this->histogram.get(), other.histogram.get());
+        hdr_add(this->histogram.rlock()->get(), other.histogram.rlock()->get());
     }
     return *this;
 }
 
 bool HdrHistogram::addValue(uint64_t v) {
     // A hdr_histogram cannot store 0, therefore we add a bias of +1.
     int64_t vBiased = v + 1;
-    return hdr_record_value(histogram.get(), vBiased);
+    return hdr_record_value(histogram.rlock()->get(), vBiased);
 }
 
 bool HdrHistogram::addValueAndCount(uint64_t v, uint64_t count) {
     // A hdr_histogram cannot store 0, therefore we add a bias of +1.
     int64_t vBiased = v + 1;
-    return hdr_record_values(histogram.get(), vBiased, count);
+    return hdr_record_values(histogram.rlock()->get(), vBiased, count);
 }
 
 uint64_t HdrHistogram::getValueCount() const {
-    return static_cast<uint64_t>(histogram->total_count);
+    return static_cast<uint64_t>(histogram.rlock()->get()->total_count);
 }
 
 uint64_t HdrHistogram::getMinValue() const {
+    // check there are values in the histogram otherwise hdr_min() will return
+    // 0 which will cause use to return a UINT64_MAX.
     if (getValueCount()) {
-        return static_cast<uint64_t>(hdr_min(histogram.get()) - 1);
+        return static_cast<uint64_t>(hdr_min(histogram.rlock()->get()) - 1);
     }
     return 0;
 }
 
 uint64_t HdrHistogram::getMaxValue() const {
+    // check there are values in the histogram otherwise hdr_max() will return
+    // 0 which will cause use to return a UINT64_MAX.
     if (getValueCount()) {
-        return static_cast<uint64_t>(hdr_max(histogram.get()) - 1);
+        return static_cast<uint64_t>(hdr_max(histogram.rlock()->get()) - 1);
     }
     return 0;
 }
 
 void HdrHistogram::reset() {
-    hdr_reset(histogram.get());
+    hdr_reset(histogram.wlock()->get());
 }
 
 uint64_t HdrHistogram::getValueAtPercentile(double percentage) const {
     // We added the bias of +1 to the input value (see
     // addValueToFreqHistogram).  Therefore need to minus the bias
     // before returning the value from the histogram.
     // Note: If the histogram is empty we just want to return 0;
-    uint64_t value = hdr_value_at_percentile(histogram.get(), percentage);
+    uint64_t value =
+            hdr_value_at_percentile(histogram.rlock()->get(), percentage);
     uint64_t result = getValueCount() > 0 ? (value - 1) : 0;
     return result;
 }
 
 HdrHistogram::Iterator HdrHistogram::makeLinearIterator(
         int64_t valueUnitsPerBucket) const {
-    HdrHistogram::Iterator iter{};
-    iter.type = Iterator::IterMode::Linear;
-    hdr_iter_linear_init(&iter, histogram.get(), valueUnitsPerBucket);
-    return iter;
+    HdrHistogram::Iterator itr(histogram, Iterator::IterMode::Linear);
+    hdr_iter_linear_init(&itr, itr.histoRLockPtr->get(), valueUnitsPerBucket);
+    return itr;
 }
 
 HdrHistogram::Iterator HdrHistogram::makeLogIterator(int64_t firstBucketWidth,
                                                      double log_base) const {
-    HdrHistogram::Iterator iter{};
-    iter.type = Iterator::IterMode::Log;
-    hdr_iter_log_init(&iter, histogram.get(), firstBucketWidth, log_base);
-    return iter;
+    HdrHistogram::Iterator itr(histogram, Iterator::IterMode::Log);
+    hdr_iter_log_init(
+            &itr, itr.histoRLockPtr->get(), firstBucketWidth, log_base);
+    return itr;
 }
 
 HdrHistogram::Iterator HdrHistogram::makePercentileIterator(
         uint32_t ticksPerHalfDist) const {
-    HdrHistogram::Iterator iter{};
-    iter.type = Iterator::IterMode::Percentiles;
-    hdr_iter_percentile_init(&iter, histogram.get(), ticksPerHalfDist);
-    return iter;
+    HdrHistogram::Iterator itr(histogram, Iterator::IterMode::Percentiles);
+    hdr_iter_percentile_init(&itr, itr.histoRLockPtr->get(), ticksPerHalfDist);
+    return itr;
 }
 
 HdrHistogram::Iterator HdrHistogram::makeRecordedIterator() const {
-    HdrHistogram::Iterator iter{};
-    iter.type = Iterator::IterMode::Recorded;
-    hdr_iter_recorded_init(&iter, histogram.get());
-    return iter;
+    HdrHistogram::Iterator itr(histogram, Iterator::IterMode::Recorded);
+    hdr_iter_recorded_init(&itr, itr.histoRLockPtr->get());
+    return itr;
 }
 
 HdrHistogram::Iterator HdrHistogram::makeIterator(
@@ -246,16 +247,16 @@ HdrHistogram::getNextValueAndPercentile(Iterator& iter) const {
 }
 
 void HdrHistogram::printPercentiles() const {
-    hdr_percentiles_print(histogram.get(), stdout, 5, 1.0, CLASSIC);
+    hdr_percentiles_print(histogram.rlock()->get(), stdout, 5, 1.0, CLASSIC);
 }
 
 void HdrHistogram::dumpLinearValues(int64_t bucketWidth) {
-    HdrHistogram::Iterator itr = makeLinearIterator(bucketWidth);
+    auto itr = makeLinearIterator(bucketWidth);
     std::cout << dumpValues(itr).str();
 }
 
 void HdrHistogram::dumpLogValues(int64_t firstBucketWidth, double log_base) {
-    HdrHistogram::Iterator itr = makeLogIterator(firstBucketWidth, log_base);
+    auto itr = makeLogIterator(firstBucketWidth, log_base);
     std::cout << dumpValues(itr).str();
 }
 
@@ -275,7 +276,7 @@ nlohmann::json HdrHistogram::to_json(Iterator::IterMode itrType) {
     nlohmann::json rootObj;
 
     // Five is the number of iteration steps per half-distance to 100%.
-    Iterator itr = makePercentileIterator(5);
+    auto itr = makePercentileIterator(5);
 
     rootObj["total"] = itr.total_count;
     // bucketsLow represents the starting value of the first bucket
@@ -302,11 +303,11 @@ std::string HdrHistogram::to_string() {
 }
 
 size_t HdrHistogram::getMemFootPrint() const {
-    return hdr_get_memory_size(histogram.get()) + sizeof(HdrHistogram);
+    return hdr_get_memory_size(histogram.rlock()->get()) + sizeof(HdrHistogram);
 }
 
 double HdrHistogram::getMean() const {
-    return hdr_mean(histogram.get()) - 1;
+    return hdr_mean(histogram.rlock()->get()) - 1;
 }
 
 void HdrHistogram::resize(uint64_t lowestTrackableValue,
@@ -331,6 +332,6 @@ void HdrHistogram::resize(uint64_t lowestTrackableValue,
                 &hist, // Pointer to initialise
                 cb_calloc);
 
-    hdr_add(hist, histogram.get());
-    histogram.reset(hist);
+    hdr_add(hist, histogram.rlock()->get());
+    histogram.wlock()->reset(hist);
 }

utilities/hdrhistogram.h

@@ -17,6 +17,7 @@
 
 #pragma once
 
+#include <folly/Synchronized.h>
 #include <nlohmann/json_fwd.hpp>
 #include <chrono>
 #include <memory>
@@ -49,6 +50,10 @@ class HdrHistogram {
         void operator()(struct hdr_histogram* val);
     };
 
+    using SyncHdrHistogramPtr = folly::Synchronized<
+            std::unique_ptr<struct hdr_histogram, HdrDeleter>>;
+    using HistoLockedPtr = SyncHdrHistogramPtr::ConstLockedPtr;
+
 public:
     struct Iterator : public hdr_iter {
         /**
@@ -77,10 +82,28 @@ class HdrHistogram {
              */
             Percentiles
         };
+
+        Iterator(const SyncHdrHistogramPtr& SyncHistoPtr,
+                 Iterator::IterMode mode)
+            : hdr_iter(), type(mode), histoRLockPtr(SyncHistoPtr.rlock()) {
+        }
+
+        Iterator(Iterator&& itr) = default;
+
         IterMode type;
         uint64_t lastVal = 0;
         uint64_t lastCumulativeCount = 0;
         bool isFirst = true;
+
+    private:
+        // allow HdrHistogram to access histoRLockPtr
+        friend class HdrHistogram;
+        /**
+         * Read lock that's held for the life time of the iterator. To prevent
+         * it being resized or reset while we're reading from the underlying
+         * data structure.
+         */
+        HistoLockedPtr histoRLockPtr;
     };
 
     /**
@@ -172,7 +195,10 @@ class HdrHistogram {
     uint64_t getMaxValue() const;
 
     /**
-     * Clears the histogram.
+     * Clears the histogram. Please not that this takes a write lock on the
+     * underlying data structure and will wait till all read locks have been
+     * released before completing and thus, could result in dead lock
+     * situations.
      */
     void reset();
 
@@ -311,7 +337,9 @@ class HdrHistogram {
         // 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->lowest_trackable_value) - 1;
+        return static_cast<uint64_t>(
+                       histogram.rlock()->get()->lowest_trackable_value) -
+               1;
     }
 
     /**
@@ -322,7 +350,9 @@ class HdrHistogram {
         // 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->highest_trackable_value) - 1;
+        return static_cast<uint64_t>(
+                       histogram.rlock()->get()->highest_trackable_value) -
+               1;
     }
 
     /**
@@ -332,7 +362,7 @@ class HdrHistogram {
      * figures bing used
      */
     int getSigFigAccuracy() const {
-        return histogram->significant_figures;
+        return histogram.rlock()->get()->significant_figures;
     }
 
     /**
@@ -359,9 +389,9 @@ class HdrHistogram {
     Iterator::IterMode defaultIterationMode;
 
     /**
-     * unique_ptr to a hdr_histogram structure
+     * Synchronized unique pointer to a struct hdr_histogram
      */
-    std::unique_ptr<struct hdr_histogram, HdrDeleter> histogram;
+    SyncHdrHistogramPtr histogram;
 };
 
 /** Histogram to store counts for microsecond intervals