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 address@hidden

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

Reviewed-by: Paolo Bonzini <address@hidden>

> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  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_);