From 4d58af5e9f29128ea5d5cbb677884eae5ba1bf81 Mon Sep 17 00:00:00 2001 From: Martin Quinson Date: Thu, 26 Oct 2023 06:22:09 +0200 Subject: [PATCH] Finish up the implementation of recursive mutexes --- .../sthread/pthread-mc-mutex-recursive.tesh | 6 +- examples/sthread/pthread-mutex-recursive.tesh | 4 +- examples/sthread/stdobject/stdobject.tesh | 4 +- examples/sthread/sthread-mutex-simple.tesh | 2 +- include/simgrid/s4u/Mutex.hpp | 3 +- src/bindings/python/simgrid_python.cpp | 3 +- src/kernel/activity/MutexImpl.cpp | 58 +++++++++++++++---- src/kernel/activity/MutexImpl.hpp | 11 +++- src/s4u/s4u_Mutex.cpp | 4 +- src/sthread/ObjectAccess.cpp | 10 +++- src/sthread/sthread_impl.cpp | 10 ++-- 11 files changed, 85 insertions(+), 30 deletions(-) diff --git a/examples/sthread/pthread-mc-mutex-recursive.tesh b/examples/sthread/pthread-mc-mutex-recursive.tesh index 9dbe680d4d..984a4b56d1 100644 --- a/examples/sthread/pthread-mc-mutex-recursive.tesh +++ b/examples/sthread/pthread-mc-mutex-recursive.tesh @@ -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) diff --git a/examples/sthread/pthread-mutex-recursive.tesh b/examples/sthread/pthread-mutex-recursive.tesh index afe8d88a90..ca535da834 100644 --- a/examples/sthread/pthread-mutex-recursive.tesh +++ b/examples/sthread/pthread-mutex-recursive.tesh @@ -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. diff --git a/examples/sthread/stdobject/stdobject.tesh b/examples/sthread/stdobject/stdobject.tesh index 457238ced8..bb8e1b7beb 100644 --- a/examples/sthread/stdobject/stdobject.tesh +++ b/examples/sthread/stdobject/stdobject.tesh @@ -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@) ************************** diff --git a/examples/sthread/sthread-mutex-simple.tesh b/examples/sthread/sthread-mutex-simple.tesh index ffa04c1dc4..c44bd9794c 100644 --- a/examples/sthread/sthread-mutex-simple.tesh +++ b/examples/sthread/sthread-mutex-simple.tesh @@ -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. diff --git a/include/simgrid/s4u/Mutex.hpp b/include/simgrid/s4u/Mutex.hpp index 2e7a1012b1..06f04c2aa5 100644 --- a/include/simgrid/s4u/Mutex.hpp +++ b/include/simgrid/s4u/Mutex.hpp @@ -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(); diff --git a/src/bindings/python/simgrid_python.cpp b/src/bindings/python/simgrid_python.cpp index 2f2ef70bec..53065f0a72 100644 --- a/src/bindings/python/simgrid_python.cpp +++ b/src/bindings/python/simgrid_python.cpp @@ -750,7 +750,8 @@ PYBIND11_MODULE(simgrid, m) py::class_(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(), "Mutex constructor.") + .def(py::init<>(&Mutex::create), py::call_guard(), + "Mutex constructor (pass True as a parameter to get a recursive Mutex).", py::arg("recursive") = false) .def("lock", &Mutex::lock, py::call_guard(), "Block until the mutex is acquired.") .def("try_lock", &Mutex::try_lock, py::call_guard(), "Try to acquire the mutex. Return true if the mutex was acquired, false otherwise.") diff --git a/src/kernel/activity/MutexImpl.cpp b/src/kernel/activity/MutexImpl.cpp index 43a511b9d3..c981c3ee76 100644 --- a/src/kernel/activity/MutexImpl.cpp +++ b/src/kernel/activity/MutexImpl.cpp @@ -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 diff --git a/src/kernel/activity/MutexImpl.hpp b/src/kernel/activity/MutexImpl.hpp index 3eebec2b12..9d9242b4a5 100644 --- a/src/kernel/activity/MutexImpl.hpp +++ b/src/kernel/activity/MutexImpl.hpp @@ -42,11 +42,18 @@ namespace simgrid::kernel::activity { class XBT_PUBLIC MutexAcquisitionImpl : public ActivityImpl_T { 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 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; diff --git a/src/s4u/s4u_Mutex.cpp b/src/s4u/s4u_Mutex.cpp index 051f11c153..12fa915d5b 100644 --- a/src/s4u/s4u_Mutex.cpp +++ b/src/s4u/s4u_Mutex.cpp @@ -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); } diff --git a/src/sthread/ObjectAccess.cpp b/src/sthread/ObjectAccess.cpp index bc5415bc26..25e9b52c32 100644 --- a/src/sthread/ObjectAccess.cpp +++ b/src/sthread/ObjectAccess.cpp @@ -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) diff --git a/src/sthread/sthread_impl.cpp b/src/sthread/sthread_impl.cpp index 885c8b6bd2..de2667b0e1 100644 --- a/src/sthread/sthread_impl.cpp +++ b/src/sthread/sthread_impl.cpp @@ -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 #include @@ -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(); -- 2.20.1