From c08b0ce638361a84d3648aacd4ffbd0da6c357d8 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Wed, 7 Aug 2019 06:19:51 +0200 Subject: Distinguish between internal/external wait deactivation in scheduler This turns out to be necessary for the deadlock detection to work properly. --- libbuild2/context.cxx | 8 ++++---- libbuild2/scheduler.cxx | 29 ++++++++++++++++++----------- libbuild2/scheduler.hxx | 23 ++++++++++++++++------- 3 files changed, 38 insertions(+), 22 deletions(-) diff --git a/libbuild2/context.cxx b/libbuild2/context.cxx index d56abb3..d1bfc33 100644 --- a/libbuild2/context.cxx +++ b/libbuild2/context.cxx @@ -62,11 +62,11 @@ namespace build2 } else if (phase != p) { - sched.deactivate (); + sched.deactivate (false /* external */); for (; phase != p; v->wait (l)) ; r = !fail_; l.unlock (); // Important: activate() can block. - sched.activate (); + sched.activate (false /* external */); } else r = !fail_; @@ -175,11 +175,11 @@ namespace build2 } else // phase != n { - sched.deactivate (); + sched.deactivate (false /* external */); for (; phase != n; v->wait (l)) ; r = !fail_; l.unlock (); // Important: activate() can block. - sched.activate (); + sched.activate (false /* external */); } } diff --git a/libbuild2/scheduler.cxx b/libbuild2/scheduler.cxx index 437f904..bb56810 100644 --- a/libbuild2/scheduler.cxx +++ b/libbuild2/scheduler.cxx @@ -97,7 +97,7 @@ namespace build2 } void scheduler:: - deactivate () + deactivate (bool external) { if (max_active_ == 1) // Serial execution. return; @@ -106,6 +106,8 @@ namespace build2 active_--; waiting_++; + if (external) + external_++; progress_.fetch_add (1, memory_order_relaxed); if (waiting_ > stat_max_waiters_) @@ -122,7 +124,7 @@ namespace build2 { activate_helper (l); } - else if (active_ == 0) + else if (active_ == 0 && external_ == 0) { // Note that we tried to handle this directly in this thread but that // wouldn't work for the phase lock case where we cal; deactivate and @@ -135,7 +137,7 @@ namespace build2 } void scheduler:: - activate (bool collision) + activate (bool external, bool collision) { if (max_active_ == 1) // Serial execution. return; @@ -148,6 +150,8 @@ namespace build2 // If we have spare active threads, then become active. Otherwise it // enters the ready queue. // + if (external) + external_--; waiting_--; ready_++; progress_.fetch_add (1, memory_order_relaxed); @@ -166,7 +170,7 @@ namespace build2 void scheduler:: sleep (const duration& d) { - deactivate (); + deactivate (true /* external */); // MINGW GCC 4.9 doesn't implement this_thread so use Win32 Sleep(). // @@ -178,7 +182,7 @@ namespace build2 Sleep (static_cast (duration_cast (d).count ())); #endif - activate (); + activate (true /* external */); } size_t scheduler:: @@ -190,7 +194,7 @@ namespace build2 // This thread is no longer active. // - deactivate (); + deactivate (false /* external */); // Note that the task count is checked while holding the lock. We also // have to notify while holding the lock (see resume()). The aim here @@ -227,7 +231,7 @@ namespace build2 // This thread is no longer waiting. // - activate (collision); + activate (false /* external */, collision); return tc; } @@ -465,6 +469,8 @@ namespace build2 l.lock (); } + assert (external_ == 0); + // Wait for the deadlock monitor (the only remaining thread). // if (orig_max_active_ != 1) // See tune() for why not max_active_. @@ -759,7 +765,7 @@ namespace build2 // if (s.ready_ != 0) s.ready_condv_.notify_one (); - else if (s.active_ == 0) + else if (s.active_ == 0 && s.external_ == 0) s.dead_condv_.notify_one (); } @@ -802,7 +808,7 @@ namespace build2 { s.dead_condv_.wait (l); - while (s.active_ == 0 && !s.shutdown_) + while (s.active_ == 0 && s.external_ == 0 && !s.shutdown_) { // We may have a deadlock which can happen because of dependency // cycles. @@ -829,9 +835,10 @@ namespace build2 } l.lock (); - // Re-check active count for good measure (maybe spinning too fast). + // Re-check active/external counts for good measure (maybe we are + // spinning too fast). // - if (np == op && s.active_ == 0 && !s.shutdown_) + if (np == op && s.active_ == 0 && s.external_ == 0 && !s.shutdown_) { // Shutting things down cleanly is tricky: we could have handled it // in the scheduler (e.g., by setting a flag and then waking diff --git a/libbuild2/scheduler.hxx b/libbuild2/scheduler.hxx index af1deba..7ceb239 100644 --- a/libbuild2/scheduler.hxx +++ b/libbuild2/scheduler.hxx @@ -132,15 +132,19 @@ namespace build2 resume (const atomic_count& task_count); // An active thread that is about to wait for potentially significant time - // on something other than task_count (e.g., mutex, condition variable) - // should deactivate itself with the scheduler and then reactivate once - // done waiting. + // on something other than task_count (e.g., mutex, condition variable, + // timer, etc) should deactivate itself with the scheduler and then + // reactivate once done waiting. + // + // The external flag indicates whether the wait is for an event external + // to the scheduler, that is, triggered by something other than one of the + // threads managed by the scheduler. // void - deactivate (); + deactivate (bool external); void - activate (bool collision = false); + activate (bool external, bool = false); // Sleep for the specified duration, deactivating the thread before going // to sleep and re-activating it after waking up (which means this @@ -413,8 +417,9 @@ namespace build2 size_t helpers_ = 0; // Number of helper threads created so far. - // Every thread that we manage must be accounted for in one of these - // counters. And their sum should equal (init_active + helpers). + // Every thread that we manage (except for the special deadlock monitor) + // must be accounted for in one of these counters. And their sum should + // equal (init_active + helpers). // size_t active_ = 0; // Active master threads executing a task. size_t idle_ = 0; // Idle helper threads waiting for a task. @@ -422,6 +427,10 @@ namespace build2 size_t ready_ = 0; // Ready master thread waiting to become active. size_t starting_ = 0; // Helper threads starting up. + // Number of waiting threads that are waiting for an external event. + // + size_t external_ = 0; + // Original values (as specified during startup) that can be altered via // tuning. // -- cgit v1.1