MB-35413 Fix data race in Timings::get_or_create_timing_histogram

Fix data race in Timings::get_or_create_timing_histogram() to prevent the data race were in one thread we might not observe correctly the pointer to a given Hdr1sfMicroSecHistogram stored by the timings array. Which could lead to the Hdr1sfMicroSecHistogram being allocated multiple times. WARNING: ThreadSanitizer: data race (pid=1085) Read of size 8 at 0x0000009bad18 by thread T7 (mutexes: write M2377): #0 std::__uniq_ptr_impl<Hdr1sfMicroSecHistogram, std::default_delete<Hdr1sfMicroSecHistogram> >::_M_ptr() const /usr/local/include/c++/7.3.0/bits/unique_ptr.h:147 (memcached+0x0000004bf317) #1 std::unique_ptr<Hdr1sfMicroSecHistogram, std::default_delete<Hdr1sfMicroSecHistogram> >::get() const /usr/local/include/c++/7.3.0/bits/unique_ptr.h:337 (memcached+0x0000004bf317) #2 std::unique_ptr<Hdr1sfMicroSecHistogram, std::default_delete<Hdr1sfMicroSecHistogram> >::operator bool() const /usr/local/include/c++/7.3.0/bits/unique_ptr.h:351 (memcached+0x0000004bf317) #3 bool std::operator==<Hdr1sfMicroSecHistogram, std::default_delete<Hdr1sfMicroSecHistogram> >(std::unique_ptr<Hdr1sfMicroSecHistogram, std::default_delete<Hdr1sfMicroSecHistogram> > const&, decltype(nullptr)) /usr/local/include/c++/7.3.0/bits/unique_ptr.h:690 (memcached+0x0000004bf317) #4 Timings::get_or_create_timing_histogram(unsigned char) /home/couchbase/couchbase/kv_engine/daemon/timings.cc:146 (memcached+0x0000004bf317) #5 Timings::collect(cb::mcbp::ClientOpcode, std::chrono::duration<long, std::ratio<1l, 1000000000l> >) /home/couchbase/couchbase/kv_engine/daemon/timings.cc:42 (memcached+0x0000004bf4aa) ... Previous write of size 8 at 0x0000009bad18 by thread T8 (mutexes: write M2371, write M2392): #0 std::enable_if<std::__and_<std::__not_<std::__is_tuple_like<Hdr1sfMicroSecHistogram*> >, std::is_move_constructible<Hdr1sfMicroSecHistogram*>, std::is_move_assignable<Hdr1sfMicroSecHistogram*> >::value, void>::type std::swap<Hdr1sfMicroSecHistogram*>(Hdr1sfMicroSecHistogram*&, Hdr1sfMicroSecHistogram*&) /usr/local/include/c++/7.3.0/bits/move.h:199 (memcached+0x0000004bf3cf) #1 std::unique_ptr<Hdr1sfMicroSecHistogram, std::default_delete<Hdr1sfMicroSecHistogram> >::reset(Hdr1sfMicroSecHistogram*) /usr/local/include/c++/7.3.0/bits/unique_ptr.h:374 (memcached+0x0000004bf3cf) #2 std::unique_ptr<Hdr1sfMicroSecHistogram, std::default_delete<Hdr1sfMicroSecHistogram> >::operator=(std::unique_ptr<Hdr1sfMicroSecHistogram, std::default_delete<Hdr1sfMicroSecHistogram> >&&) /usr/local/include/c++/7.3.0/bits/unique_ptr.h:283 (memcached+0x0000004bf3cf) #3 Timings::get_or_create_timing_histogram(unsigned char) /home/couchbase/couchbase/kv_engine/daemon/timings.cc:149 (memcached+0x0000004bf3cf) #4 Timings::collect(cb::mcbp::ClientOpcode, std::chrono::duration<long, std::ratio<1l, 1000000000l> >) /home/couchbase/couchbase/kv_engine/daemon/timings.cc:42 (memcached+0x0000004bf4aa) ... Change-Id: I34db63854a1909cbf43a9feb273a13dfa40f313d Reviewed-on: http://review.couchbase.org/113024 Reviewed-by: James Harrison <[email protected]> Tested-by: Build Bot <[email protected]> Reviewed-by: Dave Rigby <[email protected]>
couchbase · Aug 8, 2019 · d4cb889 · d4cb889
1 parent 5c6b3ba
commit d4cb889
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 13 deletions.
diff --git a/daemon/timings.cc b/daemon/timings.cc
@@ -21,10 +21,20 @@ Timings::Timings() {
     reset();
 }
 
