Better split between the cleanups from self and the ones from the kernel (ie on maest...

This shall remove the need for OS synchronization when registering the
actors that should die. So that's hopefully another fix for the problem solved
by be00fecb16c89d44253052889f4815b35ab9abbe

Also, it protects more modifications to global datastructures from
concurrent modifications by doing them in maestro context. The
previous solution was only protecting a very small amount of them.

diff --git a/src/kernel/EngineImpl.cpp b/src/kernel/EngineImpl.cpp
index 8036e2e..67e9ae8 100644 (file)
--- a/src/kernel/EngineImpl.cpp
+++ b/src/kernel/EngineImpl.cpp
@@ -453,6 +453,10 @@ void EngineImpl::run_all_actors()
 {
   instance_->get_context_factory()->run_all();
 
+  for (auto const& actor : actors_to_run_)
+    if (actor->context_->to_be_freed())
+      actor->cleanup_from_kernel();
+
   actors_to_run_.swap(actors_that_ran_);
   actors_to_run_.clear();
 }

diff --git a/src/kernel/EngineImpl.hpp b/src/kernel/EngineImpl.hpp
index d9e9197..82013ce 100644 (file)
--- a/src/kernel/EngineImpl.hpp
+++ b/src/kernel/EngineImpl.hpp
@@ -67,7 +67,6 @@ class EngineImpl {
 
   std::vector<xbt::Task<void()>> tasks;
 
-  std::mutex mutex_;
   static EngineImpl* instance_;
   actor::ActorImpl* maestro_ = nullptr;
   context::ContextFactory* context_factory_ = nullptr;
@@ -159,7 +158,6 @@ public:
   const std::vector<actor::ActorImpl*>& get_actors_to_run() const { return actors_to_run_; }
   const std::vector<actor::ActorImpl*>& get_actors_that_ran() const { return actors_that_ran_; }
 
-  std::mutex& get_mutex() { return mutex_; }
   bool execute_tasks();
   void add_task(xbt::Task<void()>&& t) { tasks.push_back(std::move(t)); }
   void wake_all_waiting_actors() const;

diff --git a/src/kernel/actor/ActorImpl.cpp b/src/kernel/actor/ActorImpl.cpp
index 37dadd2..7551774 100644 (file)
--- a/src/kernel/actor/ActorImpl.cpp
+++ b/src/kernel/actor/ActorImpl.cpp
@@ -127,7 +127,7 @@ void ActorImpl::detach()
   auto* context = dynamic_cast<context::AttachContext*>(context::Context::self());
   xbt_assert(context != nullptr, "Not a suitable context");
 
-  context->get_actor()->cleanup();
+  context->get_actor()->cleanup_from_self();
   context->attach_stop();
 }
 
@@ -137,20 +137,17 @@ bool ActorImpl::is_maestro() const
   return context_->is_maestro();
 }
 
