Re: [Qemu-devel] [PATCH] coroutine: Implement coroutines using gthread

[Stefan Hajnoczi]

On Fri, Jun 10, 2011 at 12:09 PM, Aneesh Kumar K.V
<address@hidden> wrote:
> On Fri, 10 Jun 2011 11:24:20 +0100, Stefan Hajnoczi <address@hidden> wrote:
>> On Thu, Jun 9, 2011 at 6:41 PM, Aneesh Kumar K.V
>> <address@hidden> wrote:
>> > On platforms that doesn't support makecontext use gthread
>> > based coroutine implementation.
>> >
>> > Signed-off-by: Aneesh Kumar K.V <address@hidden>
>> > ---
>> >
>> > NOTE: Tested on linux with force compliation of coroutine-gthread.c
>> >
>> >  Makefile.objs       |    5 ++
>> >  configure           |   18 +++++
>> >  coroutine-gthread.c |  172 
>> > +++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 195 insertions(+), 0 deletions(-)
>> >  create mode 100644 coroutine-gthread.c
>> >
>> > diff --git a/Makefile.objs b/Makefile.objs
>> > index 0f1d7df..d354d3c 100644
>> > --- a/Makefile.objs
>> > +++ b/Makefile.objs
>> > @@ -13,9 +13,14 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o 
>> > qemu-thread-posix.o
>> >  #######################################################################
>> >  # coroutines
>> >  coroutine-obj-y = qemu-coroutine.o qemu-coroutine-lock.o
>> > +ifeq ($(CONFIG_UCONTEXT_COROUTINE),y)
>> >  coroutine-obj-$(CONFIG_POSIX) += coroutine-ucontext.o
>> > +else
>> > +coroutine-obj-$(CONFIG_POSIX) += coroutine-gthread.o
>> > +endif
>> >  coroutine-obj-$(CONFIG_WIN32) += coroutine-win32.o
>> >
>> > +
>> >  #######################################################################
>> >  # block-obj-y is code used by both qemu system emulation and qemu-img
>> >
>> > diff --git a/configure b/configure
>> > index 980914a..529d8c4 100755
>> > --- a/configure
>> > +++ b/configure
>> > @@ -2568,6 +2568,20 @@ if test "$trace_backend" = "dtrace"; then
>> >  fi
>> >
>> >  ##########################################
>> > +# check if we have makecontext
>> > +
>> > +ucontext_coroutine=no
>> > +cat > $TMPC << EOF
>> > +#include <ucontext.h>
>> > +int main(void) { makecontext(0, 0, 0); }
>> > +EOF
>> > +if compile_prog "" "" ; then
>> > +    ucontext_coroutine=yes
>> > +fi
>> > +
>> > +
>> > +
>> > +##########################################
>> >  # End of CC checks
>> >  # After here, no more $cc or $ld runs
>> >
>> > @@ -3031,6 +3045,10 @@ if test "$rbd" = "yes" ; then
>> >   echo "CONFIG_RBD=y" >> $config_host_mak
>> >  fi
>> >
>> > +if test "$ucontext_coroutine" = "yes" ; then
>> > +  echo "CONFIG_UCONTEXT_COROUTINE=y" >> $config_host_mak
>> > +fi
>> > +
>> >  # USB host support
>> >  case "$usb" in
>> >  linux)
>> > diff --git a/coroutine-gthread.c b/coroutine-gthread.c
>> > new file mode 100644
>> > index 0000000..37e5a16
>> > --- /dev/null
>> > +++ b/coroutine-gthread.c
>> > @@ -0,0 +1,172 @@
>> > +/*
>>
>> A summary of the purpose of this file would be nice.
>>
>> > + *
>> > + * Copyright (C) 2006  Anthony Liguori <address@hidden>
>> > + * Copyright (C) 2011  Aneesh Kumar K.V <address@hidden>
>> > + *
>> > + * This library is free software; you can redistribute it and/or
>> > + * modify it under the terms of the GNU Lesser General Public
>> > + * License as published by the Free Software Foundation; either
>> > + * version 2.0 of the License, or (at your option) any later version.
>> > + *
>> > + * This library is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> > + * Lesser General Public License for more details.
>> > + *
>> > + * You should have received a copy of the GNU Lesser General Public
>> > + * License along with this library; if not, write to the Free Software
>> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  
>> > 02110-1301 USA
>>
>> The URL is preferred over the postal address:
>>
>>  * You should have received a copy of the GNU Lesser General Public
>>  * License along with this library; if not, see
>>  <http://www.gnu.org/licenses/>.
>
> inherited that header from gtk-vnc version of original file
>>
>> > + */
>> > +
>> > +#include "qemu-common.h"
>> > +#include "qemu-coroutine-int.h"
>> > +#include <glib.h>
>
> Will fix.
>
>>
>> It's good practice to include system headers first unless you are
>> explicitly setting a magic #define to affect the system header.
>>
>> > +
>> > +typedef struct {
>> > +    Coroutine qemu_co;
>> > +    GThread *thread;
>> > +    gboolean runnable;
>> > +} CoroutineGthread;
>> > +
>> > +typedef struct {
>> > +    /** Currently executing coroutine */
>> > +    CoroutineGthread *current;
>> > +
>> > +    /** The default coroutine */
>> > +    CoroutineGthread leader;
>> > +} CoroutineThreadState;
>> > +
>> > +static GCond *run_cond;
>> > +static GMutex *run_lock;
>> > +static pthread_key_t thread_state_key;
>>
>> Why pthread_key_t instead of GStaticPrivate?
>
> No specific reason other than I was not aware of GStaticPrivate.

It would be worth using that to avoid mixing GThread and pthreads
unnecessarily.  I still need to look at the rest of this patch so
don't worry about sending v2 yet.

Stefan