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

[Kevin Wolf]

Am 11.05.2011 21:12, schrieb 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;

No, this is wrong. qemu_coroutine_terminate() puts the coroutine back
into a pool, so you can't free it.

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

Hm, for me Valgrind doesn't show any memory leaks, only some
"uninitialised value of size 8" in the trampoline. I don't quite
understand these.

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

Yes, the debugging support is less than optimal. However, threads are
not the right comparison. Coroutines are meant to replace AIOCBs and
callbacks, and these are already hard to debug today. So I don't think
it becomes more difficult.

Kevin