[Qemu-devel] [PATCH] coroutine-win32.c: Add noinline attribute to work around gcc bug

[Qemu-devel] [PATCH] coroutine-win32.c: Add noinline attribute to work around gcc bug

[Peter Maydell]

A gcc codegen bug in x86_64-w64-mingw32-gcc (GCC) 4.6.3 means that
non-debug builds of QEMU for Windows tend to assert when using
coroutines. Work around this by marking qemu_coroutine_switch
as noinline.

If we allow gcc to inline qemu_coroutine_switch into
coroutine_trampoline, then it hoists the code to get the
address of the TLS variable "current" out of the while() loop.
This is an invalid transformation because the SwitchToFiber()
call may be called when running thread A but return in thread B,
and so we might be in a different thread context each time
round the loop. This can happen quite often.  Typically.
a coroutine is started when a VCPU thread does bdrv_aio_readv:

     VCPU thread

     main VCPU thread coroutine      I/O coroutine
        bdrv_aio_readv ----->
                                     start I/O operation
                                       thread_pool_submit_co
                       <------------ yields
        back to emulation

Then I/O finishes and the thread-pool.c event notifier triggers in
the I/O thread.  event_notifier_ready calls thread_pool_co_cb, and
the I/O coroutine now restarts *in another thread*:

     iothread

     main iothread coroutine         I/O coroutine (formerly in VCPU thread)
        event_notifier_ready
          thread_pool_co_cb ----->   current = I/O coroutine;
                                     call AIO callback

But on Win32, because of the bug, the "current" being set here the
current coroutine of the VCPU thread, not the iothread.

noinline is a good-enough workaround, and quite unlikely to break in
the future.

(Thanks to Paolo Bonzini for assistance in diagnosing the problem
and providing the detailed example/ascii art quoted above.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 coroutine-win32.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/coroutine-win32.c b/coroutine-win32.c
index edc1f72..17ace37 100644
--- a/coroutine-win32.c
+++ b/coroutine-win32.c
@@ -36,8 +36,17 @@ typedef struct
 static __thread CoroutineWin32 leader;
 static __thread Coroutine *current;
 
-CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
-                                      CoroutineAction action)
+/* This function is marked noinline to prevent GCC from inlining it
+ * into coroutine_trampoline(). If we allow it to do that then it
+ * hoists the code to get the address of the TLS variable "current"
+ * out of the while() loop. This is an invalid transformation because
+ * the SwitchToFiber() call may be called when running thread A but
+ * return in thread B, and so we might be in a different thread
+ * context each time round the loop.
+ */
+CoroutineAction __attribute__((noinline))
+qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
+                      CoroutineAction action)
 {
     CoroutineWin32 *from = DO_UPCAST(CoroutineWin32, base, from_);
     CoroutineWin32 *to = DO_UPCAST(CoroutineWin32, base, to_);
-- 
1.9.2

Re: [Qemu-devel] [PATCH] coroutine-win32.c: Add noinline attribute to work around gcc bug

[Paolo Bonzini]

Il 23/06/2014 16:55, Peter Maydell ha scritto:
> A gcc codegen bug in x86_64-w64-mingw32-gcc (GCC) 4.6.3 means that
> non-debug builds of QEMU for Windows tend to assert when using
> coroutines. Work around this by marking qemu_coroutine_switch
> as noinline.
>
> If we allow gcc to inline qemu_coroutine_switch into
> coroutine_trampoline, then it hoists the code to get the
> address of the TLS variable "current" out of the while() loop.
> This is an invalid transformation because the SwitchToFiber()
> call may be called when running thread A but return in thread B,
> and so we might be in a different thread context each time
> round the loop. This can happen quite often.  Typically.
> a coroutine is started when a VCPU thread does bdrv_aio_readv:
>
>      VCPU thread
>
>      main VCPU thread coroutine      I/O coroutine
>         bdrv_aio_readv ----->
>                                      start I/O operation
>                                        thread_pool_submit_co
>                        <------------ yields
>         back to emulation
>
> Then I/O finishes and the thread-pool.c event notifier triggers in
> the I/O thread.  event_notifier_ready calls thread_pool_co_cb, and
> the I/O coroutine now restarts *in another thread*:
>
>      iothread
>
>      main iothread coroutine         I/O coroutine (formerly in VCPU thread)
>         event_notifier_ready
>           thread_pool_co_cb ----->   current = I/O coroutine;
>                                      call AIO callback
>
> But on Win32, because of the bug, the "current" being set here the
> current coroutine of the VCPU thread, not the iothread.
>
> noinline is a good-enough workaround, and quite unlikely to break in
> the future.
>
> (Thanks to Paolo Bonzini for assistance in diagnosing the problem
> and providing the detailed example/ascii art quoted above.)

My only comments are that:

* I would apply the workaround on all backends, just to be safe.  I 
guess the block maintainers can do that.

* You should Cc qemu-stable@nongnu.org

* You are downplaying your insight that inlining was the real trigger of 
the bug. :)

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  coroutine-win32.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/coroutine-win32.c b/coroutine-win32.c
> index edc1f72..17ace37 100644
> --- a/coroutine-win32.c
> +++ b/coroutine-win32.c
> @@ -36,8 +36,17 @@ typedef struct
>  static __thread CoroutineWin32 leader;
>  static __thread Coroutine *current;
>
> -CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
> -                                      CoroutineAction action)
> +/* This function is marked noinline to prevent GCC from inlining it
> + * into coroutine_trampoline(). If we allow it to do that then it
> + * hoists the code to get the address of the TLS variable "current"
> + * out of the while() loop. This is an invalid transformation because
> + * the SwitchToFiber() call may be called when running thread A but
> + * return in thread B, and so we might be in a different thread
> + * context each time round the loop.
> + */
> +CoroutineAction __attribute__((noinline))
> +qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
> +                      CoroutineAction action)
>  {
>      CoroutineWin32 *from = DO_UPCAST(CoroutineWin32, base, from_);
>      CoroutineWin32 *to = DO_UPCAST(CoroutineWin32, base, to_);
>

Re: [Qemu-devel] [PATCH] coroutine-win32.c: Add noinline attribute to work around gcc bug

[Richard Henderson]

On 06/23/2014 08:02 AM, Paolo Bonzini wrote:
> * I would apply the workaround on all backends, just to be safe.  I guess the
> block maintainers can do that.

The other backends don't use TLS, they use pthread_getspecific, which is a
normal function call that gcc will not hoist out of the loop.  Therefore the
other backends do not suffer from the same problem.

It's also

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH] coroutine-win32.c: Add noinline attribute to work around gcc bug

[Peter Maydell]

On 23 June 2014 16:55, Richard Henderson <rth@twiddle.net> wrote:
> It's also
>
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Thanks; applied to master.

-- PMM