From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54627) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wz5lZ-0001m2-W9 for qemu-devel@nongnu.org; Mon, 23 Jun 2014 11:02:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wz5lT-0007dK-RE for qemu-devel@nongnu.org; Mon, 23 Jun 2014 11:02:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61323) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wz5lT-0007d0-Jc for qemu-devel@nongnu.org; Mon, 23 Jun 2014 11:02:35 -0400 Message-ID: <53A84184.6090000@redhat.com> Date: Mon, 23 Jun 2014 17:02:28 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1403535303-14939-1-git-send-email-peter.maydell@linaro.org> In-Reply-To: <1403535303-14939-1-git-send-email-peter.maydell@linaro.org> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] coroutine-win32.c: Add noinline attribute to work around gcc bug List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , qemu-devel@nongnu.org Cc: Stefan Weil , patches@linaro.org 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 > Signed-off-by: Peter Maydell > --- > 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_); >