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

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

[Stefan Hajnoczi]

John Snow has reported that qemu-io can hang when the host is under
heavy load.  He made the following observations in gdb:

1. The program is sitting in aio_poll() (called by bdrv_prwv_co())
waiting for request completion.

2. The thread pool has a ThreadPoolElement with ->state == THREAD_DONE.

The ThreadPoolElement should have been reaped by
thread_pool_completion_bh() and its callback invoked.  For some reason
this didn't happen and the program is blocked in poll(2) waiting.

This suggests a race condition in thread-pool.c or qemu_bh_schedule()
(used to complete ThreadPoolElement from a QEMU event loop).

I don't have a good theory why this happens yet.  Just wanted to share
in case someone else hits this problem.

Stefan

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

[Paolo Bonzini]

On 02/04/2015 18:26, Stefan Hajnoczi wrote:
> John Snow has reported that qemu-io can hang when the host is under
> heavy load.  He made the following observations in gdb:
> 
> 1. The program is sitting in aio_poll() (called by bdrv_prwv_co())
> waiting for request completion.
> 
> 2. The thread pool has a ThreadPoolElement with ->state == THREAD_DONE.
> 
> The ThreadPoolElement should have been reaped by
> thread_pool_completion_bh() and its callback invoked.  For some reason
> this didn't happen and the program is blocked in poll(2) waiting.
> 
> This suggests a race condition in thread-pool.c or qemu_bh_schedule()
> (used to complete ThreadPoolElement from a QEMU event loop).
> 
> I don't have a good theory why this happens yet.  Just wanted to share
> in case someone else hits this problem.

Laszlo hit something very similar fairly easily with virtio-scsi (but
not virtio-blk!) on aarch64 hosts.  Any attempt to debug it (ranging
from compilation with -O0 to tracing) made it disappear.  A reliable
reproducer with qemu-io would be a dream...

Paolo

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

[Stefan Hajnoczi]

On Thu, Apr 2, 2015 at 5:43 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 02/04/2015 18:26, Stefan Hajnoczi wrote:
>> John Snow has reported that qemu-io can hang when the host is under
>> heavy load.  He made the following observations in gdb:
>>
>> 1. The program is sitting in aio_poll() (called by bdrv_prwv_co())
>> waiting for request completion.
>>
>> 2. The thread pool has a ThreadPoolElement with ->state == THREAD_DONE.
>>
>> The ThreadPoolElement should have been reaped by
>> thread_pool_completion_bh() and its callback invoked.  For some reason
>> this didn't happen and the program is blocked in poll(2) waiting.
>>
>> This suggests a race condition in thread-pool.c or qemu_bh_schedule()
>> (used to complete ThreadPoolElement from a QEMU event loop).
>>
>> I don't have a good theory why this happens yet.  Just wanted to share
>> in case someone else hits this problem.
>
> Laszlo hit something very similar fairly easily with virtio-scsi (but
> not virtio-blk!) on aarch64 hosts.  Any attempt to debug it (ranging
> from compilation with -O0 to tracing) made it disappear.  A reliable
> reproducer with qemu-io would be a dream...

John said he hasn't seen it recently so we might have a problem reproducing it.

Stefan

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

[John Snow]

On 04/02/2015 12:43 PM, Paolo Bonzini wrote:
>
>
> On 02/04/2015 18:26, Stefan Hajnoczi wrote:
>> John Snow has reported that qemu-io can hang when the host is under
>> heavy load.  He made the following observations in gdb:
>>
>> 1. The program is sitting in aio_poll() (called by bdrv_prwv_co())
>> waiting for request completion.
>>
>> 2. The thread pool has a ThreadPoolElement with ->state == THREAD_DONE.
>>
>> The ThreadPoolElement should have been reaped by
>> thread_pool_completion_bh() and its callback invoked.  For some reason
>> this didn't happen and the program is blocked in poll(2) waiting.
>>
>> This suggests a race condition in thread-pool.c or qemu_bh_schedule()
>> (used to complete ThreadPoolElement from a QEMU event loop).
>>
>> I don't have a good theory why this happens yet.  Just wanted to share
>> in case someone else hits this problem.
>
> Laszlo hit something very similar fairly easily with virtio-scsi (but
> not virtio-blk!) on aarch64 hosts.  Any attempt to debug it (ranging
> from compilation with -O0 to tracing) made it disappear.  A reliable
> reproducer with qemu-io would be a dream...
>
> Paolo
>

Unfortunately for you, I hit it by running qemu-iotests on my laptop 
overnight and I suspect it's triggered by my screensavers hogging CPU 
when I am AFK...

I hit it pretty reliably (100% of the time I tried to run tests while 
AFK -- three independent screensavers running on three monitors) two 
weeks ago, but haven't seen it recently.

I'll keep you posted...

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

[Stefan Hajnoczi]

On Thu, Apr 2, 2015 at 5:43 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 02/04/2015 18:26, Stefan Hajnoczi wrote:
>> John Snow has reported that qemu-io can hang when the host is under
>> heavy load.  He made the following observations in gdb:
>>
>> 1. The program is sitting in aio_poll() (called by bdrv_prwv_co())
>> waiting for request completion.
>>
>> 2. The thread pool has a ThreadPoolElement with ->state == THREAD_DONE.
>>
>> The ThreadPoolElement should have been reaped by
>> thread_pool_completion_bh() and its callback invoked.  For some reason
>> this didn't happen and the program is blocked in poll(2) waiting.
>>
>> This suggests a race condition in thread-pool.c or qemu_bh_schedule()
>> (used to complete ThreadPoolElement from a QEMU event loop).
>>
>> I don't have a good theory why this happens yet.  Just wanted to share
>> in case someone else hits this problem.
>
> Laszlo hit something very similar fairly easily with virtio-scsi (but
> not virtio-blk!) on aarch64 hosts.  Any attempt to debug it (ranging
> from compilation with -O0 to tracing) made it disappear.  A reliable
> reproducer with qemu-io would be a dream...

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.

But now I'm focussing more on thread-pool.c since that has its own
threading constraints.

Stefan

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