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

[Aneesh Kumar K.V]

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. 

> 
> I've followed the main control flow once but am going to review in
> more detail to make sure.
> 

Thanks.
-aneesh