Change Cordz to synchronize tracked cords with Snapshots / DeleteQueue

PiperOrigin-RevId: 817217091
Change-Id: I93ae36c50ec475daa765b53877b1e23bcee6fad6

Author: martijnvels

absl/strings/internal/cordz_info.cc

@@ -17,7 +17,7 @@
 #include <cstdint>
 
 #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;
     }
   }
 

absl/strings/internal/cordz_info.h

@@ -20,8 +20,8 @@
 #include <functional>
 
 #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<CordzInfo*> head ABSL_GUARDED_BY(mutex){nullptr};
+    absl::Mutex mutex{absl::kConstInit};
+    CordzInfo* head ABSL_GUARDED_BY(mutex){nullptr};
   };
 
   static constexpr size_t kMaxStackDepth = 64;