Logo AND Algorithmique Numérique Distribuée

Public GIT Repository
Finish up the implementation of recursive mutexes
authorMartin Quinson <martin.quinson@ens-rennes.fr>
Thu, 26 Oct 2023 04:22:09 +0000 (06:22 +0200)
committerMartin Quinson <martin.quinson@ens-rennes.fr>
Thu, 26 Oct 2023 04:22:09 +0000 (06:22 +0200)
examples/sthread/pthread-mc-mutex-recursive.tesh
examples/sthread/pthread-mutex-recursive.tesh
examples/sthread/stdobject/stdobject.tesh
examples/sthread/sthread-mutex-simple.tesh
include/simgrid/s4u/Mutex.hpp
src/bindings/python/simgrid_python.cpp
src/kernel/activity/MutexImpl.cpp
src/kernel/activity/MutexImpl.hpp
src/s4u/s4u_Mutex.cpp
src/sthread/ObjectAccess.cpp
src/sthread/sthread_impl.cpp

index 9dbe680..984a4b5 100644 (file)
@@ -8,4 +8,8 @@ $ $VALGRIND_NO_TRACE_CHILDREN ${bindir:=.}/../../bin/simgrid-mc --cfg=model-chec
 > Failed to relock the default mutex.
 > Got the lock on the recursive mutex.
 > Got the lock again on the recursive mutex.
-> [0.000000] [mc_dfs/INFO] DFS exploration ended. 15 unique states visited; 1 backtracks (3 transition replays, 19 states visited overall)
+> Got the lock on the default mutex.
+> Failed to relock the default mutex.
+> Got the lock on the recursive mutex.
+> Got the lock again on the recursive mutex.
+> [0.000000] [mc_dfs/INFO] DFS exploration ended. 17 unique states visited; 1 backtracks (3 transition replays, 21 states visited overall)
index afe8d88..ca535da 100644 (file)
@@ -1,7 +1,7 @@
 $ env ASAN_OPTIONS=verify_asan_link_order=0:$ASAN_OPTIONS LD_PRELOAD=${libdir:=.}/libsthread.so ./pthread-mutex-recursive
 > sthread is intercepting the execution of ./pthread-mutex-recursive
-> [0.000000] [sthread/INFO] All threads exited. Terminating the simulation.
 > Got the lock on the default mutex.
 > Failed to relock the default mutex.
 > Got the lock on the recursive mutex.
-> Got the lock again on the recursive mutex.
\ No newline at end of file
+> Got the lock again on the recursive mutex.
+> [0.000000] [sthread/INFO] All threads exited. Terminating the simulation.
index 457238c..bb8e1b7 100644 (file)
@@ -5,7 +5,7 @@
 ! ignore .*LD_PRELOAD.*
 
 $ $VALGRIND_NO_TRACE_CHILDREN ${bindir:=.}/../../../bin/simgrid-mc --cfg=model-check/setenv:LD_PRELOAD=${libdir:=.}/libsthread.so ${bindir:=.}/stdobject "--log=root.fmt:[%11.6r]%e(%a@%h)%e%m%n" --log=no_loc
-> [   0.000000] (maestro@) Starting the simulation.
+> sthread is intercepting the execution of ./stdobject
 > starting two helpers...
 > waiting for helpers to finish...
 > [   0.000000] (maestro@) Start a DFS exploration. Reduction is: dpor.
@@ -16,7 +16,7 @@ $ $VALGRIND_NO_TRACE_CHILDREN ${bindir:=.}/../../../bin/simgrid-mc --cfg=model-c
 > v = { 1, 2, 3, 5, 8, 13, 21, 21, }; 
 > [   0.000000] (maestro@) thread 1 takes &v
 > [   0.000000] (maestro@) thread 2 takes &v
-> [   0.000000] (maestro@) Unprotected concurent access to &v: thread 1 vs thread 2 (locations hidden because of --log=no_loc).
+> [   0.000000] (maestro@) Unprotected concurent access to &v: thread 1 from 1 location vs thread 2 (locations hidden because of --log=no_loc).
 > [   0.000000] (maestro@) **************************
 > [   0.000000] (maestro@) *** PROPERTY NOT VALID ***
 > [   0.000000] (maestro@) **************************
index ffa04c1..c44bd97 100644 (file)
@@ -1,5 +1,5 @@
 $ ./sthread-mutex-simple
-> [0.000000] [sthread/INFO] Starting the simulation.
+> sthread is intercepting the execution of ./sthread-mutex-simple
 > All threads are started.
 > The thread 0 is terminating.
 > The thread 1 is terminating.
