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

["Alex Bennée" <alex.bennee@linaro.org>]

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