[Qemu-devel] [PATCH] cpus: make qemu_mutex_iothread_locked() understand co-routines

[Qemu-devel] [PATCH] cpus: make qemu_mutex_iothread_locked() understand co-routines

[Alex Bennée]

There is a slight wart when checking for the state of the BQL when using
GThread base co-routines (which we keep for ThreadSanitizer runs). While
the main-loop holds the BQL it is suspended until the co-routine
completes however the co-routines run in a separate thread so checking
the TLS variable could be wrong.

We fix this by expanding the check to include qemu_in_coroutine() for
GThread based builds. As it is not used for production builds I'm not
overly worried about any performance impact which should be negligible
anyway.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
 configure |  3 +++
 cpus.c    | 13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/configure b/configure
index 91a14c1..97b89fb 100755
--- a/configure
+++ b/configure
@@ -5461,6 +5461,9 @@ if test "$rbd" = "yes" ; then
 fi
 
 echo "CONFIG_COROUTINE_BACKEND=$coroutine" >> $config_host_mak
+if test "$coroutine" = "gthread" ; then
+  echo "CONFIG_COROUTINE_GTHREAD=1" >> $config_host_mak
+fi
 if test "$coroutine_pool" = "yes" ; then
   echo "CONFIG_COROUTINE_POOL=1" >> $config_host_mak
 else
diff --git a/cpus.c b/cpus.c
index 0c18a9f..a3e189a 100644
--- a/cpus.c
+++ b/cpus.c
@@ -49,6 +49,10 @@
 #include "hw/nmi.h"
 #include "sysemu/replay.h"
 
+#ifdef CONFIG_COROUTINE_GTHREAD
+#include "qemu/coroutine.h"
+#endif
+
 #ifndef _WIN32
 #include "qemu/compatfd.h"
 #endif
@@ -1422,9 +1426,18 @@ bool qemu_in_vcpu_thread(void)
 
 static __thread bool iothread_locked = false;
 
+/*
+ * There is a slight wart when using gthread based co-routines. Here
+ * the BQL is held by the main-thread which is suspended until the
+ * co-routines complete.
+ */
 bool qemu_mutex_iothread_locked(void)
 {
+#ifdef CONFIG_COROUTINE_GTHREAD
+    return iothread_locked || qemu_in_coroutine();
+#else
     return iothread_locked;
+#endif
 }
 
 void qemu_mutex_lock_iothread(void)
-- 
2.9.3

Re: [Qemu-devel] [PATCH] cpus: make qemu_mutex_iothread_locked() understand co-routines

[Paolo Bonzini]

On 21/10/2016 13:54, Alex Bennée wrote:
> There is a slight wart when checking for the state of the BQL when using
> GThread base co-routines (which we keep for ThreadSanitizer runs). While
> the main-loop holds the BQL it is suspended until the co-routine
> completes however the co-routines run in a separate thread so checking
> the TLS variable could be wrong.
> 
> We fix this by expanding the check to include qemu_in_coroutine() for
> GThread based builds. As it is not used for production builds I'm not
> overly worried about any performance impact which should be negligible
> anyway.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>

This is wrong unfortunately.  It is possible to run coroutines outside
the BQL (e.g. with -device virtio-blk,iothread=foo).

Do you know exactly why TSAN has no love for coroutines?

Paolo

> ---
>  configure |  3 +++
>  cpus.c    | 13 +++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/configure b/configure
> index 91a14c1..97b89fb 100755
> --- a/configure
> +++ b/configure
> @@ -5461,6 +5461,9 @@ if test "$rbd" = "yes" ; then
>  fi
>  
>  echo "CONFIG_COROUTINE_BACKEND=$coroutine" >> $config_host_mak
> +if test "$coroutine" = "gthread" ; then
> +  echo "CONFIG_COROUTINE_GTHREAD=1" >> $config_host_mak
> +fi
>  if test "$coroutine_pool" = "yes" ; then
>    echo "CONFIG_COROUTINE_POOL=1" >> $config_host_mak
>  else
> diff --git a/cpus.c b/cpus.c
> index 0c18a9f..a3e189a 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -49,6 +49,10 @@
>  #include "hw/nmi.h"
>  #include "sysemu/replay.h"
>  
> +#ifdef CONFIG_COROUTINE_GTHREAD
> +#include "qemu/coroutine.h"
> +#endif
> +
>  #ifndef _WIN32
>  #include "qemu/compatfd.h"
>  #endif
> @@ -1422,9 +1426,18 @@ bool qemu_in_vcpu_thread(void)
>  
>  static __thread bool iothread_locked = false;
>  
> +/*
> + * There is a slight wart when using gthread based co-routines. Here
> + * the BQL is held by the main-thread which is suspended until the
> + * co-routines complete.
> + */
>  bool qemu_mutex_iothread_locked(void)
>  {
> +#ifdef CONFIG_COROUTINE_GTHREAD
> +    return iothread_locked || qemu_in_coroutine();
> +#else
>      return iothread_locked;
> +#endif
>  }
>  
>  void qemu_mutex_lock_iothread(void)
> 

Re: [Qemu-devel] [PATCH] cpus: make qemu_mutex_iothread_locked() understand co-routines

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 21/10/2016 13:54, Alex Bennée wrote:
>> There is a slight wart when checking for the state of the BQL when using
>> GThread base co-routines (which we keep for ThreadSanitizer runs). While
>> the main-loop holds the BQL it is suspended until the co-routine
>> completes however the co-routines run in a separate thread so checking
>> the TLS variable could be wrong.
>>
>> We fix this by expanding the check to include qemu_in_coroutine() for
>> GThread based builds. As it is not used for production builds I'm not
>> overly worried about any performance impact which should be negligible
>> anyway.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>
> This is wrong unfortunately.  It is possible to run coroutines outside
> the BQL (e.g. with -device virtio-blk,iothread=foo).
>
> Do you know exactly why TSAN has no love for coroutines?

