From 37f7753a64cd983c947c403d90b546866ba20542 Mon Sep 17 00:00:00 2001
From: FritzFlorian <flo.fritz@t-online.de>
Date: Thu, 18 Apr 2019 13:17:34 +0200
Subject: [PATCH] Fix memory order in single write, multiple read lock.

---
 lib/pls/include/pls/internal/base/system_details.h      |  4 +++-
 lib/pls/include/pls/internal/scheduling/abstract_task.h |  4 ++--
 lib/pls/src/internal/base/swmr_spin_lock.cpp            | 18 +++++++++++++-----
 lib/pls/src/internal/scheduling/abstract_task.cpp       |  4 ++--
 4 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/lib/pls/include/pls/internal/base/system_details.h b/lib/pls/include/pls/internal/base/system_details.h
index 5d61b3f..82f435c 100644
--- a/lib/pls/include/pls/internal/base/system_details.h
+++ b/lib/pls/include/pls/internal/base/system_details.h
@@ -3,6 +3,9 @@
 #define PLS_SYSTEM_DETAILS_H
 
 #include <cstdint>
+#if (COMPILER == MVCC)
+#include <emmintrin.h>
+#endif
 
 namespace pls {
 namespace internal {
@@ -36,7 +39,6 @@ constexpr std::uintptr_t CACHE_LINE_SIZE = 64;
  * Choose the implementation appropriate for your compiler-cpu combination.
  */
 #if (COMPILER == MVCC)
-#include <emmintrin.h>
 inline void relax_cpu() {
   _mm_pause();
 }
diff --git a/lib/pls/include/pls/internal/scheduling/abstract_task.h b/lib/pls/include/pls/internal/scheduling/abstract_task.h
index 868ad24..21d7357 100644
--- a/lib/pls/include/pls/internal/scheduling/abstract_task.h
+++ b/lib/pls/include/pls/internal/scheduling/abstract_task.h
@@ -16,7 +16,7 @@ class abstract_task {
  private:
   unsigned int depth_;
   abstract_task::id unique_id_;
-  abstract_task *child_task_;
+  abstract_task *volatile child_task_;
 
  public:
   abstract_task(const unsigned int depth, const abstract_task::id &unique_id) :
@@ -26,7 +26,7 @@ class abstract_task {
 
   virtual void execute() = 0;
   void set_child(abstract_task *child_task) { child_task_ = child_task; }
-  abstract_task *child() { return child_task_; }
+  abstract_task *child() const { return child_task_; }
 
   void set_depth(unsigned int depth) { depth_ = depth; }
   unsigned int depth() const { return depth_; }
diff --git a/lib/pls/src/internal/base/swmr_spin_lock.cpp b/lib/pls/src/internal/base/swmr_spin_lock.cpp
index 7bc5f7a..5abb71b 100644
--- a/lib/pls/src/internal/base/swmr_spin_lock.cpp
+++ b/lib/pls/src/internal/base/swmr_spin_lock.cpp
@@ -11,7 +11,7 @@ bool swmr_spin_lock::reader_try_lock() {
     return false;
   }
   // We think we can enter the region
-  readers_++;
+  readers_.fetch_add(1, std::memory_order_acquire);
   if (write_request_.load() == 1) {
     // Whoops, the writer acquires the lock, so we back off again
     readers_--;
@@ -23,21 +23,29 @@ bool swmr_spin_lock::reader_try_lock() {
 
 void swmr_spin_lock::reader_unlock() {
   PROFILE_LOCK("Release Read Lock")
-  readers_--;
+  readers_.fetch_add(-1, std::memory_order_release);
 }
 
 void swmr_spin_lock::writer_lock() {
   PROFILE_LOCK("Acquire Write Lock")
   // Tell the readers that we would like to write
-  write_request_ = 1;
+  int expected;
+  while (true) {
+    expected = 0;
+    if (write_request_.compare_exchange_weak(expected, 1, std::memory_order_acquire)) {
+      break;
+    }
+    system_details::relax_cpu(); // Spin until WE set the write lock flag
+  }
+
   // Wait for all of them to exit the critical section
-  while (readers_.load(std::memory_order_relaxed) > 0)
+  while (readers_.load() > 0)
     system_details::relax_cpu(); // Spin, not expensive as relaxed load
 }
 
 void swmr_spin_lock::writer_unlock() {
   PROFILE_LOCK("Release Write Lock")
-  write_request_ = 0;
+  write_request_.store(0, std::memory_order_release);
 }
 
 }
diff --git a/lib/pls/src/internal/scheduling/abstract_task.cpp b/lib/pls/src/internal/scheduling/abstract_task.cpp
index b82b452..da07a5e 100644
--- a/lib/pls/src/internal/scheduling/abstract_task.cpp
+++ b/lib/pls/src/internal/scheduling/abstract_task.cpp
@@ -31,7 +31,7 @@ bool abstract_task::steal_work() {
     PROFILE_STEALING("Go to our level")
     abstract_task *current_task = target_state->root_task_;
     while (current_task != nullptr && current_task->depth() < depth()) {
-      current_task = current_task->child_task_;
+      current_task = current_task->child();
     }
     PROFILE_END_BLOCK
 
@@ -48,7 +48,7 @@ bool abstract_task::steal_work() {
         }
 
         // No success, we need to steal work from a deeper level using 'top level task stealing'
-        current_task = current_task->child_task_;
+        current_task = current_task->child();
       }
     }
     PROFILE_END_BLOCK;
--
libgit2 0.26.0