index 2e7a101..06f04c2 100644 (file)
@@ -50,7 +50,8 @@ class XBT_PUBLIC Mutex {
 
 public:
   /** \static Constructs a new mutex */
-  static MutexPtr create();
+  static MutexPtr create(bool recursive = false);
+
   void lock();
   void unlock();
   bool try_lock();
index 2f2ef70..53065f0 100644 (file)
@@ -750,7 +750,8 @@ PYBIND11_MODULE(simgrid, m)
   py::class_<Mutex, MutexPtr>(m, "Mutex",
                               "A classical mutex, but blocking in the simulation world."
                               "See the C++ documentation for details.")
-      .def(py::init<>(&Mutex::create), py::call_guard<py::gil_scoped_release>(), "Mutex constructor.")
+      .def(py::init<>(&Mutex::create), py::call_guard<py::gil_scoped_release>(),
+           "Mutex constructor (pass True as a parameter to get a recursive Mutex).", py::arg("recursive") = false)
       .def("lock", &Mutex::lock, py::call_guard<py::gil_scoped_release>(), "Block until the mutex is acquired.")
       .def("try_lock", &Mutex::try_lock, py::call_guard<py::gil_scoped_release>(),
            "Try to acquire the mutex. Return true if the mutex was acquired, false otherwise.")
index 43a511b..c981c3e 100644 (file)
@@ -47,13 +47,41 @@ unsigned MutexImpl::next_id_ = 0;
 
 MutexAcquisitionImplPtr MutexImpl::lock_async(actor::ActorImpl* issuer)
 {
-  auto res = MutexAcquisitionImplPtr(new kernel::activity::MutexAcquisitionImpl(issuer, this), true);
-
-  if (owner_ != nullptr) {
-    /* Somebody is using the mutex; register the acquisition */
+  /* If the mutex is recursive */
+  if (is_recursive_) {
+    if (owner_ == issuer) {
+      recursive_depth++;
+      auto res = MutexAcquisitionImplPtr(new kernel::activity::MutexAcquisitionImpl(issuer, this), true);
+      res->grant();
+      return res;
+    } else if (owner_ == nullptr) { // Free
+      owner_          = issuer;
+      recursive_depth = 1;
+      auto res        = MutexAcquisitionImplPtr(new kernel::activity::MutexAcquisitionImpl(issuer, this), true);
+      res->grant();
+      return res;
+    }
+
+    for (auto acq : ongoing_acquisitions_)
+      if (acq->get_issuer() == issuer) {
+        acq->recursive_depth_++;
+        return acq;
+      }
+
+    // Not yet in the ongoing acquisition list. Get in there
+    auto res = MutexAcquisitionImplPtr(new kernel::activity::MutexAcquisitionImpl(issuer, this), true);
     ongoing_acquisitions_.push_back(res);
-  } else {
+    return res;
+  }
+
+  // None-recursive mutex
+  auto res = MutexAcquisitionImplPtr(new kernel::activity::MutexAcquisitionImpl(issuer, this), true);
+  if (owner_ == nullptr) { // Lock is free, take it
     owner_  = issuer;
+    recursive_depth = 1;
+    res->grant();
+  } else { // Somebody is using the mutex; register the acquisition
+    ongoing_acquisitions_.push_back(res);
   }
   return res;
 }
@@ -65,14 +93,14 @@ MutexAcquisitionImplPtr MutexImpl::lock_async(actor::ActorImpl* issuer)
  */
 bool MutexImpl::try_lock(actor::ActorImpl* issuer)
 {
-  XBT_IN("(%p, %p)", this, issuer);
-  if (owner_ != nullptr) {
-    XBT_OUT();
-    return false;
+  if (owner_ == issuer && is_recursive_) {
+    recursive_depth++;
+    return true;
   }
+  if (owner_ != nullptr)
+    return false;
 
-  owner_  = issuer;
-  XBT_OUT();
+  owner_ = issuer;
   return true;
 }
 
@@ -88,12 +116,20 @@ void MutexImpl::unlock(actor::ActorImpl* issuer)
   xbt_assert(issuer == owner_, "Cannot release that mutex: you're not the owner. %s is (pid:%ld).",
              owner_ != nullptr ? owner_->get_cname() : "(nobody)", owner_ != nullptr ? owner_->get_pid() : -1);
 
+  if (is_recursive_) {
+    recursive_depth--;
+    if (recursive_depth > 0) // Still owning the lock
+      return;
+  }
+
   if (not ongoing_acquisitions_.empty()) {
     /* Give the ownership to the first waiting actor */
     auto acq = ongoing_acquisitions_.front();
     ongoing_acquisitions_.pop_front();
 
     owner_ = acq->get_issuer();
+    acq->grant();
+    recursive_depth = acq->recursive_depth_;
     if (acq == owner_->waiting_synchro_)
       acq->finish();
     // else, the issuer is not blocked on this acquisition so no need to release it
index 3eebec2..9d9242b 100644 (file)
@@ -42,11 +42,18 @@ namespace simgrid::kernel::activity {
 class XBT_PUBLIC MutexAcquisitionImpl : public ActivityImpl_T<MutexAcquisitionImpl> {
   actor::ActorImpl* issuer_ = nullptr;
   MutexImpl* mutex_         = nullptr;
+  int recursive_depth_      = 1;
+  // TODO: use granted_ this instead of owner_ == self to test().
+  // This is mandatory to get double-lock on non-recursive locks to properly deadlock
+  bool granted_ = false;
+
+  friend MutexImpl;
 
 public:
   MutexAcquisitionImpl(actor::ActorImpl* issuer, MutexImpl* mutex) : issuer_(issuer), mutex_(mutex) {}
   MutexImplPtr get_mutex() { return mutex_; }
   actor::ActorImpl* get_issuer() { return issuer_; }
+  void grant() { granted_ = true; }
 
   bool test(actor::ActorImpl* issuer = nullptr) override;
   void wait_for(actor::ActorImpl* issuer, double timeout) override;
@@ -63,11 +70,13 @@ class XBT_PUBLIC MutexImpl {
   std::deque<MutexAcquisitionImplPtr> ongoing_acquisitions_;
   static unsigned next_id_;
   unsigned id_ = next_id_++;
+  bool is_recursive_  = false;
+  int recursive_depth = 0;
 
   friend MutexAcquisitionImpl;
 
 public:
-  MutexImpl() : piface_(this) {}
+  MutexImpl(bool recursive = false) : piface_(this), is_recursive_(recursive) {}
   MutexImpl(MutexImpl const&) = delete;
   MutexImpl& operator=(MutexImpl const&) = delete;
 
index 051f11c..12fa915 100644 (file)
@@ -55,9 +55,9 @@ bool Mutex::try_lock()
  *
  * See @ref s4u_raii.
  */
-MutexPtr Mutex::create()
+MutexPtr Mutex::create(bool recursive)
 {
-  auto* mutex = new kernel::activity::MutexImpl();
+  auto* mutex = new kernel::activity::MutexImpl(recursive);
   return MutexPtr(&mutex->mutex(), false);
 }
 
index bc5415b..25e9b52 100644 (file)
@@ -91,11 +91,15 @@ int sthread_access_begin(void* objaddr, const char* objname, const char* file, i
           auto msg = std::string("Unprotected concurent access to ") + objname + ": " + ownership->owner->get_name();
           if (not xbt_log_no_loc) {
             msg += simgrid::xbt::string_printf(" at %s:%d", ownership->file, ownership->line);
-            if (ownership->recursive_depth > 1)
+            if (ownership->recursive_depth > 1) {
               msg += simgrid::xbt::string_printf(" (and %d other locations)", ownership->recursive_depth - 1);
+              if (ownership->recursive_depth != 2)
+                msg += "s";
+            }
           } else {
-            msg += simgrid::xbt::string_printf(" from %d locations (remove --log=no_loc for details)",
-                                               ownership->recursive_depth);
+            msg += simgrid::xbt::string_printf(" from %d location", ownership->recursive_depth);
+            if (ownership->recursive_depth != 1)
+              msg += "s";
           }
           msg += " vs " + self->get_name();
           if (xbt_log_no_loc)
index 885c8b6..de2667b 100644 (file)
@@ -6,7 +6,9 @@
 /* SimGrid's pthread interposer. Actual implementation of the symbols (see the comment in sthread.h) */
 
 #include "smpi/smpi.h"
+#include "xbt/asserts.h"
 #include "xbt/ex.h"
+#include "xbt/log.h"
 #include "xbt/string.hpp"
 #include <simgrid/actor.h>
 #include <simgrid/s4u/Actor.hpp>
@@ -123,16 +125,14 @@ int sthread_mutexattr_init(sthread_mutexattr_t* attr)
 }
 int sthread_mutexattr_settype(sthread_mutexattr_t* attr, int type)
 {
-  // reset
-  attr->recursive  = 0;
-  attr->errorcheck = 0;
-
   switch (type) {
     case PTHREAD_MUTEX_NORMAL:
+      xbt_assert(not attr->recursive, "S4U does not allow to remove the recursivness of a mutex.");
       attr->recursive = 0;
       break;
     case PTHREAD_MUTEX_RECURSIVE:
       attr->recursive = 1;
+      attr->errorcheck = 0; // reset
       break;
     case PTHREAD_MUTEX_ERRORCHECK:
       attr->errorcheck = 1;
@@ -168,7 +168,7 @@ int sthread_mutexattr_setrobust(sthread_mutexattr_t* attr, int robustness)
 
 int sthread_mutex_init(sthread_mutex_t* mutex, const sthread_mutexattr_t* attr)
 {
-  auto m = sg4::Mutex::create();
+  auto m = sg4::Mutex::create(attr != nullptr && attr->recursive);
   intrusive_ptr_add_ref(m.get());
 
   mutex->mutex = m.get();