[XRay][compiler-rt] Generational Buffer Management

[dberris]

Summary

This change updates the buffer queue implementation to support using a
generation number to identify the lifetime of buffers. This first part
introduces the notion of the generation number, without changing the way
we handle the buffers yet.

What's missing here is the cleanup of the buffers. Ideally we'll keep
the two most recent generations. We need to ensure that before we do any
writes to the buffers, that we check the generation number(s) first.

Those changes will follow-on from this change.

Depends on D52588.

[mboerger]

compiler-rt/lib/xray/tests/unit/buffer_queue_test.cc

132

Why not test B1.Generation > B0.Generation?

139

Test that the next generation is > B1.Generation even after release?

[dberris]

Address some comments by @mboerger.

[dberris]

compiler-rt/lib/xray/tests/unit/buffer_queue_test.cc

132

I don't think exposing that Generation might be increasing monotonically is worth enforcing in the test. Because Generation is unsigned, it can overflow back to 0 and the > property doesn't hold (and it's not meant to).

139

Good point. I've added a second cycle, just to ensure that we are able to keep doing the generational buffer management in the single test case.

[mboerger]

LGTM

[yroux]

Hi Dean,

this commit broke ARN bots, logs are available here:

http://lab.llvm.org:8011/builders/clang-cmake-armv7-full/builds/2683/steps/ninja%20check%201/logs/FAIL%3A%20XRay-Unit%3A%3ABufferQueueTest.GenerationalSupport

Cheers,
Yvan

[dberris]

This had been reverted. Reopening now to update.

[dberris]

Updates to expand the critical region, to include synchronisation on the backing store.

compiler-rt/lib/xray/tests/unit/buffer_queue_test.cc

 //===-- buffer_queue_test.cc ----------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of XRay, a function call tracing system.
 //
 //===----------------------------------------------------------------------===//
 #include "xray_buffer_queue.h"
 #include "gtest/gtest.h"
 
+#include <atomic>
 #include <future>
+#include <thread>
 #include <unistd.h>
 
 namespace __xray {
 
 static constexpr size_t kSize = 4096;
 
 TEST(BufferQueueTest, API) {
   bool Success = false;

 
 TEST(BufferQueueTest, ReleaseUnknown) {
   bool Success = false;
   BufferQueue Buffers(kSize, 1, Success);
   ASSERT_TRUE(Success);
   BufferQueue::Buffer Buf;
   Buf.Data = reinterpret_cast<void *>(0xdeadbeef);
   Buf.Size = kSize;
+  Buf.Generation = Buffers.generation();
   EXPECT_EQ(BufferQueue::ErrorCode::UnrecognizedBuffer,
             Buffers.releaseBuffer(Buf));
 }
 
 TEST(BufferQueueTest, ErrorsWhenFinalising) {
   bool Success = false;
   BufferQueue Buffers(kSize, 2, Success);
   ASSERT_TRUE(Success);
   BufferQueue::Buffer Buf;
   ASSERT_EQ(Buffers.getBuffer(Buf), BufferQueue::ErrorCode::Ok);
   ASSERT_NE(nullptr, Buf.Data);
   ASSERT_EQ(Buffers.finalize(), BufferQueue::ErrorCode::Ok);
   BufferQueue::Buffer OtherBuf;
   ASSERT_EQ(BufferQueue::ErrorCode::QueueFinalizing,
             Buffers.getBuffer(OtherBuf));
-  ASSERT_EQ(BufferQueue::ErrorCode::QueueFinalizing,
-            Buffers.finalize());
+  ASSERT_EQ(BufferQueue::ErrorCode::QueueFinalizing, Buffers.finalize());
   ASSERT_EQ(Buffers.releaseBuffer(Buf), BufferQueue::ErrorCode::Ok);
 }
 
 TEST(BufferQueueTest, MultiThreaded) {
   bool Success = false;
   BufferQueue Buffers(kSize, 100, Success);
   ASSERT_TRUE(Success);
   auto F = [&] {
TEST(BufferQueueTest, Apply) {
   for (int I = 0; I < 10; ++I) {
     ASSERT_EQ(Buffers.getBuffer(B), BufferQueue::ErrorCode::Ok);
     ASSERT_EQ(Buffers.releaseBuffer(B), BufferQueue::ErrorCode::Ok);
   }
   Buffers.apply([&](const BufferQueue::Buffer &B) { ++Count; });
   ASSERT_EQ(Count, 10);
 }
 
+TEST(BufferQueueTest, GenerationalSupport) {
+  bool Success = false;
+  BufferQueue Buffers(kSize, 10, Success);
+  ASSERT_TRUE(Success);
+  BufferQueue::Buffer B0;
+  ASSERT_EQ(Buffers.getBuffer(B0), BufferQueue::ErrorCode::Ok);
+  ASSERT_EQ(Buffers.finalize(),
+            BufferQueue::ErrorCode::Ok); // No more new buffers.
+
+  // Re-initialise the queue.
+  ASSERT_EQ(Buffers.init(kSize, 10), BufferQueue::ErrorCode::Ok);
+
+  BufferQueue::Buffer B1;
+  ASSERT_EQ(Buffers.getBuffer(B1), BufferQueue::ErrorCode::Ok);
+
+  // Validate that the buffers come from different generations.
+  ASSERT_NE(B0.Generation, B1.Generation);
+
+  // We stash the current generation, for use later.
+  auto PrevGen = B1.Generation;
+
+  // At this point, we want to ensure that we can return the buffer from the
+  // first "generation" would still be accepted in the new generation...
+  EXPECT_EQ(Buffers.releaseBuffer(B0), BufferQueue::ErrorCode::Ok);
+
+  // ... and that the new buffer is also accepted.
+  EXPECT_EQ(Buffers.releaseBuffer(B1), BufferQueue::ErrorCode::Ok);
+
+  // A next round will do the same, ensure that we are able to do multiple
+  // rounds in this case.
+  ASSERT_EQ(Buffers.finalize(), BufferQueue::ErrorCode::Ok);
+  ASSERT_EQ(Buffers.init(kSize, 10), BufferQueue::ErrorCode::Ok);
+  EXPECT_EQ(Buffers.getBuffer(B0), BufferQueue::ErrorCode::Ok);
+  EXPECT_EQ(Buffers.getBuffer(B1), BufferQueue::ErrorCode::Ok);
+
+  // Here we ensure that the generation is different from the previous
+  // generation.
+  EXPECT_NE(B0.Generation, PrevGen);
+  EXPECT_EQ(B1.Generation, B1.Generation);
+  ASSERT_EQ(Buffers.finalize(), BufferQueue::ErrorCode::Ok);
+  EXPECT_EQ(Buffers.releaseBuffer(B0), BufferQueue::ErrorCode::Ok);
+  EXPECT_EQ(Buffers.releaseBuffer(B1), BufferQueue::ErrorCode::Ok);
+}
+
+TEST(BufferQueueTest, GenerationalSupportAcrossThreads) {
+  bool Success = false;
+  BufferQueue Buffers(kSize, 10, Success);
+  ASSERT_TRUE(Success);
+
+  std::atomic<int> Counter{0};
+
+  // This function allows us to use thread-local storage to isolate the
+  // instances of the buffers to be used. It also allows us signal the threads
+  // of a new generation, and allow those to get new buffers. This is
+  // representative of how we expect the buffer queue to be used by the XRay
+  // runtime.
+  auto Process = [&] {
+    thread_local BufferQueue::Buffer B;
+    ASSERT_EQ(Buffers.getBuffer(B), BufferQueue::ErrorCode::Ok);
+    auto FirstGen = B.Generation;
+
+    // Signal that we've gotten a buffer in the thread.
+    Counter.fetch_add(1, std::memory_order_acq_rel);
+    while (!Buffers.finalizing()) {
+      Buffers.releaseBuffer(B);
+      Buffers.getBuffer(B);
+    }
+
+    // Signal that we've exited the get/release buffer loop.
+    Counter.fetch_sub(1, std::memory_order_acq_rel);
+    if (B.Data != nullptr)
+      Buffers.releaseBuffer(B);
+
+    // Spin until we find that the Buffer Queue is no longer finalizing.
+    while (Buffers.getBuffer(B) != BufferQueue::ErrorCode::Ok)
+      ;
+
+    // Signal that we've successfully gotten a buffer in the thread.
+    Counter.fetch_add(1, std::memory_order_acq_rel);
+
+    EXPECT_NE(FirstGen, B.Generation);
+    EXPECT_EQ(Buffers.releaseBuffer(B), BufferQueue::ErrorCode::Ok);
+
+    // Signal that we've successfully exited.
+    Counter.fetch_sub(1, std::memory_order_acq_rel);
+  };
+
+  // Spawn two threads running Process.
+  std::thread T0(Process), T1(Process);
+
+  // Spin until we find the counter is up to 2.
+  while (Counter.load(std::memory_order_acquire) != 2)
+    ;
+
+  // Then we finalize, then re-initialize immediately.
+  Buffers.finalize();
+
+  // Spin until we find the counter is down to 0.
+  while (Counter.load(std::memory_order_acquire) != 0)
+    ;
+
+  // Then we re-initialize.
+  EXPECT_EQ(Buffers.init(kSize, 10), BufferQueue::ErrorCode::Ok);
+
+  T0.join();
+  T1.join();
+
+  ASSERT_EQ(Counter.load(std::memory_order_acquire), 0);
+}
+
 } // namespace __xray

compiler-rt/lib/xray/xray_buffer_queue.h

 /// freelist) but is concerned mostly with making it really quick to initialise,
 /// finalise, and get/return buffers to the queue. This is one key component of
 /// the "flight data recorder" (FDR) mode to support ongoing XRay function call
 /// trace collection.
 class BufferQueue {
 public:
   struct Buffer {
     atomic_uint64_t Extents{0};
+    uint64_t Generation{0};
     void *Data = nullptr;
     size_t Size = 0;
   };
 
   struct BufferRep {
     // The managed buffer.
     Buffer Buff;
 
private:
 
   // Pointer to the entry in the array where the next released buffer will be
   // placed.
   BufferRep *First;
 
   // Count of buffers that have been handed out through 'getBuffer'.
   size_t LiveBuffers;
 
+  // We use a generation number to identify buffers and which generation they're
+  // associated with.
+  atomic_uint64_t Generation;
+
 public:
   enum class ErrorCode : unsigned {
     Ok,
     NotEnoughMemory,
     QueueFinalizing,
     UnrecognizedBuffer,
     AlreadyFinalized,
+    AlreadyInitialized,
   };
 
   static const char *getErrorString(ErrorCode E) {
     switch (E) {
     case ErrorCode::Ok:
       return "(none)";
     case ErrorCode::NotEnoughMemory:
       return "no available buffers in the queue";
     case ErrorCode::QueueFinalizing:
       return "queue already finalizing";
     case ErrorCode::UnrecognizedBuffer:
       return "buffer being returned not owned by buffer queue";
     case ErrorCode::AlreadyFinalized:
       return "queue already finalized";
+    case ErrorCode::AlreadyInitialized:
+      return "queue already initialized";
     }
     return "unknown error";
   }
 
   /// Initialise a queue of size |N| with buffers of size |B|. We report success
   /// through |Success|.
   BufferQueue(size_t B, size_t N, bool &Success);
 
public:
   /// Updates |Buf| to point to nullptr, with size 0.
   ///
   /// Returns:
   ///   - ErrorCode::Ok when we successfully release the buffer.
   ///   - ErrorCode::UnrecognizedBuffer for when this BufferQueue does not own
   ///     the buffer being released.
   ErrorCode releaseBuffer(Buffer &Buf);
 
+  /// Initializes the buffer queue, starting a new generation. We can re-set the
+  /// size of buffers with |BS| along with the buffer count with |BC|.
+  ///
+  /// Returns:
+  ///   - ErrorCode::Ok when we successfully initialize the buffer. This
+  ///   requires that the buffer queue is previously finalized.
+  ///   - ErrorCode::AlreadyInitialized when the buffer queue is not finalized.
+  ErrorCode init(size_t BS, size_t BC);
+
   bool finalizing() const {
     return atomic_load(&Finalizing, memory_order_acquire);
   }
 
+  uint64_t generation() const {
+    return atomic_load(&Generation, memory_order_acquire);
+  }
+
   /// Returns the configured size of the buffers in the buffer queue.
   size_t ConfiguredBufferSize() const { return BufferSize; }
 
   /// Sets the state of the BufferQueue to finalizing, which ensures that:
   ///
   ///   - All subsequent attempts to retrieve a Buffer will fail.
   ///   - All releaseBuffer operations will not fail.
   ///

compiler-rt/lib/xray/xray_buffer_queue.cc

 #include "xray_allocator.h"
 #include "xray_defs.h"
 #include <memory>
 #include <sys/mman.h>
 
 using namespace __xray;
 using namespace __sanitizer;
 
-BufferQueue::BufferQueue(size_t B, size_t N,
-                         bool &Success) XRAY_NEVER_INSTRUMENT
-    : BufferSize(B),
-      BufferCount(N),
-      Mutex(),
-      Finalizing{0},
-      BackingStore(allocateBuffer(B *N)),
-      Buffers(initArray<BufferQueue::BufferRep>(N)),
-      Next(Buffers),
-      First(Buffers),
-      LiveBuffers(0) {
-  if (BackingStore == nullptr) {
-    Success = false;
+BufferQueue::ErrorCode BufferQueue::init(size_t BS, size_t BC) {
+  if (!finalizing())
+    return BufferQueue::ErrorCode::AlreadyInitialized;
+
+  atomic_store(&Finalizing, 0, memory_order_release);
+  SpinMutexLock Guard(&Mutex);
+  bool Success = false;
+  BufferSize = BS;
+  BufferCount = BC;
+  BackingStore = allocateBuffer(BufferSize * BufferCount);
+  if (BackingStore == nullptr)
+    return BufferQueue::ErrorCode::NotEnoughMemory;
+
+  auto CleanupBackingStore = __sanitizer::at_scope_exit([&, this] {
+    if (Success)
       return;
-  }
-  if (Buffers == nullptr) {
     deallocateBuffer(BackingStore, BufferSize * BufferCount);
-    Success = false;
-    return;
-  }
+  });
+
+  Buffers = initArray<BufferRep>(BufferCount);
+  if (Buffers == nullptr)
+    return BufferQueue::ErrorCode::NotEnoughMemory;
+
+  // At this point we increment the generation number to associate the buffers
+  // to the new generation.
+  atomic_fetch_add(&Generation, 1, memory_order_acq_rel);
 
-  for (size_t i = 0; i < N; ++i) {
+  Success = true;
+  for (size_t i = 0; i < BufferCount; ++i) {
     auto &T = Buffers[i];
     auto &Buf = T.Buff;
-    Buf.Data = reinterpret_cast<char *>(BackingStore) + (BufferSize * i);
-    Buf.Size = B;
     atomic_store(&Buf.Extents, 0, memory_order_release);
+    Buf.Generation = generation();
+    Buf.Data = reinterpret_cast<char *>(BackingStore) + (BufferSize * i);
+    Buf.Size = BufferSize;
     T.Used = false;
   }
-  Success = true;
+
+  Next = Buffers;
+  First = Buffers;
+  LiveBuffers = 0;
+  return BufferQueue::ErrorCode::Ok;
+}
+
+BufferQueue::BufferQueue(size_t B, size_t N,
+                         bool &Success) XRAY_NEVER_INSTRUMENT
+    : BufferSize(B),
+      BufferCount(N),
+      Mutex(),
+      Finalizing{1},
+      BackingStore(nullptr),
+      Buffers(nullptr),
+      Next(Buffers),
+      First(Buffers),
+      LiveBuffers(0),
+      Generation{0} {
+  Success = init(B, N) == BufferQueue::ErrorCode::Ok;
 }
 
 BufferQueue::ErrorCode BufferQueue::getBuffer(Buffer &Buf) {
   if (atomic_load(&Finalizing, memory_order_acquire))
     return ErrorCode::QueueFinalizing;
 
+  BufferRep *B = nullptr;
+  {
     SpinMutexLock Guard(&Mutex);
     if (LiveBuffers == BufferCount)
       return ErrorCode::NotEnoughMemory;
-
-  auto &T = *Next;
-  auto &B = T.Buff;
-  auto Extents = atomic_load(&B.Extents, memory_order_acquire);
-  atomic_store(&Buf.Extents, Extents, memory_order_release);
-  Buf.Data = B.Data;
-  Buf.Size = B.Size;
-  T.Used = true;
-  ++LiveBuffers;
-
-  if (++Next == (Buffers + BufferCount))
+    B = Next++;
+    if (Next == (Buffers + BufferCount))
       Next = Buffers;
+    ++LiveBuffers;
+  }
 
+  Buf.Data = B->Buff.Data;
+  Buf.Generation = generation();
+  Buf.Size = B->Buff.Size;
+  B->Used = true;
   return ErrorCode::Ok;
 }
 
 BufferQueue::ErrorCode BufferQueue::releaseBuffer(Buffer &Buf) {
   // Check whether the buffer being referred to is within the bounds of the
   // backing store's range.
   if (Buf.Data < BackingStore ||
       Buf.Data >
-          reinterpret_cast<char *>(BackingStore) + (BufferCount * BufferSize))
-    return ErrorCode::UnrecognizedBuffer;
+          reinterpret_cast<char *>(BackingStore) + (BufferCount * BufferSize)) {
+    if (Buf.Generation != generation()) {
+      Buf.Data = nullptr;
+      Buf.Size = 0;
+      Buf.Generation = 0;
+      return BufferQueue::ErrorCode::Ok;
+    }
+    return BufferQueue::ErrorCode::UnrecognizedBuffer;
+  }
 
+  BufferRep *B = nullptr;
+  {
     SpinMutexLock Guard(&Mutex);
 
     // This points to a semantic bug, we really ought to not be releasing more
     // buffers than we actually get.
     if (LiveBuffers == 0)
       return ErrorCode::NotEnoughMemory;
 
-  // Now that the buffer has been released, we mark it as "used".
-  auto Extents = atomic_load(&Buf.Extents, memory_order_acquire);
-  atomic_store(&First->Buff.Extents, Extents, memory_order_release);
-  First->Buff.Data = Buf.Data;
-  First->Buff.Size = Buf.Size;
-  First->Used = true;
-  Buf.Data = nullptr;
-  Buf.Size = 0;
-  atomic_store(&Buf.Extents, 0, memory_order_release);
     --LiveBuffers;
-  if (++First == (Buffers + BufferCount))
+    B = First++;
+    if (First == (Buffers + BufferCount))
       First = Buffers;
+  }
 
+  // Now that the buffer has been released, we mark it as "used".
+  B->Buff.Data = Buf.Data;
+  B->Buff.Size = Buf.Size;
+  B->Buff.Generation = Buf.Generation;
+  B->Used = true;
+  atomic_store(&B->Buff.Extents,
+               atomic_load(&Buf.Extents, memory_order_acquire),
+               memory_order_release);
+  Buf.Data = nullptr;
+  Buf.Size = 0;
+  Buf.Generation = 0;
   return ErrorCode::Ok;
 }
 
 BufferQueue::ErrorCode BufferQueue::finalize() {
   if (atomic_exchange(&Finalizing, 1, memory_order_acq_rel))
     return ErrorCode::QueueFinalizing;
   return ErrorCode::Ok;
 }
 
 BufferQueue::~BufferQueue() {
   for (auto B = Buffers, E = Buffers + BufferCount; B != E; ++B)
     B->~BufferRep();
   deallocateBuffer(Buffers, BufferCount);
   deallocateBuffer(BackingStore, BufferSize * BufferCount);
 }

compiler-rt/lib/xray/xray_fdr_logging.cc

void fdrLoggingHandleTypedEvent(
 
   TLD.RecordPtr += sizeof(TypedEvent);
   internal_memcpy(TLD.RecordPtr, Event, ReducedEventSize);
   TLD.RecordPtr += ReducedEventSize;
   incrementExtents(MetadataRecSize + EventSize);
   endBufferIfFull();
 }
 
-XRayLogInitStatus fdrLoggingInit(UNUSED size_t BufferSize,
-                                 UNUSED size_t BufferMax, void *Options,
+XRayLogInitStatus fdrLoggingInit(size_t, size_t, void *Options,
                                  size_t OptionsSize) XRAY_NEVER_INSTRUMENT {
   if (Options == nullptr)
     return XRayLogInitStatus::XRAY_LOG_UNINITIALIZED;
 
   s32 CurrentStatus = XRayLogInitStatus::XRAY_LOG_UNINITIALIZED;
   if (!atomic_compare_exchange_strong(&LoggingStatus, &CurrentStatus,
                                       XRayLogInitStatus::XRAY_LOG_INITIALIZING,
                                       memory_order_release)) {
FDRFlags.func_duration_threshold_us =
         flags()->xray_fdr_log_func_duration_threshold_us;
     FDRFlags.grace_period_ms = flags()->xray_fdr_log_grace_period_ms;
   }
 
   // The provided options should always override the compiler-provided and
   // environment-variable defined options.
   FDRParser.ParseString(static_cast<const char *>(Options));
   *fdrFlags() = FDRFlags;
-  BufferSize = FDRFlags.buffer_size;
-  BufferMax = FDRFlags.buffer_max;
-
+  auto BufferSize = FDRFlags.buffer_size;
+  auto BufferMax = FDRFlags.buffer_max;
   bool Success = false;
 
   if (BQ != nullptr) {
     BQ->~BufferQueue();
     BQ = nullptr;
   }
 
   if (BQ == nullptr) {