diff --git a/absl/strings/internal/cordz_info.cc b/absl/strings/internal/cordz_info.cc index a916c0c6..6696da7b 100644 --- a/absl/strings/internal/cordz_info.cc +++ b/absl/strings/internal/cordz_info.cc @@ -17,7 +17,7 @@ #include #include "absl/base/config.h" -#include "absl/base/internal/spinlock.h" +#include "absl/base/const_init.h" #include "absl/container/inlined_vector.h" #include "absl/debugging/stacktrace.h" #include "absl/strings/internal/cord_internal.h" @@ -224,21 +224,23 @@ class CordRepAnalyzer { CordzInfo* CordzInfo::Head(const CordzSnapshot& snapshot) { ABSL_ASSERT(snapshot.is_snapshot()); - // We can do an 'unsafe' load of 'head', as we are guaranteed that the - // instance it points to is kept alive by the provided CordzSnapshot, so we - // can simply return the current value using an acquire load. + // We obtain the lock here as we must synchronize the first call into the list + // with any concurrent 'Untrack()` operation to avoid any read in the list to + // reorder before the observation of the thread 'untracking a cord' of the + // delete queue being empty or not. After this all next observations are safe + // as we have established all subsequent untracks will be queued for delete. // We do enforce in DEBUG builds that the 'head' value is present in the // delete queue: ODR violations may lead to 'snapshot' and 'global_list_' // being in different libraries / modules. - CordzInfo* head = global_list_.head.load(std::memory_order_acquire); - ABSL_ASSERT(snapshot.DiagnosticsHandleIsSafeToInspect(head)); - return head; + absl::MutexLock l(global_list_.mutex); + ABSL_ASSERT(snapshot.DiagnosticsHandleIsSafeToInspect(global_list_.head)); + return global_list_.head; } CordzInfo* CordzInfo::Next(const CordzSnapshot& snapshot) const { ABSL_ASSERT(snapshot.is_snapshot()); - // Similar to the 'Head()' function, we do not need a mutex here. + // We do not need a lock here. See also comments in Head(). CordzInfo* next = ci_next_.load(std::memory_order_acquire); ABSL_ASSERT(snapshot.DiagnosticsHandleIsSafeToInspect(this)); ABSL_ASSERT(snapshot.DiagnosticsHandleIsSafeToInspect(next)); @@ -327,22 +329,21 @@ CordzInfo::~CordzInfo() { } void CordzInfo::Track() { - SpinLockHolder l(list_->mutex); - - CordzInfo* const head = list_->head.load(std::memory_order_acquire); + absl::MutexLock l(list_->mutex); + CordzInfo* const head = list_->head; if (head != nullptr) { head->ci_prev_.store(this, std::memory_order_release); } ci_next_.store(head, std::memory_order_release); - list_->head.store(this, std::memory_order_release); + list_->head = this; } void CordzInfo::Untrack() { ODRCheck(); { - SpinLockHolder l(list_->mutex); + absl::MutexLock l(list_->mutex); - CordzInfo* const head = list_->head.load(std::memory_order_acquire); + CordzInfo* const head = list_->head; CordzInfo* const next = ci_next_.load(std::memory_order_acquire); CordzInfo* const prev = ci_prev_.load(std::memory_order_acquire); @@ -356,7 +357,7 @@ void CordzInfo::Untrack() { prev->ci_next_.store(next, std::memory_order_release); } else { ABSL_ASSERT(head == this); - list_->head.store(next, std::memory_order_release); + list_->head = next; } } diff --git a/absl/strings/internal/cordz_info.h b/absl/strings/internal/cordz_info.h index 578aa593..ef282a3f 100644 --- a/absl/strings/internal/cordz_info.h +++ b/absl/strings/internal/cordz_info.h @@ -20,8 +20,8 @@ #include #include "absl/base/config.h" +#include "absl/base/const_init.h" #include "absl/base/internal/raw_logging.h" -#include "absl/base/internal/spinlock.h" #include "absl/base/thread_annotations.h" #include "absl/strings/internal/cord_internal.h" #include "absl/strings/internal/cordz_functions.h" @@ -121,12 +121,10 @@ class ABSL_LOCKABLE CordzInfo : public CordzHandle { CordzInfo& operator=(const CordzInfo&) = delete; // Retrieves the oldest existing CordzInfo. - static CordzInfo* Head(const CordzSnapshot& snapshot) - ABSL_NO_THREAD_SAFETY_ANALYSIS; + static CordzInfo* Head(const CordzSnapshot& snapshot); // Retrieves the next oldest existing CordzInfo older than 'this' instance. - CordzInfo* Next(const CordzSnapshot& snapshot) const - ABSL_NO_THREAD_SAFETY_ANALYSIS; + CordzInfo* Next(const CordzSnapshot& snapshot) const; // Locks this instance for the update identified by `method`. // Increases the count for `method` in `update_tracker`. @@ -185,16 +183,13 @@ class ABSL_LOCKABLE CordzInfo : public CordzHandle { int64_t sampling_stride() const { return sampling_stride_; } private: - using SpinLock = absl::base_internal::SpinLock; - using SpinLockHolder = ::absl::base_internal::SpinLockHolder; - // Global cordz info list. CordzInfo stores a pointer to the global list // instance to harden against ODR violations. struct List { constexpr explicit List(absl::ConstInitType) {} - SpinLock mutex; - std::atomic head ABSL_GUARDED_BY(mutex){nullptr}; + absl::Mutex mutex{absl::kConstInit}; + CordzInfo* head ABSL_GUARDED_BY(mutex){nullptr}; }; static constexpr size_t kMaxStackDepth = 64;