Re: [Qemu-devel] thread-pool.c race condition?

[Paolo Bonzini]

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