[Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Stefan Hajnoczi]

The 'gthread' coroutine backend was written before the freelist (aka
pool) existed in qemu-coroutine.c.

This means that every thread is expected to exit when its coroutine
terminates.  It is not possible to reuse threads from a pool.

This patch automatically disables the pool when 'gthread' is used.  This
allows the 'gthread' backend to work again (for example,
tests/test-coroutine completes successfully instead of hanging).

I considered implementing thread reuse but I don't want quirks like CPU
affinity differences due to coroutine threads being recycled.  The
'gthread' backend is a reference backend and it's therefore okay to skip
the pool optimization.

Note this patch also makes it easy to toggle the pool for benchmarking
purposes:

  ./configure --with-coroutine-backend=ucontext \
              --disable-coroutine-pool

Reported-by: Gabriel Kerneis <gabriel@kerneis.info>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 configure        | 24 ++++++++++++++++++++++++
 qemu-coroutine.c | 34 +++++++++++++++++++---------------
 2 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/configure b/configure
index e989609..5cf6341 100755
--- a/configure
+++ b/configure
@@ -235,6 +235,7 @@ guest_agent=""
 want_tools="yes"
 libiscsi=""
 coroutine=""
+coroutine_pool=""
 seccomp=""
 glusterfs=""
 glusterfs_discard="no"
@@ -875,6 +876,10 @@ for opt do
   ;;
   --with-coroutine=*) coroutine="$optarg"
   ;;
+  --disable-coroutine-pool) coroutine_pool="no"
+  ;;
+  --enable-coroutine-pool) coroutine_pool="yes"
+  ;;
   --disable-docs) docs="no"
   ;;
   --enable-docs) docs="yes"
@@ -1161,6 +1166,8 @@ echo "  --disable-seccomp        disable seccomp support"
 echo "  --enable-seccomp         enables seccomp support"
 echo "  --with-coroutine=BACKEND coroutine backend. Supported options:"
 echo "                           gthread, ucontext, sigaltstack, windows"
+echo "  --disable-coroutine-pool disable coroutine freelist (worse performance)"
+echo "  --enable-coroutine-pool  enable coroutine freelist (better performance)"
 echo "  --enable-glusterfs       enable GlusterFS backend"
 echo "  --disable-glusterfs      disable GlusterFS backend"
 echo "  --enable-gcov            enable test coverage analysis with gcov"
@@ -3279,6 +3286,17 @@ else
   esac
 fi
 
+if test "$coroutine_pool" = ""; then
+  if test "$coroutine" = "gthread"; then
+    coroutine_pool=no
+  else
+    coroutine_pool=yes
+  fi
+fi
+if test "$coroutine" = "gthread" -a "$coroutine_pool" = "yes"; then
+  error_exit "'gthread' coroutine backend does not support pool (use --disable-coroutine-pool)"
+fi
+
 ##########################################
 # check if we have open_by_handle_at
 
@@ -3644,6 +3662,7 @@ echo "libiscsi support  $libiscsi"
 echo "build guest agent $guest_agent"
 echo "seccomp support   $seccomp"
 echo "coroutine backend $coroutine"
+echo "coroutine pool    $coroutine_pool"
 echo "GlusterFS support $glusterfs"
 echo "virtio-blk-data-plane $virtio_blk_data_plane"
 echo "gcov              $gcov_tool"
@@ -3999,6 +4018,11 @@ if test "$rbd" = "yes" ; then
 fi
 
 echo "CONFIG_COROUTINE_BACKEND=$coroutine" >> $config_host_mak
+if test "$coroutine_pool" = "yes" ; then
+  echo "CONFIG_COROUTINE_POOL=1" >> $config_host_mak
+else
+  echo "CONFIG_COROUTINE_POOL=0" >> $config_host_mak
+fi
 
 if test "$open_by_handle_at" = "yes" ; then
   echo "CONFIG_OPEN_BY_HANDLE=y" >> $config_host_mak
