[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] thread-pool.c race condition?
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] thread-pool.c race condition? |
Date: |
Thu, 02 Apr 2015 19:00:11 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
On 02/04/2015 18:47, Stefan Hajnoczi wrote:
> My initial speculation was that the qemu_bh_schedule():
>
> if (bh->scheduled)
> return;
>
> Check is causing us to skip BH invocations.
>
> When I look at the code the lack of explicit barriers or atomic
> operations for bh->scheduled itself is a little suspicious.
You may have been onto something. If bh->scheduled is already 1, we do not
execute a memory barrier to order "any writes needed by the callback
[...] before the locations are read in the aio_bh_poll" (quoting from
the comment).
In particular, req->state might be see as THREAD_ACTIVE. This would
explain the failure on aarch64, but not on x86_64.
So, this is probably worth testing:
diff --git a/async.c b/async.c
index 2be88cc..c5d9939 100644
--- a/async.c
+++ b/async.c
@@ -122,19 +122,17 @@ void qemu_bh_schedule(QEMUBH *bh)
{
AioContext *ctx;
- if (bh->scheduled)
- return;
ctx = bh->ctx;
bh->idle = 0;
- /* Make sure that:
+ /* The memory barrier implicit in atomic_xchg makes sure that:
* 1. idle & any writes needed by the callback are done before the
* locations are read in the aio_bh_poll.
* 2. ctx is loaded before scheduled is set and the callback has a chance
* to execute.
*/
- smp_mb();
- bh->scheduled = 1;
- aio_notify(ctx);
+ if (atomic_xchg(&bh->scheduled, 1) == 0) {
+ aio_notify(ctx);
+ }
}
> But now I'm focussing more on thread-pool.c since that has its own
> threading constraints.
Making thread-pool.c less clever didn't make the bug go away for Laszlo.
Paolo