From: Giuliano Procida <gprocida@google.com>
To: Matthias Maennich <maennich@google.com>
Cc: libabigail@sourceware.org, Dodji Seketeli <dodji@seketeli.org>,
kernel-team@android.com
Subject: Re: [PATCH 2/2] abg-workers: guard bring_workers_down to avoid dead lock
Date: Mon, 16 Mar 2020 15:13:52 +0000 [thread overview]
Message-ID: <CAGvU0HmeBgckSYF41ZxEWoV19-ZNPxkz7CTBfofCo0hWPWZAvA@mail.gmail.com> (raw)
In-Reply-To: <20200312113158.24055-2-maennich@google.com>
Hi.
This looks good to me.
On Thu, 12 Mar 2020 at 11:32, 'Matthias Maennich' via kernel-team
<kernel-team@android.com> wrote:
>
> 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.
>
Reviewed-by: Giuliano Procida <gprocida@google.com>
Regards,
Giuliano.
> Signed-off-by: Matthias Maennich <maennich@google.com>
> ---
> 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<bool>, 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<worker>::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;
> }
> --
> 2.25.1.481.gfbce0eb801-goog
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>
next prev parent reply other threads:[~2020-03-16 15:14 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-12 11:31 [PATCH 1/2] configure: add support for thread sanitizer (--enable-tsan) Matthias Maennich
2020-03-12 11:31 ` [PATCH 2/2] abg-workers: guard bring_workers_down to avoid dead lock Matthias Maennich
2020-03-16 15:13 ` Giuliano Procida [this message]
2020-03-18 13:41 ` Dodji Seketeli
2020-03-16 15:00 ` [PATCH 1/2] configure: add support for thread sanitizer (--enable-tsan) Giuliano Procida
2020-03-17 23:18 ` Matthias Maennich
2020-03-22 22:45 ` Matthias Maennich
2020-03-18 12:47 ` Dodji Seketeli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAGvU0HmeBgckSYF41ZxEWoV19-ZNPxkz7CTBfofCo0hWPWZAvA@mail.gmail.com \
--to=gprocida@google.com \
--cc=dodji@seketeli.org \
--cc=kernel-team@android.com \
--cc=libabigail@sourceware.org \
--cc=maennich@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).