diff --git a/qemu-coroutine.c b/qemu-coroutine.c
index 423430d..4708521 100644
--- a/qemu-coroutine.c
+++ b/qemu-coroutine.c
@@ -30,15 +30,17 @@ static unsigned int pool_size;
 
 Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
 {
-    Coroutine *co;
-
-    qemu_mutex_lock(&pool_lock);
-    co = QSLIST_FIRST(&pool);
-    if (co) {
-        QSLIST_REMOVE_HEAD(&pool, pool_next);
-        pool_size--;
+    Coroutine *co = NULL;
+
+    if (CONFIG_COROUTINE_POOL) {
+        qemu_mutex_lock(&pool_lock);
+        co = QSLIST_FIRST(&pool);
+        if (co) {
+            QSLIST_REMOVE_HEAD(&pool, pool_next);
+            pool_size--;
+        }
+        qemu_mutex_unlock(&pool_lock);
     }
-    qemu_mutex_unlock(&pool_lock);
 
     if (!co) {
         co = qemu_coroutine_new();
@@ -51,15 +53,17 @@ Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
 
 static void coroutine_delete(Coroutine *co)
 {
-    qemu_mutex_lock(&pool_lock);
-    if (pool_size < POOL_MAX_SIZE) {
-        QSLIST_INSERT_HEAD(&pool, co, pool_next);
-        co->caller = NULL;
-        pool_size++;
+    if (CONFIG_COROUTINE_POOL) {
+        qemu_mutex_lock(&pool_lock);
+        if (pool_size < POOL_MAX_SIZE) {
+            QSLIST_INSERT_HEAD(&pool, co, pool_next);
+            co->caller = NULL;
+            pool_size++;
+            qemu_mutex_unlock(&pool_lock);
+            return;
+        }
         qemu_mutex_unlock(&pool_lock);
-        return;
     }
-    qemu_mutex_unlock(&pool_lock);
 
     qemu_coroutine_delete(co);
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Gabriel Kerneis]

On Wed, Sep 11, 2013 at 04:42:35PM +0200, Stefan Hajnoczi wrote:
> This patch automatically disables the pool when 'gthread' is used.  This
> allows the 'gthread' backend to work again (for example,
> tests/test-coroutine completes successfully instead of hanging).
> 
> Reported-by: Gabriel Kerneis <gabriel@kerneis.info>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Gabriel Kerneis <gabriel@kerneis.info>

-- 
Gabriel

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Kevin Wolf]

Am 11.09.2013 um 16:42 hat Stefan Hajnoczi geschrieben:
> The 'gthread' coroutine backend was written before the freelist (aka
> pool) existed in qemu-coroutine.c.
> 
> This means that every thread is expected to exit when its coroutine
> terminates.  It is not possible to reuse threads from a pool.
> 
> This patch automatically disables the pool when 'gthread' is used.  This
> allows the 'gthread' backend to work again (for example,
> tests/test-coroutine completes successfully instead of hanging).
> 
> I considered implementing thread reuse but I don't want quirks like CPU
> affinity differences due to coroutine threads being recycled.  The
> 'gthread' backend is a reference backend and it's therefore okay to skip
> the pool optimization.
> 
> Note this patch also makes it easy to toggle the pool for benchmarking
> purposes:
> 
>   ./configure --with-coroutine-backend=ucontext \
>               --disable-coroutine-pool
> 
> Reported-by: Gabriel Kerneis <gabriel@kerneis.info>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  configure        | 24 ++++++++++++++++++++++++
>  qemu-coroutine.c | 34 +++++++++++++++++++---------------
>  2 files changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/configure b/configure
> index e989609..5cf6341 100755
> --- a/configure
> +++ b/configure
> @@ -235,6 +235,7 @@ guest_agent=""
>  want_tools="yes"
>  libiscsi=""
>  coroutine=""
> +coroutine_pool=""
>  seccomp=""
>  glusterfs=""
>  glusterfs_discard="no"
> @@ -875,6 +876,10 @@ for opt do
>    ;;
>    --with-coroutine=*) coroutine="$optarg"
>    ;;
> +  --disable-coroutine-pool) coroutine_pool="no"
> +  ;;
> +  --enable-coroutine-pool) coroutine_pool="yes"
> +  ;;
>    --disable-docs) docs="no"
>    ;;
>    --enable-docs) docs="yes"
> @@ -1161,6 +1166,8 @@ echo "  --disable-seccomp        disable seccomp support"
>  echo "  --enable-seccomp         enables seccomp support"
>  echo "  --with-coroutine=BACKEND coroutine backend. Supported options:"
>  echo "                           gthread, ucontext, sigaltstack, windows"
> +echo "  --disable-coroutine-pool disable coroutine freelist (worse performance)"
> +echo "  --enable-coroutine-pool  enable coroutine freelist (better performance)"
>  echo "  --enable-glusterfs       enable GlusterFS backend"
>  echo "  --disable-glusterfs      disable GlusterFS backend"
>  echo "  --enable-gcov            enable test coverage analysis with gcov"
> @@ -3279,6 +3286,17 @@ else
>    esac
>  fi
>  
> +if test "$coroutine_pool" = ""; then
> +  if test "$coroutine" = "gthread"; then
> +    coroutine_pool=no
> +  else
> +    coroutine_pool=yes
> +  fi
> +fi
> +if test "$coroutine" = "gthread" -a "$coroutine_pool" = "yes"; then
> +  error_exit "'gthread' coroutine backend does not support pool (use --disable-coroutine-pool)"
> +fi
> +
>  ##########################################
>  # check if we have open_by_handle_at
>  
> @@ -3644,6 +3662,7 @@ echo "libiscsi support  $libiscsi"
>  echo "build guest agent $guest_agent"
>  echo "seccomp support   $seccomp"
>  echo "coroutine backend $coroutine"
> +echo "coroutine pool    $coroutine_pool"
>  echo "GlusterFS support $glusterfs"
>  echo "virtio-blk-data-plane $virtio_blk_data_plane"
>  echo "gcov              $gcov_tool"
> @@ -3999,6 +4018,11 @@ if test "$rbd" = "yes" ; then
>  fi
>  
>  echo "CONFIG_COROUTINE_BACKEND=$coroutine" >> $config_host_mak
> +if test "$coroutine_pool" = "yes" ; then
> +  echo "CONFIG_COROUTINE_POOL=1" >> $config_host_mak
> +else
> +  echo "CONFIG_COROUTINE_POOL=0" >> $config_host_mak
> +fi