-void ActorImpl::cleanup_from_simix()
+void ActorImpl::cleanup_from_kernel()
 {
+  xbt_assert(s4u::Actor::is_maestro(), "Cleanup_from_kernel called from '%s' on '%s'", ActorImpl::self()->get_cname(),
+             get_cname());
+
   auto* engine = EngineImpl::get_instance();
-  const std::lock_guard<std::mutex> lock(engine->get_mutex());
   engine->remove_actor(pid_);
   if (host_ && host_actor_list_hook.is_linked())
     host_->get_impl()->remove_actor(this);
   if (not kernel_destroy_list_hook.is_linked())
     engine->add_actor_to_destroy_list(*this);
-}
-
-void ActorImpl::cleanup()
-{
-  finished_ = true;
 
   if (has_to_auto_restart() && not get_host()->is_on()) {
     XBT_DEBUG("Insert host %s to watched_hosts because it's off and %s needs to restart", get_host()->get_cname(),
@@ -158,6 +155,19 @@ void ActorImpl::cleanup()
     watched_hosts().insert(get_host()->get_name());
   }
 
+  undaemonize();
+
+  while (not mailboxes.empty())
+    mailboxes.back()->set_receiver(nullptr);
+}
+
+/* Do all the cleanups from the actor context. Warning, the simcall mechanism was not reignited so doing simcalls in
+ * this context is dangerous */
+void ActorImpl::cleanup_from_self()
+{
+  xbt_assert(not ActorImpl::is_maestro(), "Cleanup_from_self called from maestro on '%s'", get_cname());
+  context_->set_to_be_freed();
+
   if (on_exit) {
     // Execute the termination callbacks
     bool failed = context_->wannadie();
@@ -165,23 +175,14 @@ void ActorImpl::cleanup()
       (*exit_fun)(failed);
     on_exit.reset();
   }
-  undaemonize();
 
   /* cancel non-blocking activities */
   for (auto activity : activities_)
     activity->cancel();
   activities_.clear();
 
-  while (not mailboxes.empty())
-    mailboxes.back()->set_receiver(nullptr);
-
   XBT_DEBUG("%s@%s(%ld) should not run anymore", get_cname(), get_host()->get_cname(), get_pid());
 
-  if (EngineImpl::get_instance()->is_maestro(this)) /* Do not cleanup maestro */
-    return;
-
-  XBT_DEBUG("Cleanup actor %s (%p), waiting synchro %p", get_cname(), this, waiting_synchro_.get());
-
   /* Unregister associated timers if any */
   if (kill_timer_ != nullptr) {
     kill_timer_->remove();
@@ -192,8 +193,6 @@ void ActorImpl::cleanup()
     simcall_.timeout_cb_ = nullptr;
   }
 
-  cleanup_from_simix();
-
   context_->set_wannadie(false); // don't let the simcall's yield() do a Context::stop(), to avoid infinite loops
   actor::simcall_answered([this] { s4u::Actor::on_termination(*get_ciface()); });
   context_->set_wannadie();
@@ -202,8 +201,8 @@ void ActorImpl::cleanup()
 void ActorImpl::exit()
 {
   context_->set_wannadie();
-  suspended_          = false;
-  exception_          = nullptr;
+  suspended_ = false;
+  exception_ = nullptr;
 
   /* destroy the blocking synchro if any */
   if (auto activity = waiting_synchro_) {
@@ -228,7 +227,7 @@ void ActorImpl::exit()
 void ActorImpl::kill(ActorImpl* actor) const
 {
   xbt_assert(not actor->is_maestro(), "Killing maestro is a rather bad idea.");
-  if (actor->finished_) {
+  if (actor->context_->wannadie()) {
     XBT_DEBUG("Ignoring request to kill actor %s@%s that is already dead", actor->get_cname(),
               actor->host_->get_cname());
     return;
@@ -300,7 +299,7 @@ void ActorImpl::yield()
     }
   }
 #if HAVE_SMPI
-  if (not finished_)
+  if (not context_->wannadie())
     smpi_switch_data_segment(get_iface());
 #endif
 }
@@ -357,7 +356,7 @@ void ActorImpl::resume()
   XBT_IN("actor = %p", this);
 
   if (context_->wannadie()) {
-    XBT_VERB("Ignoring request to suspend an actor that is currently dying.");
+    XBT_VERB("Ignoring request to resume an actor that is currently dying.");
     return;
   }
 

diff --git a/src/kernel/actor/ActorImpl.hpp b/src/kernel/actor/ActorImpl.hpp
index 32d37ed..f0fb5c2 100644 (file)
--- a/src/kernel/actor/ActorImpl.hpp
+++ b/src/kernel/actor/ActorImpl.hpp
@@ -78,7 +78,6 @@ public:
   std::unique_ptr<context::Context> context_; /* the context (uctx/raw/thread) that executes the user function */
 
   std::exception_ptr exception_;
-  bool finished_  = false;
   bool suspended_ = false;
 
   activity::ActivityImplPtr waiting_synchro_ = nullptr; /* the current blocking synchro if any */
@@ -123,7 +122,6 @@ public:
 private:
   s4u::Actor piface_; // Our interface is part of ourselves
 
-  void cleanup_from_simix();
   void undaemonize();
 
 public:
@@ -138,7 +136,8 @@ public:
   static ActorImplPtr create(ProcessArg* args);
   static ActorImplPtr attach(const std::string& name, void* data, s4u::Host* host);
   static void detach();
-  void cleanup();
+  void cleanup_from_self();
+  void cleanup_from_kernel();
   void exit();
   void kill(ActorImpl* actor) const;
   void kill_all() const;

diff --git a/src/kernel/context/Context.cpp b/src/kernel/context/Context.cpp
index b20d786..627eb36 100644 (file)
--- a/src/kernel/context/Context.cpp
+++ b/src/kernel/context/Context.cpp
@@ -138,7 +138,7 @@ Context::~Context()
 
 void Context::stop()
 {
-  this->actor_->cleanup();
+  this->actor_->cleanup_from_self();
 }
 
 void Context::set_wannadie(bool value)

diff --git a/src/kernel/context/Context.hpp b/src/kernel/context/Context.hpp
index 0e315e1..473af7a 100644 (file)
--- a/src/kernel/context/Context.hpp
+++ b/src/kernel/context/Context.hpp
@@ -51,7 +51,8 @@ class XBT_PUBLIC Context {
 
   std::function<void()> code_;
   actor::ActorImpl* actor_ = nullptr;
-  bool iwannadie_          = false;
+  bool iwannadie_          = false; // True if we need to do some cleanups in actor mode.
+  bool to_be_freed_        = false; // True if cleanups in actor mode done, but cleanups in kernel mode pending
   bool is_maestro_;
   void declare_context(std::size_t size);
 
@@ -67,6 +68,8 @@ public:
 
   bool wannadie() const { return iwannadie_; }
   void set_wannadie(bool value = true);
+  bool to_be_freed() const { return to_be_freed_; }
+  void set_to_be_freed() { to_be_freed_ = true; }
   bool is_maestro() const { return is_maestro_; }
   void operator()() const { code_(); }
   bool has_code() const { return static_cast<bool>(code_); }

diff --git a/src/s4u/s4u_Actor.cpp b/src/s4u/s4u_Actor.cpp
index 751f333..1d563c4 100644 (file)
--- a/src/s4u/s4u_Actor.cpp
+++ b/src/s4u/s4u_Actor.cpp
@@ -112,7 +112,7 @@ void Actor::join(double timeout) const
   kernel::actor::ActorImpl* issuer = kernel::actor::ActorImpl::self();
   const kernel::actor::ActorImpl* target = pimpl_;
   kernel::actor::simcall_blocking([issuer, target, timeout] {
-    if (target->finished_) {
+    if (target->context_->wannadie()) {
       // The joined actor is already finished, just wake up the issuer right away
       issuer->simcall_answer();
     } else {

diff --git a/teshsuite/s4u/host-on-off-actors/host-on-off-actors.cpp b/teshsuite/s4u/host-on-off-actors/host-on-off-actors.cpp
index f394ea4..f4e9cd8 100644 (file)
--- a/teshsuite/s4u/host-on-off-actors/host-on-off-actors.cpp
+++ b/teshsuite/s4u/host-on-off-actors/host-on-off-actors.cpp
@@ -72,7 +72,7 @@ static void test_launcher(int test_number)
                simgrid::s4u::Engine::get_instance()->get_actor_count(), tasks_done);
       break;
     case 2:
-      // Create an actorthat on a host that is turned off (this is not allowed)
+      // Create an actor on an host that is turned off (this is not allowed)
       XBT_INFO("Test 2:");
       XBT_INFO("  Turn off Jupiter");
       // adsein: Jupiter is already off, hence nothing should happen
@@ -101,6 +101,7 @@ static void test_launcher(int test_number)
       simgrid::s4u::this_actor::sleep_for(10);
       XBT_INFO("  Turn Jupiter off");
       jupiter->turn_off();
+      simgrid::s4u::this_actor::sleep_for(1); // Allow some time to the other actors to die
       XBT_INFO("Test 4 is ok.  (number of actors : %zu, it should be 1 or 2 if RX has not been satisfied)."
                " An exception is raised when we turn off a node that has an actor sleeping",
                simgrid::s4u::Engine::get_instance()->get_actor_count());
@@ -116,6 +117,7 @@ static void test_launcher(int test_number)
       simgrid::s4u::this_actor::sleep_for(10);
       XBT_INFO("  Turn Jupiter off");
       jupiter->turn_off();
+      simgrid::s4u::this_actor::sleep_for(1); // Allow some time to the other actors to die
       XBT_INFO("Test 5 seems ok (number of actors: %zu, it should be 2)",
                simgrid::s4u::Engine::get_instance()->get_actor_count());
       break;

diff --git a/teshsuite/s4u/host-on-off-actors/host-on-off-actors.tesh b/teshsuite/s4u/host-on-off-actors/host-on-off-actors.tesh
index 3eb77ab..43f1dee 100644 (file)
--- a/teshsuite/s4u/host-on-off-actors/host-on-off-actors.tesh
+++ b/teshsuite/s4u/host-on-off-actors/host-on-off-actors.tesh
@@ -25,8 +25,8 @@ $ ./host-on-off-actors ${platfdir}/small_platform.xml 4 --log=no_loc
 > [Jupiter:commTX:(3) 10.000000] [s4u_test/INFO]   Start TX
 > [Tremblay:test_launcher:(1) 10.000000] [s4u_test/INFO]   number of actors: 3
 > [Tremblay:test_launcher:(1) 20.000000] [s4u_test/INFO]   Turn Jupiter off
-> [Tremblay:test_launcher:(1) 20.000000] [s4u_test/INFO] Test 4 is ok.  (number of actors : 2, it should be 1 or 2 if RX has not been satisfied). An exception is raised when we turn off a node that has an actor sleeping
-> [Tremblay:test_launcher:(1) 20.000000] [s4u_test/INFO]   Test done. See you!
+> [Tremblay:test_launcher:(1) 21.000000] [s4u_test/INFO] Test 4 is ok.  (number of actors : 2, it should be 1 or 2 if RX has not been satisfied). An exception is raised when we turn off a node that has an actor sleeping
+> [Tremblay:test_launcher:(1) 21.000000] [s4u_test/INFO]   Test done. See you!
 > [Tremblay:commRX:(2) 25.033047] [s4u_test/INFO]   Receive message: TRANSFER_FAILURE
 > [Tremblay:commRX:(2) 25.033047] [s4u_test/INFO]   RX Done
 > [25.033047] [s4u_test/INFO] Simulation time 25.033
@@ -37,8 +37,8 @@ $ ./host-on-off-actors ${platfdir}/small_platform.xml 5 --log=no_loc
 > [Tremblay:commTX:(3) 10.000000] [s4u_test/INFO]   Start TX
 > [Tremblay:test_launcher:(1) 10.000000] [s4u_test/INFO]   number of actors: 3
 > [Tremblay:test_launcher:(1) 20.000000] [s4u_test/INFO]   Turn Jupiter off
-> [Tremblay:test_launcher:(1) 20.000000] [s4u_test/INFO] Test 5 seems ok (number of actors: 2, it should be 2)
-> [Tremblay:test_launcher:(1) 20.000000] [s4u_test/INFO]   Test done. See you!
+> [Tremblay:test_launcher:(1) 21.000000] [s4u_test/INFO] Test 5 seems ok (number of actors: 2, it should be 2)
+> [Tremblay:test_launcher:(1) 21.000000] [s4u_test/INFO]   Test done. See you!
 > [Tremblay:commTX:(3) 40.000000] [s4u_test/INFO]   TX done
 > [40.000000] [s4u_test/INFO] Simulation time 40