-void Timings::reset() {
+Timings::~Timings() {
+    std::lock_guard<std::mutex> lg(histogram_mutex);
     for (auto& t : timings) {
-        if (t) {
-            t->reset();
+        delete t;
+    }
+}
+
+void Timings::reset() {
+    {
+        std::lock_guard<std::mutex> lg(histogram_mutex);
+        for (auto& t : timings) {
+            if (t) {
+                t.load()->reset();
+            }
         }
     }
 
@@ -50,7 +60,7 @@ void Timings::collect(cb::mcbp::ClientOpcode opcode,
 std::string Timings::generate(cb::mcbp::ClientOpcode opcode) {
     auto* histoPtr =
             timings[std::underlying_type<cb::mcbp::ClientOpcode>::type(opcode)]
-                    .get();
+                    .load();
     if (histoPtr) {
         return histoPtr->to_string();
     }
@@ -109,7 +119,7 @@ uint64_t Timings::get_aggregated_mutation_stats() {
     for (auto cmd : timings_mutations) {
         auto* histoPtr =
                 timings[std::underlying_type<cb::mcbp::ClientOpcode>::type(cmd)]
-                        .get();
+                        .load();
         if (histoPtr) {
             ret += histoPtr->getValueCount();
         }
@@ -123,7 +133,7 @@ uint64_t Timings::get_aggregated_retrival_stats() {
     for (auto cmd : timings_retrievals) {
         auto* histoPtr =
                 timings[std::underlying_type<cb::mcbp::ClientOpcode>::type(cmd)]
-                        .get();
+                        .load();
         if (histoPtr) {
             ret += histoPtr->getValueCount();
         }
@@ -143,17 +153,17 @@ cb::sampling::Interval Timings::get_interval_lookup_latency() {
 
 Hdr1sfMicroSecHistogram& Timings::get_or_create_timing_histogram(
         uint8_t opcode) {
-    if (timings[opcode] == nullptr) {
+    if (!timings[opcode]) {
         std::lock_guard<std::mutex> allocLock(histogram_mutex);
-        if (timings[opcode] == nullptr) {
-            timings[opcode] = std::make_unique<Hdr1sfMicroSecHistogram>();
+        if (!timings[opcode]) {
+            timings[opcode] = new Hdr1sfMicroSecHistogram();
         }
     }
-    return *timings[opcode];
+    return *(timings[opcode].load());
 }
 
 Hdr1sfMicroSecHistogram* Timings::get_timing_histogram(uint8_t opcode) const {
-    return timings[opcode].get();
+    return timings[opcode].load();
 }
 
 void Timings::sample(std::chrono::seconds sample_interval) {

diff --git a/daemon/timings.h b/daemon/timings.h
@@ -34,6 +34,7 @@ class Timings {
 public:
     Timings();
     Timings(const Timings&) = delete;
+    ~Timings();
 
     void reset();
     void collect(cb::mcbp::ClientOpcode opcode, std::chrono::nanoseconds nsec);
@@ -69,8 +70,7 @@ class Timings {
     // create an array of unique_ptrs as we want to create HdrHistograms
     // in a lazy manner as their foot print is larger than our old
     // histogram class
-    std::array<std::unique_ptr<Hdr1sfMicroSecHistogram>, MAX_NUM_OPCODES>
-            timings;
+    std::array<std::atomic<Hdr1sfMicroSecHistogram*>, MAX_NUM_OPCODES> timings;
     std::mutex histogram_mutex;
     std::array<cb::sampling::Interval, MAX_NUM_OPCODES> interval_counters;
 };