Doesn't work.

In config-host.make we do get:

    CONFIG_COROUTINE_POOL=0

But when config-host.h is generated from it, I assume it's only checked
if the variable is defined, so we end up with:

    #define CONFIG_COROUTINE_POOL 1

That is, you never really disable the pool.

Kevin

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Gabriel Kerneis]

On Wed, Sep 11, 2013 at 05:24:55PM +0200, Kevin Wolf wrote:
> In config-host.make we do get:
> 
>     CONFIG_COROUTINE_POOL=0
> 
> But when config-host.h is generated from it, I assume it's only checked
> if the variable is defined, so we end up with:
> 
>     #define CONFIG_COROUTINE_POOL 1

Did you clean your tree?

$ rm -rf *
$ ../../configure --disable-coroutine-pool
$ make config-host.h
$ grep COROUTINE_POOL config-host.h
#define CONFIG_COROUTINE_POOL 0

And scripts/create_config seems to take the value into account:

 CONFIG_*=*) # configuration
    name=${line%=*}
    value=${line#*=}
    echo "#define $name $value"
    ;;

-- 
Gabriel

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Kevin Wolf]

Am 11.09.2013 um 17:32 hat Gabriel Kerneis geschrieben:
> On Wed, Sep 11, 2013 at 05:24:55PM +0200, Kevin Wolf wrote:
> > In config-host.make we do get:
> > 
> >     CONFIG_COROUTINE_POOL=0
> > 
> > But when config-host.h is generated from it, I assume it's only checked
> > if the variable is defined, so we end up with:
> > 
> >     #define CONFIG_COROUTINE_POOL 1
> 
> Did you clean your tree?
> 
> $ rm -rf *
> $ ../../configure --disable-coroutine-pool
> $ make config-host.h
> $ grep COROUTINE_POOL config-host.h
> #define CONFIG_COROUTINE_POOL 0

My bad, I checked after configure, but before running make. We must have
a weird build system that this isn't created during configure. :-)

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Stefan Hajnoczi]

On Wed, Sep 11, 2013 at 05:38:07PM +0200, Kevin Wolf wrote:
> Am 11.09.2013 um 17:32 hat Gabriel Kerneis geschrieben:
> > On Wed, Sep 11, 2013 at 05:24:55PM +0200, Kevin Wolf wrote:
> > > In config-host.make we do get:
> > > 
> > >     CONFIG_COROUTINE_POOL=0
> > > 
> > > But when config-host.h is generated from it, I assume it's only checked
> > > if the variable is defined, so we end up with:
> > > 
> > >     #define CONFIG_COROUTINE_POOL 1
> > 
> > Did you clean your tree?
> > 
> > $ rm -rf *
> > $ ../../configure --disable-coroutine-pool
> > $ make config-host.h
> > $ grep COROUTINE_POOL config-host.h
> > #define CONFIG_COROUTINE_POOL 0
> 
> My bad, I checked after configure, but before running make. We must have
> a weird build system that this isn't created during configure. :-)

Yes, this confused by too once or twice while writing this patch :).

The relevant code in scripts/create_config is:

 CONFIG_*=*) # configuration
    name=${line%=*}
    value=${line#*=}
    echo "#define $name $value"
    ;;

Stefan

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Stefan Weil]

Am 11.09.2013 16:42, schrieb Stefan Hajnoczi:
> The 'gthread' coroutine backend was written before the freelist (aka
> pool) existed in qemu-coroutine.c.
>
> This means that every thread is expected to exit when its coroutine
> terminates.  It is not possible to reuse threads from a pool.
>
> This patch automatically disables the pool when 'gthread' is used.  This
> allows the 'gthread' backend to work again (for example,
> tests/test-coroutine completes successfully instead of hanging).
>
> I considered implementing thread reuse but I don't want quirks like CPU
> affinity differences due to coroutine threads being recycled.  The
> 'gthread' backend is a reference backend and it's therefore okay to skip
> the pool optimization.
>
> Note this patch also makes it easy to toggle the pool for benchmarking
> purposes:
>
>   ./configure --with-coroutine-backend=ucontext \
>               --disable-coroutine-pool
>
> Reported-by: Gabriel Kerneis <gabriel@kerneis.info>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  configure        | 24 ++++++++++++++++++++++++
>  qemu-coroutine.c | 34 +++++++++++++++++++---------------
>  2 files changed, 43 insertions(+), 15 deletions(-)
>

This patch is important for QEMU 1.5 as well, but needs some
modifications there.
A recent bug report for MinGW shows that the win32 coroutine needs it, too.

Stefan, could you please prepare a patch for stable-1.5?

Regards
Stefan W.

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Stefan Hajnoczi]

