Re: [Qemu-devel] [PATCH 1/2] coroutine: introduce coroutines

[Stefan Weil]

Am 11.05.2011 12:15, schrieb Stefan Hajnoczi:
> From: Kevin Wolf <address@hidden>
>
> Asynchronous code is becoming very complex. At the same time
> synchronous code is growing because it is convenient to write.
> Sometimes duplicate code paths are even added, one synchronous and the
> other asynchronous. This patch introduces coroutines which allow code
> that looks synchronous but is asynchronous under the covers.
>
> A coroutine has its own stack and is therefore able to preserve state
> across blocking operations, which traditionally require callback
> functions and manual marshalling of parameters.
>
> Creating and starting a coroutine is easy:
>
> coroutine = qemu_coroutine_create(my_coroutine);
> qemu_coroutine_enter(coroutine, my_data);
>
> The coroutine then executes until it returns or yields:
>
> void coroutine_fn my_coroutine(void *opaque) {
> MyData *my_data = opaque;
>
> /* do some work */
>
> qemu_coroutine_yield();
>
> /* do some more work */
> }
>
> Yielding switches control back to the caller of qemu_coroutine_enter().
> This is typically used to switch back to the main thread's event loop
> after issuing an asynchronous I/O request. The request callback will
> then invoke qemu_coroutine_enter() once more to switch back to the
> coroutine.
>
> Note that coroutines never execute concurrently and should only be used
> from threads which hold the global mutex. This restriction makes
> programming with coroutines easier than with threads. Race conditions
> cannot occur since only one coroutine may be active at any time. Other
> coroutines can only run across yield.
>
> This coroutines implementation is based on the gtk-vnc implementation
> written by Anthony Liguori <address@hidden> but it has been
> significantly rewritten by Kevin Wolf <address@hidden> to use
> setjmp()/longjmp() instead of the more expensive swapcontext().
>
> Signed-off-by: Kevin Wolf <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---


Hi Stefan,

you might want to add the following or a similar patch:

diff --git a/qemu-coroutine.c b/qemu-coroutine.c
index 0927f58..9cd0dd7 100644
--- a/qemu-coroutine.c
+++ b/qemu-coroutine.c
@@ -91,7 +91,10 @@ static void *coroutine_swap(Coroutine *from, 
Coroutine *to, void *opaque)
     case COROUTINE_TERMINATE:
         current = to->caller;
         qemu_coroutine_terminate(to);
-        return to->data;
+        opaque = to->data;
+        qemu_free(to->stack);
+        qemu_free(to);
+        return opaque;
     default:
         /* Switch to called coroutine */
         current = to;

I tested your test code with Valgrind. Beside of the memory leaks which 
are fixed
with the small modification shown above, Valgrind has a lot of complains.
Maybe you can try it yourself, otherwise please wait until I have finished
analyzing the Valgrind results. At a first glance, I'm afraid that
debugging with gdb or Valgrind might become more difficult when coroutines
are used. This is different with threads: they are fully supported by gdb.

The w32 build needs additional libraries (ws2_32, maybe more), then
check-coroutine works.

Cheers,
Stefan W.