From d4cb889cd6aa7b5970936148c65786d74e88dd43 Mon Sep 17 00:00:00 2001 From: Richard de Mellow Date: Wed, 7 Aug 2019 12:06:08 +0100 Subject: [PATCH] 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 >::_M_ptr() const /usr/local/include/c++/7.3.0/bits/unique_ptr.h:147 (memcached+0x0000004bf317) #1 std::unique_ptr >::get() const /usr/local/include/c++/7.3.0/bits/unique_ptr.h:337 (memcached+0x0000004bf317) #2 std::unique_ptr >::operator bool() const /usr/local/include/c++/7.3.0/bits/unique_ptr.h:351 (memcached+0x0000004bf317) #3 bool std::operator== >(std::unique_ptr > 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 >) /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::is_move_constructible, std::is_move_assignable >::value, void>::type std::swap(Hdr1sfMicroSecHistogram*&, Hdr1sfMicroSecHistogram*&) /usr/local/include/c++/7.3.0/bits/move.h:199 (memcached+0x0000004bf3cf) #1 std::unique_ptr >::reset(Hdr1sfMicroSecHistogram*) /usr/local/include/c++/7.3.0/bits/unique_ptr.h:374 (memcached+0x0000004bf3cf) #2 std::unique_ptr >::operator=(std::unique_ptr >&&) /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 >) /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 Tested-by: Build Bot Reviewed-by: Dave Rigby --- daemon/timings.cc | 32 +++++++++++++++++++++----------- daemon/timings.h | 4 ++-- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/daemon/timings.cc b/daemon/timings.cc index 41380175f3..bd513cfbc5 100644 --- a/daemon/timings.cc +++ b/daemon/timings.cc @@ -21,10 +21,20 @@ Timings::Timings() { reset(); } -void Timings::reset() { +Timings::~Timings() { + std::lock_guard lg(histogram_mutex); for (auto& t : timings) { - if (t) { - t->reset(); + delete t; + } +} + +void Timings::reset() { + { + std::lock_guard 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::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::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::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 allocLock(histogram_mutex); - if (timings[opcode] == nullptr) { - timings[opcode] = std::make_unique(); + 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 index 0a75349d8c..c41e2e0fcb 100644 --- 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, MAX_NUM_OPCODES> - timings; + std::array, MAX_NUM_OPCODES> timings; std::mutex histogram_mutex; std::array interval_counters; };