On Fri, Sep 27, 2013 at 07:20:21AM +0200, Stefan Weil wrote:
> Am 11.09.2013 16:42, schrieb Stefan Hajnoczi:
> > The 'gthread' coroutine backend was written before the freelist (aka
> > pool) existed in qemu-coroutine.c.
> >
> > This means that every thread is expected to exit when its coroutine
> > terminates.  It is not possible to reuse threads from a pool.
> >
> > This patch automatically disables the pool when 'gthread' is used.  This
> > allows the 'gthread' backend to work again (for example,
> > tests/test-coroutine completes successfully instead of hanging).
> >
> > I considered implementing thread reuse but I don't want quirks like CPU
> > affinity differences due to coroutine threads being recycled.  The
> > 'gthread' backend is a reference backend and it's therefore okay to skip
> > the pool optimization.
> >
> > Note this patch also makes it easy to toggle the pool for benchmarking
> > purposes:
> >
> >   ./configure --with-coroutine-backend=ucontext \
> >               --disable-coroutine-pool
> >
> > Reported-by: Gabriel Kerneis <gabriel@kerneis.info>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  configure        | 24 ++++++++++++++++++++++++
> >  qemu-coroutine.c | 34 +++++++++++++++++++---------------
> >  2 files changed, 43 insertions(+), 15 deletions(-)
> >
> 
> This patch is important for QEMU 1.5 as well, but needs some
> modifications there.
> A recent bug report for MinGW shows that the win32 coroutine needs it, too.

coroutine-win32.c is designed to support reuse:

static void CALLBACK coroutine_trampoline(void *co_)
{
    Coroutine *co = co_;

    while (true) {
        co->entry(co->entry_arg);
        qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
    }
}

We return from qemu_coroutine_switch() when the fiber is reused and
simply run another iteration of the while loop.

Why do you say win32 coroutines should disable the pool?

Stefan

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Stefan Weil]

Am 27.09.2013 11:11, schrieb Stefan Hajnoczi:
> On Fri, Sep 27, 2013 at 07:20:21AM +0200, Stefan Weil wrote:
>> Am 11.09.2013 16:42, schrieb Stefan Hajnoczi:
>>> The 'gthread' coroutine backend was written before the freelist (aka
>>> pool) existed in qemu-coroutine.c.
>>>
>>> This means that every thread is expected to exit when its coroutine
>>> terminates.  It is not possible to reuse threads from a pool.
>>>
>>> This patch automatically disables the pool when 'gthread' is used.  This
>>> allows the 'gthread' backend to work again (for example,
>>> tests/test-coroutine completes successfully instead of hanging).
>>>
>>> I considered implementing thread reuse but I don't want quirks like CPU
>>> affinity differences due to coroutine threads being recycled.  The
>>> 'gthread' backend is a reference backend and it's therefore okay to skip
>>> the pool optimization.
>>>
>>> Note this patch also makes it easy to toggle the pool for benchmarking
>>> purposes:
>>>
>>>   ./configure --with-coroutine-backend=ucontext \
>>>               --disable-coroutine-pool
>>>
>>> Reported-by: Gabriel Kerneis <gabriel@kerneis.info>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  configure        | 24 ++++++++++++++++++++++++
>>>  qemu-coroutine.c | 34 +++++++++++++++++++---------------
>>>  2 files changed, 43 insertions(+), 15 deletions(-)
>>>
>> This patch is important for QEMU 1.5 as well, but needs some
>> modifications there.
>> A recent bug report for MinGW shows that the win32 coroutine needs it, too.
> coroutine-win32.c is designed to support reuse:
>
> static void CALLBACK coroutine_trampoline(void *co_)
> {
>     Coroutine *co = co_;
>
>     while (true) {
>         co->entry(co->entry_arg);
>         qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
>     }
> }
>
> We return from qemu_coroutine_switch() when the fiber is reused and
> simply run another iteration of the while loop.
>
> Why do you say win32 coroutines should disable the pool?
>
> Stefan

QEMU MinGW binaries with coroutine pool simply crash, and disabling the
pool helps
for the moment until we have a better fix. That's a regression which was
introduced
with a patch which added the pool for win32.

See this discussion thread for more details:
http://lists.nongnu.org/archive/html/qemu-devel/2013-09/msg04195.html

Regards,
Stefan

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Stefan Hajnoczi]

