From patchwork Thu Mar 12 11:31:58 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Matthias_M=C3=A4nnich?= X-Patchwork-Id: 39031 From: maennich@google.com (Matthias Maennich) Date: Thu, 12 Mar 2020 12:31:58 +0100 Subject: [PATCH 2/2] abg-workers: guard bring_workers_down to avoid dead lock In-Reply-To: <20200312113158.24055-1-maennich@google.com> References: <20200312113158.24055-1-maennich@google.com> Message-ID: <20200312113158.24055-2-maennich@google.com> Since bring_workers_down is not atomic, a worker thread can make a racy read on it while do_bring_workers_down() writes it. That can lead to a deadlock between the worker thread waiting for more work and do_bring_workers_down() waiting for the worker to finish. Address this by guarding all access to bring_workers_down with locking tasks_todo_mutex. This is likely to be dropped after migrating to newer C++ standards supporting std::atomic. * src/abg-workers.cc(do_bring_workers_down): keep task_todo_mutex locked while writing bring_workers_down, (wait_to_execute_a_task): rewrite the loop condition to ensure safe access to bring_workers_down. Signed-off-by: Matthias Maennich Reviewed-by: Giuliano Procida --- src/abg-workers.cc | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/abg-workers.cc b/src/abg-workers.cc index eb892688c489..9035854df958 100644 --- a/src/abg-workers.cc +++ b/src/abg-workers.cc @@ -114,7 +114,9 @@ struct worker struct queue::priv { // A boolean to say if the user wants to shutdown the worker - // threads. + // threads. guarded by tasks_todo_mutex. + // TODO: once we have std::atomic, use it and reconsider the + // synchronization around its reads and writes bool bring_workers_down; // The number of worker threads. size_t num_workers; @@ -249,11 +251,11 @@ struct queue::priv while (!tasks_todo.empty()) pthread_cond_wait(&tasks_done_cond, &tasks_todo_mutex); + bring_workers_down = true; pthread_mutex_unlock(&tasks_todo_mutex); // Now that the task queue is empty, drain the workers by waking them up, // letting them finish their final task before termination. - bring_workers_down = true; ABG_ASSERT(pthread_cond_broadcast(&tasks_todo_cond) == 0); for (std::vector::const_iterator i = workers.begin(); @@ -388,7 +390,7 @@ queue::task_done_notify::operator()(const task_sptr&/*task_done*/) queue::priv* worker::wait_to_execute_a_task(queue::priv* p) { - do + while (true) { pthread_mutex_lock(&p->tasks_todo_mutex); // If there is no more tasks to perform and the queue is not to @@ -426,8 +428,15 @@ worker::wait_to_execute_a_task(queue::priv* p) pthread_mutex_unlock(&p->tasks_done_mutex); pthread_cond_signal(&p->tasks_done_cond); } + + // ensure we access bring_workers_down always guarded + bool drop_out = false; + pthread_mutex_lock(&p->tasks_todo_mutex); + drop_out = p->bring_workers_down; + pthread_mutex_unlock(&p->tasks_todo_mutex); + if (drop_out) + break; } - while (!p->bring_workers_down); return p; }