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

[Stefan Hajnoczi]

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/>.

> + */
> +
> +#include "qemu-common.h"
> +#include "qemu-coroutine-int.h"
> +#include <glib.h>

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?

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

Stefan