On Fri, Sep 27, 2013 at 06:49:18PM +0200, Stefan Weil wrote:
> Am 27.09.2013 11:11, schrieb Stefan Hajnoczi:
> > On Fri, Sep 27, 2013 at 07:20:21AM +0200, Stefan Weil wrote:
> >> Am 11.09.2013 16:42, schrieb Stefan Hajnoczi:
> >>> The 'gthread' coroutine backend was written before the freelist (aka
> >>> pool) existed in qemu-coroutine.c.
> >>>
> >>> This means that every thread is expected to exit when its coroutine
> >>> terminates.  It is not possible to reuse threads from a pool.
> >>>
> >>> This patch automatically disables the pool when 'gthread' is used.  This
> >>> allows the 'gthread' backend to work again (for example,
> >>> tests/test-coroutine completes successfully instead of hanging).
> >>>
> >>> I considered implementing thread reuse but I don't want quirks like CPU
> >>> affinity differences due to coroutine threads being recycled.  The
> >>> 'gthread' backend is a reference backend and it's therefore okay to skip
> >>> the pool optimization.
> >>>
> >>> Note this patch also makes it easy to toggle the pool for benchmarking
> >>> purposes:
> >>>
> >>>   ./configure --with-coroutine-backend=ucontext \
> >>>               --disable-coroutine-pool
> >>>
> >>> Reported-by: Gabriel Kerneis <gabriel@kerneis.info>
> >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>> ---
> >>>  configure        | 24 ++++++++++++++++++++++++
> >>>  qemu-coroutine.c | 34 +++++++++++++++++++---------------
> >>>  2 files changed, 43 insertions(+), 15 deletions(-)
> >>>
> >> This patch is important for QEMU 1.5 as well, but needs some
> >> modifications there.
> >> A recent bug report for MinGW shows that the win32 coroutine needs it, too.
> > coroutine-win32.c is designed to support reuse:
> >
> > static void CALLBACK coroutine_trampoline(void *co_)
> > {
> >     Coroutine *co = co_;
> >
> >     while (true) {
> >         co->entry(co->entry_arg);
> >         qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
> >     }
> > }
> >
> > We return from qemu_coroutine_switch() when the fiber is reused and
> > simply run another iteration of the while loop.
> >
> > Why do you say win32 coroutines should disable the pool?
> >
> > Stefan
> 
> QEMU MinGW binaries with coroutine pool simply crash, and disabling the
> pool helps
> for the moment until we have a better fix. That's a regression which was
> introduced
> with a patch which added the pool for win32.
> 
> See this discussion thread for more details:
> http://lists.nongnu.org/archive/html/qemu-devel/2013-09/msg04195.html

I'd like to fix the win32 coroutine backend.

In your email you said it can be reproduced under wine on Linux.  I
wasn't able to reproduce it with this command-line:

  $ wine qemu-system-x86_64.exe -m 1024 -vnc :1 -L ../pc-bios -hda ../test.img

Stefan: Do you have a ./configure and QEMU command-line which reproduces
the assertion failure under wine?

Stefan

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Stefan Weil]

Am 30.09.2013 11:06, schrieb Stefan Hajnoczi:
> On Fri, Sep 27, 2013 at 06:49:18PM +0200, Stefan Weil wrote:
>> Am 27.09.2013 11:11, schrieb Stefan Hajnoczi:
>>> On Fri, Sep 27, 2013 at 07:20:21AM +0200, Stefan Weil wrote:
>>>> Am 11.09.2013 16:42, schrieb Stefan Hajnoczi:
>>>>> The 'gthread' coroutine backend was written before the freelist (aka
>>>>> pool) existed in qemu-coroutine.c.
>>>>>
>>>>> This means that every thread is expected to exit when its coroutine
>>>>> terminates.  It is not possible to reuse threads from a pool.
>>>>>
>>>>> This patch automatically disables the pool when 'gthread' is used.  This
>>>>> allows the 'gthread' backend to work again (for example,
>>>>> tests/test-coroutine completes successfully instead of hanging).
>>>>>
>>>>> I considered implementing thread reuse but I don't want quirks like CPU
>>>>> affinity differences due to coroutine threads being recycled.  The
>>>>> 'gthread' backend is a reference backend and it's therefore okay to skip
>>>>> the pool optimization.
>>>>>
>>>>> Note this patch also makes it easy to toggle the pool for benchmarking
>>>>> purposes:
>>>>>
>>>>>   ./configure --with-coroutine-backend=ucontext \
>>>>>               --disable-coroutine-pool
>>>>>
>>>>> Reported-by: Gabriel Kerneis <gabriel@kerneis.info>
>>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>> ---
>>>>>  configure        | 24 ++++++++++++++++++++++++
>>>>>  qemu-coroutine.c | 34 +++++++++++++++++++---------------
>>>>>  2 files changed, 43 insertions(+), 15 deletions(-)
>>>>>
>>>> This patch is important for QEMU 1.5 as well, but needs some
>>>> modifications there.
>>>> A recent bug report for MinGW shows that the win32 coroutine needs it, too.
>>> coroutine-win32.c is designed to support reuse:
>>>
>>> static void CALLBACK coroutine_trampoline(void *co_)
>>> {
>>>     Coroutine *co = co_;
>>>
>>>     while (true) {
>>>         co->entry(co->entry_arg);
>>>         qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
>>>     }
>>> }
>>>
>>> We return from qemu_coroutine_switch() when the fiber is reused and
>>> simply run another iteration of the while loop.
>>>
>>> Why do you say win32 coroutines should disable the pool?
>>>
>>> Stefan
>> QEMU MinGW binaries with coroutine pool simply crash, and disabling the
>> pool helps
>> for the moment until we have a better fix. That's a regression which was
>> introduced
>> with a patch which added the pool for win32.
>>
>> See this discussion thread for more details:
>> http://lists.nongnu.org/archive/html/qemu-devel/2013-09/msg04195.html
> I'd like to fix the win32 coroutine backend.
>
> In your email you said it can be reproduced under wine on Linux.  I
> wasn't able to reproduce it with this command-line:
>
>   $ wine qemu-system-x86_64.exe -m 1024 -vnc :1 -L ../pc-bios -hda ../test.img
>
> Stefan: Do you have a ./configure and QEMU command-line which reproduces
> the assertion failure under wine?
>
> Stefan

