This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 9c84920811881873733655d6c95fc7740456c4d7 Author: Alexey Serbin <[email protected]> AuthorDate: Wed Feb 12 22:55:19 2020 -0800 [clock] cleanup on accessing HybridClock::next_timestamp_ Added DCHECK() into HybridClock::NowWithError() method to ensure that access to HybridClock::next_timestamp_ field is guarded. This patch contains other minor updates. Change-Id: I251c538c65bfc75a882218c2d946c795e69bcbbe Reviewed-on: http://gerrit.cloudera.org:8080/15216 Reviewed-by: Adar Dembo <[email protected]> Tested-by: Alexey Serbin <[email protected]> --- src/kudu/clock/clock.h | 2 +- src/kudu/clock/hybrid_clock.cc | 158 ++++++++++++++++++----------------------- src/kudu/clock/hybrid_clock.h | 34 ++++++--- src/kudu/clock/logical_clock.h | 2 +- 4 files changed, 96 insertions(+), 100 deletions(-) diff --git a/src/kudu/clock/clock.h b/src/kudu/clock/clock.h index 020679c..19be1aa 100644 --- a/src/kudu/clock/clock.h +++ b/src/kudu/clock/clock.h @@ -63,7 +63,7 @@ class Clock { } // Indicates whether this clock supports the required external consistency mode. - virtual bool SupportsExternalConsistencyMode(ExternalConsistencyMode mode) = 0; + virtual bool SupportsExternalConsistencyMode(ExternalConsistencyMode mode) const = 0; // Indicates whether the clock has a physical component to its timestamps // (wallclock time). diff --git a/src/kudu/clock/hybrid_clock.cc b/src/kudu/clock/hybrid_clock.cc index 2be46ec..4280eca 100644 --- a/src/kudu/clock/hybrid_clock.cc +++ b/src/kudu/clock/hybrid_clock.cc @@ -149,12 +149,6 @@ Status CheckDeadlineNotWithinMicros(const MonoTime& deadline, int64_t wait_for_u } // anonymous namespace -// Left shifting 12 bits gives us 12 bits for the logical value -// and should still keep accurate microseconds time until 2100+ -const int HybridClock::kBitsToShift = 12; -// This mask gives us back the logical bits. -const uint64_t HybridClock::kLogicalBitMask = (1 << kBitsToShift) - 1; - HybridClock::HybridClock(const scoped_refptr<MetricEntity>& metric_entity) : next_timestamp_(0), state_(kNotInitialized) { @@ -224,15 +218,20 @@ Status HybridClock::Init() { LOG(INFO) << Substitute("HybridClock initialized: " "now $0 us; error $1 us; skew $2 ppm", now_usec, error_usec, time_service_->skew_ppm()); - state_ = kInitialized; + { + // We typically don't expect other threads to access an object until after + // its Init() is called, but there is nothing preventing some other thread + // accessing clock-related metrics via the metrics registry before Init() + // is called. + std::lock_guard<decltype(lock_)> lock(lock_); + state_ = kInitialized; + } return Status::OK(); } Timestamp HybridClock::Now() { Timestamp now; uint64_t error; - - std::lock_guard<simple_spinlock> lock(lock_); NowWithError(&now, &error); return now; } @@ -240,11 +239,7 @@ Timestamp HybridClock::Now() { Timestamp HybridClock::NowLatest() { Timestamp now; uint64_t error; - - { - std::lock_guard<simple_spinlock> lock(lock_); - NowWithError(&now, &error); - } + NowWithError(&now, &error); uint64_t now_latest = GetPhysicalValueMicros(now) + error; uint64_t now_logical = GetLogicalValue(now); @@ -261,57 +256,15 @@ Status HybridClock::GetGlobalLatest(Timestamp* t) { } void HybridClock::NowWithError(Timestamp* timestamp, uint64_t* max_error_usec) { - DCHECK_EQ(state_, kInitialized) << "Clock not initialized. Must call Init() first."; - - uint64_t now_usec; - uint64_t error_usec; - WalltimeWithErrorOrDie(&now_usec, &error_usec); - - // If the physical time from the system clock is higher than our last-returned - // time, we should use the physical timestamp. - uint64_t candidate_phys_timestamp = now_usec << kBitsToShift; - if (PREDICT_TRUE(candidate_phys_timestamp > next_timestamp_)) { - next_timestamp_ = candidate_phys_timestamp; - *timestamp = Timestamp(next_timestamp_++); - *max_error_usec = error_usec; - VLOG(2) << Substitute("Current clock is higher than the last one. " - "Resetting logical values. Physical Value: $0 usec " - "Logical Value: 0 Error: $1", - now_usec, error_usec); - return; - } - - // We don't have the last time read max error since it might have originated - // in another machine, but we can put a bound on the maximum error of the - // timestamp we are providing. - // In particular we know that the "true" time falls within the interval - // now_usec +- now.maxerror so we get the following situations: - // - // 1) - // --------|----------|----|---------|--------------------------> time - // now - e now last now + e - // 2) - // --------|----------|--------------|------|-------------------> time - // now - e now now + e last - // - // Assuming, in the worst case, that the "true" time is now - error we need to - // always return: last - (now - e) as the new maximum error. - // This broadens the error interval for both cases but always returns - // a correct error interval. - - *max_error_usec = (next_timestamp_ >> kBitsToShift) - (now_usec - error_usec); - *timestamp = Timestamp(next_timestamp_++); - VLOG(2) << Substitute("Current clock is lower than the last one. Returning " - "last read and incrementing logical values. " - "Clock: $0 Error: $1", - Stringify(*timestamp), *max_error_usec); + std::lock_guard<decltype(lock_)> lock(lock_); + NowWithErrorUnlocked(timestamp, max_error_usec); } Status HybridClock::Update(const Timestamp& to_update) { - std::lock_guard<simple_spinlock> lock(lock_); Timestamp now; uint64_t error_ignored; - NowWithError(&now, &error_ignored); + std::lock_guard<decltype(lock_)> lock(lock_); + NowWithErrorUnlocked(&now, &error_ignored); // If the incoming message is in the past relative to our current // physical clock, there's nothing to do. @@ -339,14 +292,6 @@ Status HybridClock::Update(const Timestamp& to_update) { return Status::OK(); } -bool HybridClock::SupportsExternalConsistencyMode(ExternalConsistencyMode mode) { - return true; -} - -bool HybridClock::HasPhysicalComponent() const { - return true; -} - MonoDelta HybridClock::GetPhysicalComponentDifference(Timestamp lhs, Timestamp rhs) const { return MonoDelta::FromMicroseconds(static_cast<int64_t>(GetPhysicalValueMicros(lhs)) - static_cast<int64_t>(GetPhysicalValueMicros(rhs))); @@ -357,15 +302,11 @@ Status HybridClock::WaitUntilAfter(const Timestamp& then, TRACE_EVENT0("clock", "HybridClock::WaitUntilAfter"); Timestamp now; uint64_t error; - { - std::lock_guard<simple_spinlock> lock(lock_); - NowWithError(&now, &error); - } + NowWithError(&now, &error); // "unshift" the timestamps so that we can measure actual time uint64_t now_usec = GetPhysicalValueMicros(now); uint64_t then_latest_usec = GetPhysicalValueMicros(then); - uint64_t now_earliest_usec = now_usec - error; // Case 1, event happened definitely in the past, return @@ -398,10 +339,7 @@ Status HybridClock::WaitUntilAfterLocally(const Timestamp& then, const MonoTime& deadline) { Timestamp now; uint64_t error; - { - std::lock_guard<simple_spinlock> lock(lock_); - NowWithError(&now, &error); - } + NowWithError(&now, &error); if (now > then) { return Status::OK(); } @@ -424,18 +362,59 @@ bool HybridClock::IsAfter(Timestamp t) { Timestamp now; { - std::lock_guard<simple_spinlock> lock(lock_); + std::lock_guard<decltype(lock_)> lock(lock_); now = Timestamp(std::max(next_timestamp_, now_usec << kBitsToShift)); } return t.value() < now.value(); } -void HybridClock::WalltimeWithErrorOrDie(uint64_t* now_usec, uint64_t* error_usec) { - Status s = WalltimeWithError(now_usec, error_usec); - if (PREDICT_FALSE(!s.ok())) { - time_service_->DumpDiagnostics(/*log=*/nullptr); - CHECK_OK_PREPEND(s, "unable to get current time with error bound"); +void HybridClock::NowWithErrorUnlocked(Timestamp *timestamp, + uint64_t *max_error_usec) { + DCHECK(lock_.is_locked()); + DCHECK_EQ(state_, kInitialized) << "Clock not initialized. Must call Init() first."; + + uint64_t now_usec; + uint64_t error_usec; + WalltimeWithErrorOrDie(&now_usec, &error_usec); + + // If the physical time from the system clock is higher than our last-returned + // time, we should use the physical timestamp. + uint64_t candidate_phys_timestamp = now_usec << kBitsToShift; + if (PREDICT_TRUE(candidate_phys_timestamp > next_timestamp_)) { + next_timestamp_ = candidate_phys_timestamp; + *timestamp = Timestamp(next_timestamp_++); + *max_error_usec = error_usec; + VLOG(2) << Substitute("Current clock is higher than the last one. " + "Resetting logical values. Physical Value: $0 usec " + "Logical Value: 0 Error: $1", + now_usec, error_usec); + return; } + + // We don't have the last time read max error since it might have originated + // in another machine, but we can put a bound on the maximum error of the + // timestamp we are providing. + // In particular we know that the "true" time falls within the interval + // now_usec +- now.maxerror so we get the following situations: + // + // 1) + // --------|----------|----|---------|--------------------------> time + // now - e now last now + e + // 2) + // --------|----------|--------------|------|-------------------> time + // now - e now now + e last + // + // Assuming, in the worst case, that the "true" time is now - error we need to + // always return: last - (now - e) as the new maximum error. + // This broadens the error interval for both cases but always returns + // a correct error interval. + + *max_error_usec = (next_timestamp_ >> kBitsToShift) - (now_usec - error_usec); + *timestamp = Timestamp(next_timestamp_++); + VLOG(2) << Substitute("Current clock is lower than the last one. Returning " + "last read and incrementing logical values. " + "Clock: $0 Error: $1", + Stringify(*timestamp), *max_error_usec); } Status HybridClock::WalltimeWithError(uint64_t* now_usec, uint64_t* error_usec) { @@ -468,7 +447,7 @@ Status HybridClock::WalltimeWithError(uint64_t* now_usec, uint64_t* error_usec) MonoTime read_time_max_likelihood = read_time_before + MonoDelta::FromMicroseconds(read_time_error_us); - std::unique_lock<simple_spinlock> l(last_clock_read_lock_); + std::unique_lock<decltype(last_clock_read_lock_)> l(last_clock_read_lock_); if (!last_clock_read_time_.Initialized() || last_clock_read_time_ < read_time_max_likelihood) { last_clock_read_time_ = read_time_max_likelihood; @@ -478,7 +457,7 @@ Status HybridClock::WalltimeWithError(uint64_t* now_usec, uint64_t* error_usec) } else { // We failed to read the clock. Extrapolate the new time based on our // last successful read. - std::unique_lock<simple_spinlock> l(last_clock_read_lock_); + std::unique_lock<decltype(last_clock_read_lock_)> l(last_clock_read_lock_); if (!last_clock_read_time_.Initialized()) { RETURN_NOT_OK_PREPEND(s, "could not read system time source"); } @@ -506,6 +485,13 @@ Status HybridClock::WalltimeWithError(uint64_t* now_usec, uint64_t* error_usec) return kudu::Status::OK(); } +void HybridClock::WalltimeWithErrorOrDie(uint64_t* now_usec, uint64_t* error_usec) { + Status s = WalltimeWithError(now_usec, error_usec); + if (PREDICT_FALSE(!s.ok())) { + time_service_->DumpDiagnostics(/*log=*/nullptr); + CHECK_OK_PREPEND(s, "unable to get current time with error bound"); + } +} // Used to get the timestamp for metrics. uint64_t HybridClock::NowForMetrics() { @@ -516,8 +502,6 @@ uint64_t HybridClock::NowForMetrics() { uint64_t HybridClock::ErrorForMetrics() { Timestamp now; uint64_t error; - - std::lock_guard<simple_spinlock> lock(lock_); NowWithError(&now, &error); return error; } diff --git a/src/kudu/clock/hybrid_clock.h b/src/kudu/clock/hybrid_clock.h index be855de..1589937 100644 --- a/src/kudu/clock/hybrid_clock.h +++ b/src/kudu/clock/hybrid_clock.h @@ -62,9 +62,14 @@ class HybridClock : public Clock { Status Update(const Timestamp& to_update) override; // HybridClock supports all external consistency modes. - bool SupportsExternalConsistencyMode(ExternalConsistencyMode mode) override; + bool SupportsExternalConsistencyMode( + ExternalConsistencyMode /* mode */) const override { + return true; + } - bool HasPhysicalComponent() const override; + bool HasPhysicalComponent() const override { + return true; + } MonoDelta GetPhysicalComponentDifference(Timestamp lhs, Timestamp rhs) const override; @@ -157,6 +162,18 @@ class HybridClock : public Clock { } private: + // How many bits to left shift a microseconds clock read. The remainder + // of the timestamp will be reserved for logical values. Left shifting 12 bits + // gives us 12 bits for the logical value and should still keep accurate + // microseconds time until 2100+ + static constexpr const int kBitsToShift = 12; + + // Mask to extract the pure logical bits. + static constexpr const uint64_t kLogicalBitMask = (1 << kBitsToShift) - 1; + + // Variant of NowWithError() that requires 'lock_' to be held already. + void NowWithErrorUnlocked(Timestamp* timestamp, uint64_t* max_error_usec); + // Obtains the current wallclock time and maximum error in microseconds, // and checks if the clock is synchronized. // @@ -176,10 +193,12 @@ class HybridClock : public Clock { // service. std::unique_ptr<clock::TimeService> time_service_; + // Guards access to 'state_' and 'next_timestamp_'. simple_spinlock lock_; // The next timestamp to be generated from this clock, assuming that - // the physical clock hasn't advanced beyond the value stored here. + // the physical clock hasn't advanced beyond the value stored here. Guarded + // by 'lock_'. uint64_t next_timestamp_; // The last valid clock reading we got from the time source, along @@ -189,18 +208,11 @@ class HybridClock : public Clock { uint64_t last_clock_read_physical_; uint64_t last_clock_read_error_; - // How many bits to left shift a microseconds clock read. The remainder - // of the timestamp will be reserved for logical values. - static const int kBitsToShift; - - // Mask to extract the pure logical bits. - static const uint64_t kLogicalBitMask; - + // The state of the object. Guarded by 'lock_'. enum State { kNotInitialized, kInitialized }; - State state_; // Clock metrics are set to detach to their last value. This means diff --git a/src/kudu/clock/logical_clock.h b/src/kudu/clock/logical_clock.h index bd92945..ff144dc 100644 --- a/src/kudu/clock/logical_clock.h +++ b/src/kudu/clock/logical_clock.h @@ -74,7 +74,7 @@ class LogicalClock : public Clock { uint64_t GetCurrentTime(); // Logical clock doesn't support COMMIT_WAIT. - bool SupportsExternalConsistencyMode(ExternalConsistencyMode mode) override { + bool SupportsExternalConsistencyMode(ExternalConsistencyMode mode) const override { return mode != COMMIT_WAIT; }
