Attempt to fix a race condition around fiber stats

Summary: Mcrouter has a logger thread which queries the fiber manager for 3 stats; these stats are non-atomic and therefore this causes a race. This diff applies a simple fix to make these atomic. It does not feel like this is the best fix though, so let me know if anyone has ideas for a better fix. It feels that stats should be provided some other way (rather than a bunch of atomics)...

Reviewed By: yfeldblum

Differential Revision: D22878457

fbshipit-source-id: f17544b38836c454162b83cfa570c90c5e7e6b9a

folly/fibers/Fiber.cpp

@@ -120,8 +120,7 @@ void Fiber::recordStackPosition() {
   auto currentPosition = static_cast<size_t>(
       fiberStackLimit_ + fiberStackSize_ -
       static_cast<unsigned char*>(static_cast<void*>(&stackDummy)));
-  fiberManager_.stackHighWatermark_ =
-      std::max(fiberManager_.stackHighWatermark_, currentPosition);
+  fiberManager_.recordStackPosition(currentPosition);
   VLOG(4) << "Stack usage: " << currentPosition;
 #endif
 }
@@ -154,13 +153,10 @@ void Fiber::recordStackPosition() {
     }
 
     if (UNLIKELY(recordStackUsed_)) {
-      fiberManager_.stackHighWatermark_ = std::max(
-          fiberManager_.stackHighWatermark_,
+      auto newHighWatermark = fiberManager_.recordStackPosition(
           nonMagicInBytes(fiberStackLimit_, fiberStackSize_));
-      VLOG(3) << "Max stack usage: " << fiberManager_.stackHighWatermark_;
-      CHECK(
-          fiberManager_.stackHighWatermark_ <
-          fiberManager_.options_.stackSize - 64)
+      VLOG(3) << "Max stack usage: " << newHighWatermark;
+      CHECK_LT(newHighWatermark, fiberManager_.options_.stackSize - 64)
           << "Fiber stack overflow";
     }
 

folly/fibers/FiberManager.cpp

@@ -127,12 +127,13 @@ Fiber* FiberManager::getFiber() {
 
   if (fibersPool_.empty()) {
     fiber = new Fiber(*this);
-    ++fibersAllocated_;
+    fibersAllocated_.store(fibersAllocated() + 1, std::memory_order_relaxed);
   } else {
     fiber = &fibersPool_.front();
     fibersPool_.pop_front();
-    assert(fibersPoolSize_ > 0);
-    --fibersPoolSize_;
+    auto fibersPoolSize = fibersPoolSize_.load(std::memory_order_relaxed);
+    assert(fibersPoolSize > 0);
+    fibersPoolSize_.store(fibersPoolSize - 1, std::memory_order_relaxed);
   }
   assert(fiber);
   if (++fibersActive_ > maxFibersActiveLastPeriod_) {
@@ -151,15 +152,15 @@ void FiberManager::setExceptionCallback(FiberManager::ExceptionCallback ec) {
 }
 
 size_t FiberManager::fibersAllocated() const {
-  return fibersAllocated_;
+  return fibersAllocated_.load(std::memory_order_relaxed);
 }
 
 size_t FiberManager::fibersPoolSize() const {
-  return fibersPoolSize_;
+  return fibersPoolSize_.load(std::memory_order_relaxed);
 }
 
 size_t FiberManager::stackHighWatermark() const {
-  return stackHighWatermark_;
+  return stackHighWatermark_.load(std::memory_order_relaxed);
 }
 
 void FiberManager::remoteReadyInsert(Fiber* fiber) {
@@ -184,14 +185,19 @@ void FiberManager::setPreemptRunner(InlineFunctionRunner* preemptRunner) {
 }
 
 void FiberManager::doFibersPoolResizing() {
-  while (fibersAllocated_ > maxFibersActiveLastPeriod_ &&
-         fibersPoolSize_ > options_.maxFibersPoolSize) {
+  while (true) {
+    auto fibersAllocated = this->fibersAllocated();
+    auto fibersPoolSize = this->fibersPoolSize();
+    if (!(fibersAllocated > maxFibersActiveLastPeriod_ &&
+          fibersPoolSize > options_.maxFibersPoolSize)) {
+      break;
+    }
     auto fiber = &fibersPool_.front();
     assert(fiber != nullptr);
     fibersPool_.pop_front();
     delete fiber;
-    --fibersPoolSize_;
-    --fibersAllocated_;
+    fibersPoolSize_.store(fibersPoolSize - 1, std::memory_order_relaxed);
+    fibersAllocated_.store(fibersAllocated - 1, std::memory_order_relaxed);
   }
 
   maxFibersActiveLastPeriod_ = fibersActive_;

folly/fibers/FiberManagerInternal-inl.h

@@ -234,6 +234,12 @@ void FiberManager::runFibersHelper(LoopFunc&& loopFunc) {
   loopFunc();
 }
 
+inline size_t FiberManager::recordStackPosition(size_t position) {
+  auto newPosition = std::max(stackHighWatermark(), position);
+  stackHighWatermark_.store(newPosition, std::memory_order_relaxed);
+  return newPosition;
+}
+
 inline void FiberManager::loopUntilNoReadyImpl() {
   runFibersHelper([&] {
     SCOPE_EXIT {

folly/fibers/FiberManagerInternal.h

@@ -465,6 +465,8 @@ class FiberManager : public ::folly::Executor {
   template <typename LoopFunc>
   void runFibersHelper(LoopFunc&& loopFunc);
 
+  size_t recordStackPosition(size_t position);
+
   typedef folly::IntrusiveList<Fiber, &Fiber::listHook_> FiberTailQueue;
   typedef folly::IntrusiveList<Fiber, &Fiber::globalListHook_>
       GlobalFiberTailQueue;
@@ -483,8 +485,10 @@ class FiberManager : public ::folly::Executor {
 
   GlobalFiberTailQueue allFibers_; /**< list of all Fiber objects owned */
 
-  size_t fibersAllocated_{0}; /**< total number of fibers allocated */
-  size_t fibersPoolSize_{0}; /**< total number of fibers in the free pool */
+  // total number of fibers allocated
+  std::atomic<size_t> fibersAllocated_{0};
+  // total number of fibers in the free pool
+  std::atomic<size_t> fibersPoolSize_{0};
   size_t fibersActive_{0}; /**< number of running or blocked fibers */
   size_t fiberId_{0}; /**< id of last fiber used */
 
@@ -514,7 +518,7 @@ class FiberManager : public ::folly::Executor {
   /**
    * Largest observed individual Fiber stack usage in bytes.
    */
-  size_t stackHighWatermark_{0};
+  std::atomic<size_t> stackHighWatermark_{0};
 
   /**
    * Schedules a loop with loopController (unless already scheduled before).