Here is my current test environment:

* Debian wheezy x86_64 host (running on a KVM server)
* latest QEMU sources

$ ./configure' '--cross-prefix=i686-w64-mingw32-'
'--enable-trace-backend=stderr' --target-list=i386-softmmu && make
# Get Debian installer CDROM mini.iso.
$ wine i386-softmmu/qemu-system-i386 -L pc-bios -cdrom /var/tmp/mini.iso
-sdl # or -vnc :1

=> Assertion in qemu-coroutine-lock.c:99.

I also tested a similar scenario on an ASUS netbook with Ubuntu some
days ago and got the same result.

Regards,
Stefan

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Paolo Bonzini]

Il 30/09/2013 11:06, Stefan Hajnoczi ha scritto:
> I'd like to fix the win32 coroutine backend.
> 
> In your email you said it can be reproduced under wine on Linux.  I
> wasn't able to reproduce it with this command-line:
> 
>   $ wine qemu-system-x86_64.exe -m 1024 -vnc :1 -L ../pc-bios -hda ../test.img
> 
> Stefan: Do you have a ./configure and QEMU command-line which reproduces
> the assertion failure under wine?

Neither could I.  The patch was definitely tested under Wine.

Paolo

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Gabriel Kerneis]

On Tue, Oct 01, 2013 at 07:51:24AM +0200, Stefan Weil wrote:
> $ wine i386-softmmu/qemu-system-i386 -L pc-bios -cdrom /var/tmp/mini.iso
> -sdl # or -vnc :1
> 
> => Assertion in qemu-coroutine-lock.c:99.

Could you please provide the backtrace for this?  I am curious which of the uses
of qemu_co_queue_restart_all() fails.  I wonder if it is a bug in the
implementation of qemu_in_coroutine() on your platform, or really a rare
interleaving which exhibits a bug in the logic of coroutine functions.

-- 
Gabriel

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Stefan Weil]

Am 01.10.2013 09:29, schrieb Gabriel Kerneis:
> On Tue, Oct 01, 2013 at 07:51:24AM +0200, Stefan Weil wrote:
>> $ wine i386-softmmu/qemu-system-i386 -L pc-bios -cdrom /var/tmp/mini.iso
>> -sdl # or -vnc :1
>>
>> => Assertion in qemu-coroutine-lock.c:99.
> Could you please provide the backtrace for this?  I am curious which of the uses
> of qemu_co_queue_restart_all() fails.  I wonder if it is a bug in the
> implementation of qemu_in_coroutine() on your platform, or really a rare
> interleaving which exhibits a bug in the logic of coroutine functions.
>

Here is a GDB protocol. Build environment:

Windows 7 (64 bit) host
MinGW toolchain (not MinGW-w64 which I usually prefer)
./configure && make