The current production stuff is due to missing support for new stacks
with setcontext. However I have built the latest tsan support library
and that seems happy without the gthread co-routines.

Currently I'm dealing with glib's racy gthread support however.

>
> Paolo
>
>> ---
>>  configure |  3 +++
>>  cpus.c    | 13 +++++++++++++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/configure b/configure
>> index 91a14c1..97b89fb 100755
>> --- a/configure
>> +++ b/configure
>> @@ -5461,6 +5461,9 @@ if test "$rbd" = "yes" ; then
>>  fi
>>
>>  echo "CONFIG_COROUTINE_BACKEND=$coroutine" >> $config_host_mak
>> +if test "$coroutine" = "gthread" ; then
>> +  echo "CONFIG_COROUTINE_GTHREAD=1" >> $config_host_mak
>> +fi
>>  if test "$coroutine_pool" = "yes" ; then
>>    echo "CONFIG_COROUTINE_POOL=1" >> $config_host_mak
>>  else
>> diff --git a/cpus.c b/cpus.c
>> index 0c18a9f..a3e189a 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -49,6 +49,10 @@
>>  #include "hw/nmi.h"
>>  #include "sysemu/replay.h"
>>
>> +#ifdef CONFIG_COROUTINE_GTHREAD
>> +#include "qemu/coroutine.h"
>> +#endif
>> +
>>  #ifndef _WIN32
>>  #include "qemu/compatfd.h"
>>  #endif
>> @@ -1422,9 +1426,18 @@ bool qemu_in_vcpu_thread(void)
>>
>>  static __thread bool iothread_locked = false;
>>
>> +/*
>> + * There is a slight wart when using gthread based co-routines. Here
>> + * the BQL is held by the main-thread which is suspended until the
>> + * co-routines complete.
>> + */
>>  bool qemu_mutex_iothread_locked(void)
>>  {
>> +#ifdef CONFIG_COROUTINE_GTHREAD
>> +    return iothread_locked || qemu_in_coroutine();
>> +#else
>>      return iothread_locked;
>> +#endif
>>  }
>>
>>  void qemu_mutex_lock_iothread(void)
>>


--
Alex Bennée

Re: [Qemu-devel] [PATCH] cpus: make qemu_mutex_iothread_locked() understand co-routines

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

On Tue, Nov 01, 2016 at 04:21:36PM +0000, Alex Bennée wrote:
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 21/10/2016 13:54, Alex Bennée wrote:
> >> There is a slight wart when checking for the state of the BQL when using
> >> GThread base co-routines (which we keep for ThreadSanitizer runs). While
> >> the main-loop holds the BQL it is suspended until the co-routine
> >> completes however the co-routines run in a separate thread so checking
> >> the TLS variable could be wrong.
> >>
> >> We fix this by expanding the check to include qemu_in_coroutine() for
> >> GThread based builds. As it is not used for production builds I'm not
> >> overly worried about any performance impact which should be negligible
> >> anyway.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >
> > This is wrong unfortunately.  It is possible to run coroutines outside
> > the BQL (e.g. with -device virtio-blk,iothread=foo).
> >
> > Do you know exactly why TSAN has no love for coroutines?
> 
> The current production stuff is due to missing support for new stacks
> with setcontext. However I have built the latest tsan support library
> and that seems happy without the gthread co-routines.
> 
> Currently I'm dealing with glib's racy gthread support however.

I think Paolo suggested we drop the GThread backend on IRC.  I agree
that we should do that since GThread co-routines break code that uses
thread-local variables and have never truly worked.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] cpus: make qemu_mutex_iothread_locked() understand co-routines

[Alex Bennée]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Tue, Nov 01, 2016 at 04:21:36PM +0000, Alex Bennée wrote:
>>
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>> > On 21/10/2016 13:54, Alex Bennée wrote:
>> >> There is a slight wart when checking for the state of the BQL when using
>> >> GThread base co-routines (which we keep for ThreadSanitizer runs). While
>> >> the main-loop holds the BQL it is suspended until the co-routine
>> >> completes however the co-routines run in a separate thread so checking
>> >> the TLS variable could be wrong.
>> >>
>> >> We fix this by expanding the check to include qemu_in_coroutine() for
>> >> GThread based builds. As it is not used for production builds I'm not
>> >> overly worried about any performance impact which should be negligible
>> >> anyway.
>> >>
>> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> >
>> > This is wrong unfortunately.  It is possible to run coroutines outside
>> > the BQL (e.g. with -device virtio-blk,iothread=foo).
>> >
>> > Do you know exactly why TSAN has no love for coroutines?
>>
>> The current production stuff is due to missing support for new stacks
>> with setcontext. However I have built the latest tsan support library
>> and that seems happy without the gthread co-routines.
>>
>> Currently I'm dealing with glib's racy gthread support however.
>
> I think Paolo suggested we drop the GThread backend on IRC.  I agree
> that we should do that since GThread co-routines break code that uses
> thread-local variables and have never truly worked.

Indeed I have pulled your patch into my current series of sanitizer
fixes. Once I can fix the setcontext/sigjmp confusion for a
__SANITIZER__ builds I'll post the series.

--
Alex Bennée