[base] Implement shared mutex for Mac OS X

Bug: chromium:1355917, v8:12037
Change-Id: I5a0a19fd1abb06920f851ef04f5313e9d37dadc6
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3855361Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Darius Mercadier <dmercadier@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82826}

src/base/platform/mutex.cc

@@ -6,6 +6,10 @@
 
 #include <errno.h>
 
+#include <atomic>
+
+#include "src/base/platform/condition-variable.h"
+
 #if DEBUG
 #include <unordered_set>
 #endif  // DEBUG
@@ -224,31 +228,136 @@ bool RecursiveMutex::TryLock() {
 
 #if V8_OS_DARWIN
 
-SharedMutex::SharedMutex() { InitializeNativeHandle(&native_handle_); }
-
-SharedMutex::~SharedMutex() { DestroyNativeHandle(&native_handle_); }
+// On Mac OS X, we have a custom implementation of SharedMutex, which uses
+// exclusive locks, since shared locks are broken. Here is how it works.
+//
+// It uses two exclusive mutexes, {ex_lock} and {ex_cv_lock}, a condition
+// variable {ex_cv} and a counted {shared_count}.
+//
+// To lock the SharedMutex, readers and writers need to first lock {ex_lock}.
+// Readers then simply increment {shared_count} and release {ex_lock} (so that
+// other readers can take the shared lock as well). On the other hand, writers
+// keep {ex_lock} locked until they release the SharedLock. This means that
+// while a writer has the SharedLock, no other readers or writers can aquire it,
+// since {ex_lock} is locked.
+//
+// Additionally, after having locked {ex_lock}, writers wait (using {ex_cv_lock}
+// and {ex_cv}) for {shared_count} to become 0. Once that's the case, it means
+// that no reader has the lock anymore (and no reader or writer can lock it
+// again until the current writer unlocked it, since {ex_lock} is locked).
+//
+// To release the lock:
+//   * readers decrement {shared_count}, and NotifyOne on {ex_cv} to wake up any
+//     potential waiting writer.
+//   * writers simply unlock {ex_lock}
+//
+// Why {ex_cv_lock} is needed: condition variables always need a mutex: the
+// "sleeper" (writer waiting for the SharedMutex here) locks the mutex, test the
+// condition and then "wait" (which releases the mutex), while the "awaiter"
+// (readers releasing the SharedMutex here) needs to take the mutex and notify.
+// Without the mutex, this could happen:
+//   * writer sees that `native_handle_.shared_count != 0` and decides to wait.
+//   * but writer actually gets interrupted before calling `wait`.
+//   * meanwhile, reader decrements `shared_count` which reaches 0 and calls
+//     `NotifyOne`.
+//   * writer is resumed and calls `wait`
+// In this situation, "writer" missed the NotifyOne, and there is no other
+// NotifyOne coming (and writer has the exclusive lock on {ex_lock}, and won't
+// release it until it gets Notified, which means that no-one can ever take this
+// lock again). Thanks to the lock on {ex_cv_lock}, this cannot happen: writer
+// takes the lock before checking `native_handle_.shared_count != 0` and only
+// releases it during the call to `wait`, and reader acquires it before calling
+// NotifyOne.
+//
+//
+// This SharedMutex implementation prevents both readers and writers starvation
+// if the underlying implementation of Mutex::Lock is fair (and it should be!).
+// This is because both LockShared and LockExclusive try to lock {ex_lock} in
+// order to lock the SharedLock, which means that:
+//   - when the 1st writer wants the lock, he'll lock {ex_lock}.
+//   - all readers and writers that want the lock after that will try to lock
+//     {ex_lock} as well, and will hang until it's released.
+//   - once {ex_lock} is released by the writer, any of the reader or writer
+//     waiting for {ex_lock} could get it (and thus get the shared/exclusive
+//     lock on this SharedMutex).
+
+// The default constructors of Mutex will initialize and destruct
+// native_handle_.ex_lock and native_handle_.ex_cv_lock automatically. So, we
+// just have to take care of native_handle_.ex_cv manually, because it's a
+// pointer (and it's a pointer because condition-variable.h includes mutex.h,
+// which means that it couldn't be included in mutex.h).
+// TODO(v8:12037): Consider moving SharedMutex to a separate file to solve this.
+SharedMutex::SharedMutex() { native_handle_.ex_cv = new ConditionVariable(); }
+SharedMutex::~SharedMutex() { delete native_handle_.ex_cv; }
 
-void SharedMutex::LockShared() { LockExclusive(); }
+void SharedMutex::LockShared() {
+  // We need to lock {ex_lock} when taking a shared_lock, in order to prevent
+  // taking a shared lock while a thread has the exclusive lock or is waiting
+  // for it. If a thread is already waiting for an exclusive lock, then this
+  // ex_lock.Lock() will hang until the exclusive lock is released. Once we've
+  // incremented {shared_count}, this shared lock is externally visible, and
+  // {ex_lock} is released, so that other threads can take the shared lock (or
+  // can wait for the exclusive lock).
+  MutexGuard guard(&native_handle_.ex_lock);
+  native_handle_.shared_count.fetch_add(1, std::memory_order_relaxed);
+}
 
 void SharedMutex::LockExclusive() {
   DCHECK(TryHoldSharedMutex(this));
-  LockNativeHandle(&native_handle_);
+  native_handle_.ex_lock.Lock();
+  MutexGuard guard(&native_handle_.ex_cv_lock);
+  while (native_handle_.shared_count.load(std::memory_order_relaxed) != 0) {
+    // If {shared_count} is not 0, then some threads still have the shared lock.
+    // Once the last of them releases its lock, {shared_count} will fall to 0,
+    // and this other thread will call ex_cv->NotifyOne().
+    native_handle_.ex_cv->Wait(&native_handle_.ex_cv_lock);
+  }
+  // Once {shared_count} reaches 0, we are guaranteed that there are no more
+  // threads with the shared lock, and because we hold the lock for {ex_lock},
+  // no thread can take the shared (or exclusive) lock after we've woken from
+  // Wait or after we've checked that "shared_count != 0".
+  DCHECK_EQ(native_handle_.shared_count, 0u);
 }
 
-void SharedMutex::UnlockShared() { UnlockExclusive(); }
+void SharedMutex::UnlockShared() {
+  MutexGuard guard(&native_handle_.ex_cv_lock);
+  if (native_handle_.shared_count.fetch_sub(1, std::memory_order_relaxed) ==
+      1) {
+    // {shared_count} was 1 before the subtraction (`x.fetch_sub(1)` is similar
+    // to `x--`), so it is now 0. We wake up any potential writer that was
+    // waiting for readers to let go of the lock.
+    native_handle_.ex_cv->NotifyOne();
+  }
+}
 
 void SharedMutex::UnlockExclusive() {
   DCHECK(TryReleaseSharedMutex(this));
-  UnlockNativeHandle(&native_handle_);
+  native_handle_.ex_lock.Unlock();
 }
 
-bool SharedMutex::TryLockShared() { return TryLockExclusive(); }
+bool SharedMutex::TryLockShared() {
+  if (!native_handle_.ex_lock.TryLock()) return false;
+  native_handle_.shared_count.fetch_add(1, std::memory_order_relaxed);
+  native_handle_.ex_lock.Unlock();
+  return true;
+}
 
 bool SharedMutex::TryLockExclusive() {
   DCHECK(SharedMutexNotHeld(this));
-  if (!TryLockNativeHandle(&native_handle_)) return false;
+  if (!native_handle_.ex_lock.TryLock()) return false;
+  if (native_handle_.shared_count.load(std::memory_order_relaxed) == 0) {
+    // Is {shared_count} is 0, then all of the shared locks have been released,
+    // and there is no need to use the condition variable.
     DCHECK(TryHoldSharedMutex(this));
     return true;
+  } else {
+    // Note that there is a chance that {shared_count} became 0 after we've
+    // checked if it's 0, since UnlockShared doesn't lock {ex_lock}.
+    // Nevertheless, the specification of TryLockExclusive allows to return
+    // false even though the mutex isn't already locked.
+    native_handle_.ex_lock.Unlock();
+    return false;
+  }
 }
 
 #else  // !V8_OS_DARWIN

src/base/platform/mutex.h

@@ -26,6 +26,8 @@
 namespace v8 {
 namespace base {
 
+class ConditionVariable;
+
 // ----------------------------------------------------------------------------
 // Mutex - a replacement for std::mutex
 //
@@ -268,8 +270,14 @@ class V8_BASE_EXPORT SharedMutex final {
 #if V8_OS_DARWIN
   // pthread_rwlock_t is broken on MacOS when signals are being sent to the
   // process (see https://crbug.com/v8/11399). Until Apple fixes that in the OS,
-  // we have to fall back to a non-shared mutex.
-  using NativeHandle = pthread_mutex_t;
+  // we use our own shared mutex implementation.
+  struct NativeHandle {
+    std::atomic<unsigned int> shared_count = 0;
+    Mutex ex_lock;             // Mutex for exclusive access
+    Mutex ex_cv_lock;          // Mutex for {ex_cv}
+    ConditionVariable* ex_cv;  // Condition variable to wake up the thread
+                               // waiting for {shared_count} to be 0.
+  };
 #elif V8_OS_POSIX
   using NativeHandle = pthread_rwlock_t;
 #elif V8_OS_WIN

test/unittests/base/platform/mutex-unittest.cc

@@ -4,6 +4,13 @@
 
 #include "src/base/platform/mutex.h"
 
+#include <chrono>  // NOLINT(build/c++11)
+#include <queue>
+#include <thread>  // NOLINT(build/c++11)
+
+#include "src/base/platform/condition-variable.h"
+#include "src/base/platform/platform.h"
+#include "src/base/utils/random-number-generator.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace v8 {
@@ -87,5 +94,218 @@ TEST(Mutex, MultipleRecursiveMutexes) {
   recursive_mutex1.Unlock();
 }
 
+TEST(Mutex, SharedMutexSimple) {
+  SharedMutex mutex;
+  mutex.LockShared();
+  mutex.UnlockShared();
+  mutex.LockExclusive();
+  mutex.UnlockExclusive();
+  mutex.LockShared();
+  mutex.UnlockShared();
+}
+
+namespace {
+
+void CheckCannotLockShared(SharedMutex& mutex) {
+  std::thread([&]() { EXPECT_FALSE(mutex.TryLockShared()); }).join();
+}
+
+void CheckCannotLockExclusive(SharedMutex& mutex) {
+  std::thread([&]() { EXPECT_FALSE(mutex.TryLockExclusive()); }).join();
+}
+
+class SharedMutexTestWorker : public Thread {
+  // This class starts a thread that can lock/unlock shared/exclusive a
+  // SharedMutex. The thread has a queue of actions that it needs to execute
+  // (FIFO). Tasks can be added to the queue through the method `Do`.
+  // After each lock/unlock, this class does a few checks about the state of the
+  // SharedMutex (e.g., if we hold a shared lock on a mutex, no one can hold an
+  // exclusive lock on this mutex).
+ public:
+  explicit SharedMutexTestWorker(SharedMutex& shared_mutex,
+                                 std::atomic<int>& reader_count,
+                                 std::atomic<int>& writer_count)
+      : Thread(Options("SharedMutexTestWorker")),
+        shared_mutex_(shared_mutex),
+        reader_count_(reader_count),
+        writer_count_(writer_count) {
+    EXPECT_TRUE(Start());
+  }
+
+  enum class Action {
+    kLockShared,
+    kUnlockShared,
+    kLockExclusive,
+    kUnlockExclusive,
+    kSleep,
+    kEnd
+  };
+
+  void Do(Action what) {
+    MutexGuard guard(&queue_mutex_);
+    actions_.push(what);
+    cv_.NotifyOne();
+  }
+
+  void End() {
+    Do(Action::kEnd);
+    Join();
+  }
+
+  static constexpr int kSleepTimeMs = 5;
+
+  void Run() override {
+    while (true) {
+      queue_mutex_.Lock();
+      while (actions_.empty()) {
+        cv_.Wait(&queue_mutex_);
+      }
+      Action action = actions_.front();
+      actions_.pop();
+      // Unblock the queue before processing the action, in order to not block
+      // the queue if the action is blocked.
+      queue_mutex_.Unlock();
+      switch (action) {
+        case Action::kLockShared:
+          shared_mutex_.LockShared();
+          EXPECT_EQ(writer_count_, 0);
+          CheckCannotLockExclusive(shared_mutex_);
+          reader_count_++;
+          break;
+        case Action::kUnlockShared:
+          reader_count_--;
+          EXPECT_EQ(writer_count_, 0);
+          CheckCannotLockExclusive(shared_mutex_);
+          shared_mutex_.UnlockShared();
+          break;
+        case Action::kLockExclusive:
+          shared_mutex_.LockExclusive();
+          EXPECT_EQ(reader_count_, 0);
+          EXPECT_EQ(writer_count_, 0);
+          CheckCannotLockShared(shared_mutex_);
+          CheckCannotLockExclusive(shared_mutex_);
+          writer_count_++;
+          break;
+        case Action::kUnlockExclusive:
+          writer_count_--;
+          EXPECT_EQ(reader_count_, 0);
+          EXPECT_EQ(writer_count_, 0);
+          CheckCannotLockShared(shared_mutex_);
+          CheckCannotLockExclusive(shared_mutex_);
+          shared_mutex_.UnlockExclusive();
+          break;
+        case Action::kSleep:
+          std::this_thread::sleep_for(std::chrono::milliseconds(kSleepTimeMs));
+          break;
+        case Action::kEnd:
+          return;
+      }
+    }
+  }
+
+ private:
+  // {actions_}, the queue of actions to execute, is shared between the thread
+  // and the object. Holding {queue_mutex_} is required to access it. When the
+  // queue is empty, the thread will Wait on {cv_}. Once `Do` adds an item to
+  // the queue, it should NotifyOne on {cv_} to wake up the thread.
+  Mutex queue_mutex_;
+  ConditionVariable cv_;
+  std::queue<Action> actions_;
+
+  SharedMutex& shared_mutex_;
+
+  // {reader_count} and {writer_count_} are used to verify the integrity of
+  // {shared_mutex_}. For instance, if a thread acquires a shared lock, we
+  // expect {writer_count_} to be 0.
+  std::atomic<int>& reader_count_;
+  std::atomic<int>& writer_count_;
+};
+
+}  // namespace
+
+TEST(Mutex, SharedMutexThreads) {
+  // A simple hand-written scenario involving 3 threads using the SharedMutex.
+  SharedMutex mutex;
+  std::atomic<int> reader_count = 0;
+  std::atomic<int> writer_count = 0;
+
+  SharedMutexTestWorker worker1(mutex, reader_count, writer_count);
+  SharedMutexTestWorker worker2(mutex, reader_count, writer_count);
+  SharedMutexTestWorker worker3(mutex, reader_count, writer_count);
+
+  worker1.Do(SharedMutexTestWorker::Action::kLockShared);
+  worker2.Do(SharedMutexTestWorker::Action::kLockShared);
+  worker3.Do(SharedMutexTestWorker::Action::kLockExclusive);
+  worker3.Do(SharedMutexTestWorker::Action::kSleep);
+  worker1.Do(SharedMutexTestWorker::Action::kUnlockShared);
+  worker1.Do(SharedMutexTestWorker::Action::kLockExclusive);
+  worker2.Do(SharedMutexTestWorker::Action::kUnlockShared);
+  worker2.Do(SharedMutexTestWorker::Action::kLockShared);
+  worker2.Do(SharedMutexTestWorker::Action::kSleep);
+  worker1.Do(SharedMutexTestWorker::Action::kUnlockExclusive);
+  worker3.Do(SharedMutexTestWorker::Action::kUnlockExclusive);
+  worker2.Do(SharedMutexTestWorker::Action::kUnlockShared);
+
+  worker1.End();
+  worker2.End();
+  worker3.End();
+
+  EXPECT_EQ(reader_count, 0);
+  EXPECT_EQ(writer_count, 0);
+
+  // Since the all of the worker threads are done, we should be able to take
+  // both the shared and exclusive lock.
+  EXPECT_TRUE(mutex.TryLockShared());
+  mutex.UnlockShared();
+  EXPECT_TRUE(mutex.TryLockExclusive());
+  mutex.UnlockExclusive();
+}
+
+TEST(Mutex, SharedMutexThreadsFuzz) {
+  // This test creates a lot of threads, each of which tries to take shared or
+  // exclusive lock on a single SharedMutex.
+  SharedMutex mutex;
+  std::atomic<int> reader_count = 0;
+  std::atomic<int> writer_count = 0;
+
+  static constexpr int kThreadCount = 50;
+  static constexpr int kActionPerWorker = 10;
+  static constexpr int kReadToWriteRatio = 5;
+
+  SharedMutexTestWorker* workers[kThreadCount];
+  for (int i = 0; i < kThreadCount; i++) {
+    workers[i] = new SharedMutexTestWorker(mutex, reader_count, writer_count);
+  }
+
+  base::RandomNumberGenerator rand_gen(::testing::FLAGS_gtest_random_seed);
+  for (int i = 0; i < kActionPerWorker; i++) {
+    for (int j = 0; j < kThreadCount; j++) {
+      if (rand_gen.NextInt() % kReadToWriteRatio == 0) {
+        workers[j]->Do(SharedMutexTestWorker::Action::kLockExclusive);
+        workers[j]->Do(SharedMutexTestWorker::Action::kSleep);
+        workers[j]->Do(SharedMutexTestWorker::Action::kUnlockExclusive);
+      } else {
+        workers[j]->Do(SharedMutexTestWorker::Action::kLockShared);
+        workers[j]->Do(SharedMutexTestWorker::Action::kSleep);
+        workers[j]->Do(SharedMutexTestWorker::Action::kUnlockShared);
+      }
+    }
+  }
+  for (int i = 0; i < kThreadCount; i++) {
+    workers[i]->End();
+    delete workers[i];
+  }
+
+  EXPECT_EQ(reader_count, 0);
+  EXPECT_EQ(writer_count, 0);
+
+  // Since the all of the worker threads are done, we should be able to take
+  // both the shared and exclusive lock.
+  EXPECT_TRUE(mutex.TryLockShared());
+  mutex.UnlockShared();
+  EXPECT_TRUE(mutex.TryLockExclusive());
+  mutex.UnlockExclusive();
+}
+
 }  // namespace base
 }  // namespace v8