$ gdb --args bin/ndebug/mingw32/i386-softmmu/qemu-system-i386 -L pc-bios
-cdrom mini.iso
GNU gdb (GDB) 7.5
Copyright (C) 2012 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later
<http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-mingw32".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from
c:\home\stefan\src\qemu\qemu.org\qemu\bin\ndebug\mingw32\i386-softmmu\qemu-system-i386.exe...done.
(gdb) b abort
Breakpoint 1 at 0x6b0070
(gdb) r
Starting program:
c:\home\stefan\src\qemu\qemu.org\qemu\bin\ndebug\mingw32\i386-softmmu\qemu-system-i386.exe
-L pc-bios -cdrom mini.iso
[New Thread 4120.0x19c4]
[New Thread 4120.0x1724]
[New Thread 4120.0x1a98]
[New Thread 4120.0x1854]
[New Thread 4120.0x1bf8]
VNC server running on `::1:5900'
Assertion failed: qemu_in_coroutine(), file
c:/home/stefan/src/qemu/qemu.org/qemu/qemu-coroutine-lock.c, line 99
[Switching to Thread 4120.0x1a98]

Breakpoint 1, 0x76118e76 in msvcrt!abort ()
   from C:\Windows\syswow64\msvcrt.dll
(gdb) i s
#0  0x76118e76 in msvcrt!abort () from C:\Windows\syswow64\msvcrt.dll
#1  0x7611680c in msvcrt!_assert () from C:\Windows\syswow64\msvcrt.dll
#2  0x00518f2d in qemu_co_queue_restart_all (queue=queue@entry=0x6d3fe90)
    at c:/home/stefan/src/qemu/qemu.org/qemu/qemu-coroutine-lock.c:99
#3  0x0040ee81 in tracked_request_end (req=0x6d3fe6c)
    at c:/home/stefan/src/qemu/qemu.org/qemu/block.c:1963
#4  bdrv_co_do_readv (bs=0x3057658, sector_num=<optimized out>,
nb_sectors=4,
    qiov=0x733f9d0, flags=<optimized out>)
    at c:/home/stefan/src/qemu/qemu.org/qemu/block.c:2675
#5  0x0040ee52 in bdrv_co_do_readv (bs=0x3055c10, sector_num=<optimized
out>,
    nb_sectors=4, qiov=0x733f9d0, flags=<optimized out>)
    at c:/home/stefan/src/qemu/qemu.org/qemu/block.c:2645
#6  0x0040ffbc in bdrv_rw_co_entry (opaque=0x733f968)
    at c:/home/stefan/src/qemu/qemu.org/qemu/block.c:2276
#7  0x00441be8 in coroutine_trampoline (co_=0x3058958)
    at c:/home/stefan/src/qemu/qemu.org/qemu/coroutine-win32.c:57
#8  0x7549bfa2 in KERNEL32!GetQueuedCompletionStatus ()
   from C:\Windows\syswow64\kernel32.dll
#9  0x03058958 in ?? ()
#10 0x7549bf5a in KERNEL32!GetQueuedCompletionStatus ()
   from C:\Windows\syswow64\kernel32.dll
#11 0x014feff0 in ?? ()

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Stefan Hajnoczi]

On Tue, Oct 01, 2013 at 06:44:54PM +0200, Stefan Weil wrote:
> Am 01.10.2013 09:29, schrieb Gabriel Kerneis:
> > On Tue, Oct 01, 2013 at 07:51:24AM +0200, Stefan Weil wrote:
> >> $ wine i386-softmmu/qemu-system-i386 -L pc-bios -cdrom /var/tmp/mini.iso
> >> -sdl # or -vnc :1
> >>
> >> => Assertion in qemu-coroutine-lock.c:99.
> > Could you please provide the backtrace for this?  I am curious which of the uses
> > of qemu_co_queue_restart_all() fails.  I wonder if it is a bug in the
> > implementation of qemu_in_coroutine() on your platform, or really a rare
> > interleaving which exhibits a bug in the logic of coroutine functions.
> >
> 
> Here is a GDB protocol. Build environment:
> 
> Windows 7 (64 bit) host
> MinGW toolchain (not MinGW-w64 which I usually prefer)
> ./configure && make
> 
> $ gdb --args bin/ndebug/mingw32/i386-softmmu/qemu-system-i386 -L pc-bios
> -cdrom mini.iso
> GNU gdb (GDB) 7.5
> Copyright (C) 2012 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later
> <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "i686-pc-mingw32".
> For bug reporting instructions, please see:
> <http://www.gnu.org/software/gdb/bugs/>...
> Reading symbols from
> c:\home\stefan\src\qemu\qemu.org\qemu\bin\ndebug\mingw32\i386-softmmu\qemu-system-i386.exe...done.
> (gdb) b abort
> Breakpoint 1 at 0x6b0070
> (gdb) r
> Starting program:
> c:\home\stefan\src\qemu\qemu.org\qemu\bin\ndebug\mingw32\i386-softmmu\qemu-system-i386.exe
> -L pc-bios -cdrom mini.iso
> [New Thread 4120.0x19c4]
> [New Thread 4120.0x1724]
> [New Thread 4120.0x1a98]
> [New Thread 4120.0x1854]
> [New Thread 4120.0x1bf8]
> VNC server running on `::1:5900'
> Assertion failed: qemu_in_coroutine(), file
> c:/home/stefan/src/qemu/qemu.org/qemu/qemu-coroutine-lock.c, line 99
> [Switching to Thread 4120.0x1a98]
> 
> Breakpoint 1, 0x76118e76 in msvcrt!abort ()
>    from C:\Windows\syswow64\msvcrt.dll
> (gdb) i s
> #0  0x76118e76 in msvcrt!abort () from C:\Windows\syswow64\msvcrt.dll
> #1  0x7611680c in msvcrt!_assert () from C:\Windows\syswow64\msvcrt.dll
> #2  0x00518f2d in qemu_co_queue_restart_all (queue=queue@entry=0x6d3fe90)
>     at c:/home/stefan/src/qemu/qemu.org/qemu/qemu-coroutine-lock.c:99
> #3  0x0040ee81 in tracked_request_end (req=0x6d3fe6c)
>     at c:/home/stefan/src/qemu/qemu.org/qemu/block.c:1963
> #4  bdrv_co_do_readv (bs=0x3057658, sector_num=<optimized out>,
> nb_sectors=4,
>     qiov=0x733f9d0, flags=<optimized out>)
>     at c:/home/stefan/src/qemu/qemu.org/qemu/block.c:2675
> #5  0x0040ee52 in bdrv_co_do_readv (bs=0x3055c10, sector_num=<optimized
> out>,
>     nb_sectors=4, qiov=0x733f9d0, flags=<optimized out>)
>     at c:/home/stefan/src/qemu/qemu.org/qemu/block.c:2645
> #6  0x0040ffbc in bdrv_rw_co_entry (opaque=0x733f968)
>     at c:/home/stefan/src/qemu/qemu.org/qemu/block.c:2276
> #7  0x00441be8 in coroutine_trampoline (co_=0x3058958)
>     at c:/home/stefan/src/qemu/qemu.org/qemu/coroutine-win32.c:57
> #8  0x7549bfa2 in KERNEL32!GetQueuedCompletionStatus ()
>    from C:\Windows\syswow64\kernel32.dll
> #9  0x03058958 in ?? ()
> #10 0x7549bf5a in KERNEL32!GetQueuedCompletionStatus ()
>    from C:\Windows\syswow64\kernel32.dll
> #11 0x014feff0 in ?? ()

This is an interesting backtrace.  The 'current' thread-local variable
from coroutine-win32.c is NULL or doesn't have a caller assigned.

Please post 'thread apply all bt' so we can identify the other threads.

Stefan

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Stefan Weil]

Am 02.10.2013 10:54, schrieb Stefan Hajnoczi:
> This is an interesting backtrace.  The 'current' thread-local variable
> from coroutine-win32.c is NULL or doesn't have a caller assigned.
>
> Please post 'thread apply all bt' so we can identify the other threads.
>
> Stefan

The other threads don't have a reasonable backtrace:

(gdb) thread apply all bt

Thread 3 (Thread 5708.0x15bc):
#0  0x76118e76 in msvcrt!abort () from C:\Windows\syswow64\msvcrt.dll
#1  0x7611680c in msvcrt!_assert () from C:\Windows\syswow64\msvcrt.dll
#2  0x0051957d in qemu_co_queue_restart_all (queue=queue@entry=0x6e5fe90)
    at c:/cygwin/home/stweil/src/qemu/qemu.org/qemu/qemu-coroutine-lock.c:99
#3  0x0040f4d1 in tracked_request_end (req=0x6e5fe6c)
    at c:/cygwin/home/stweil/src/qemu/qemu.org/qemu/block.c:1963
#4  bdrv_co_do_readv (bs=0x31e78f0, sector_num=<optimized out>,
nb_sectors=4,
    qiov=0x745f9d0, flags=<optimized out>)
    at c:/cygwin/home/stweil/src/qemu/qemu.org/qemu/block.c:2675
#5  0x0040f4a2 in bdrv_co_do_readv (bs=0x31e5ea8, sector_num=<optimized
out>,
    nb_sectors=4, qiov=0x745f9d0, flags=<optimized out>)
    at c:/cygwin/home/stweil/src/qemu/qemu.org/qemu/block.c:2645
#6  0x0041060c in bdrv_rw_co_entry (opaque=0x745f968)
    at c:/cygwin/home/stweil/src/qemu/qemu.org/qemu/block.c:2276
#7  0x00442238 in coroutine_trampoline (co_=0x31e8bf0)
    at c:/cygwin/home/stweil/src/qemu/qemu.org/qemu/coroutine-win32.c:57
#8  0x7549bfa2 in KERNEL32!GetQueuedCompletionStatus ()
   from C:\Windows\syswow64\kernel32.dll
#9  0x031e8bf0 in ?? ()
#10 0x7549bf5a in KERNEL32!GetQueuedCompletionStatus ()
   from C:\Windows\syswow64\kernel32.dll
#11 0x016d24e0 in ?? ()

Thread 2 (Thread 5708.0xe1c):
#0  0x7749f8d1 in ntdll!RtlUpdateClonedSRWLock ()
   from C:\Windows\system32\ntdll.dll
#1  0x7749f8d1 in ntdll!RtlUpdateClonedSRWLock ()
   from C:\Windows\system32\ntdll.dll
#2  0x759c149d in WaitForSingleObjectEx ()
   from C:\Windows\syswow64\KernelBase.dll
#3  0x00000150 in ?? ()
#4  0x00000000 in ?? ()

Thread 1 (Thread 5708.0x1600):
#0  0x7749f8d1 in ntdll!RtlUpdateClonedSRWLock ()
   from C:\Windows\system32\ntdll.dll
#1  0x7749f8d1 in ntdll!RtlUpdateClonedSRWLock ()
   from C:\Windows\system32\ntdll.dll
#2  0x774b8e44 in ntdll!TpCallbackMayRunLong ()
   from C:\Windows\system32\ntdll.dll
#3  0x00000160 in ?? ()
#4  0x00000000 in ?? ()

Re: [Qemu-devel] [PATCH] coroutine: add ./configure --disable-coroutine-pool

[Stefan Hajnoczi]

On Wed, Oct 02, 2013 at 11:15:56PM +0200, Stefan Weil wrote:
> Am 02.10.2013 10:54, schrieb Stefan Hajnoczi:
> > This is an interesting backtrace.  The 'current' thread-local variable
> > from coroutine-win32.c is NULL or doesn't have a caller assigned.
> >
> > Please post 'thread apply all bt' so we can identify the other threads.
> >
> > Stefan
> 
> The other threads don't have a reasonable backtrace:

Okay.  I've been able to reproduce this with the Debian netinst i386 ISO
and mingw32.

I'll take a look at fixing this.

Stefan