On Wed, Mar 06, 2013 at 04:03:49PM +0100, Paolo Bonzini wrote: > Il 06/03/2013 15:53, Stefan Hajnoczi ha scritto: > > CoQueue uses a BH to awake coroutines that were made ready to run again > > using qemu_co_queue_next() or qemu_co_queue_restart_all(). The BH > > currently runs in the iothread AioContext and would break coroutines > > that run in a different AioContext. > > > > This is a slightly tricky problem because the lifetime of the BH exceeds > > that of the CoQueue. This means coroutines can be awoken after CoQueue > > itself has been freed. Also, there is no qemu_co_queue_destroy() > > function which we could use to handle freeing resources. > > > > Introducing qemu_co_queue_destroy() has a ripple effect of requiring us > > to also add qemu_co_mutex_destroy() and qemu_co_rwlock_destroy(), as > > well as updating all callers. Avoid doing that. > > > > We also cannot switch from BH to GIdle function because aio_poll() does > > not dispatch GIdle functions. (GIdle functions make memory management > > slightly easier because they free themselves.) > > > > Finally, I don't want to move unlock_queue and unlock_bh into > > AioContext. That would break encapsulation - AioContext isn't supposed > > to know about CoQueue. > > > > This patch implements a different solution: each qemu_co_queue_next() or > > qemu_co_queue_restart_all() call creates a new BH and list of coroutines > > to wake up. Callers tend to invoke qemu_co_queue_next() and > > qemu_co_queue_restart_all() occasionally after blocking I/O, so creating > > a new BH for each call shouldn't be massively inefficient. > > > > Note that this patch does not add an interface for specifying the > > AioContext. That is left to future patches which will convert CoQueue, > > CoMutex, and CoRwlock to expose AioContext. > > > > Signed-off-by: Stefan Hajnoczi <[email protected]> > > --- > > include/block/coroutine.h | 1 + > > qemu-coroutine-lock.c | 59 > > ++++++++++++++++++++++++++++++++--------------- > > 2 files changed, 42 insertions(+), 18 deletions(-) > > > > diff --git a/include/block/coroutine.h b/include/block/coroutine.h > > index c31fae3..a978162 100644 > > --- a/include/block/coroutine.h > > +++ b/include/block/coroutine.h > > @@ -104,6 +104,7 @@ bool qemu_in_coroutine(void); > > */ > > typedef struct CoQueue { > > QTAILQ_HEAD(, Coroutine) entries; > > + AioContext *ctx; > > } CoQueue; > > > > /** > > diff --git a/qemu-coroutine-lock.c b/qemu-coroutine-lock.c > > index 97ef01c..ae986b3 100644 > > --- a/qemu-coroutine-lock.c > > +++ b/qemu-coroutine-lock.c > > @@ -29,28 +29,34 @@ > > #include "block/aio.h" > > #include "trace.h" > > > > -static QTAILQ_HEAD(, Coroutine) unlock_bh_queue = > > - QTAILQ_HEAD_INITIALIZER(unlock_bh_queue); > > -static QEMUBH* unlock_bh; > > +/* Coroutines are awoken from a BH to allow the current coroutine to > > complete > > + * its flow of execution. The BH may run after the CoQueue has been > > destroyed, > > + * so keep BH data in a separate heap-allocated struct. > > + */ > > +typedef struct { > > + QEMUBH *bh; > > + QTAILQ_HEAD(, Coroutine) entries; > > +} CoQueueNextData; > > > > static void qemu_co_queue_next_bh(void *opaque) > > { > > + CoQueueNextData *data = opaque; > > Coroutine *next; > > > > trace_qemu_co_queue_next_bh(); > > - while ((next = QTAILQ_FIRST(&unlock_bh_queue))) { > > - QTAILQ_REMOVE(&unlock_bh_queue, next, co_queue_next); > > + while ((next = QTAILQ_FIRST(&data->entries))) { > > + QTAILQ_REMOVE(&data->entries, next, co_queue_next); > > qemu_coroutine_enter(next, NULL); > > } > > + > > + qemu_bh_delete(data->bh); > > + g_slice_free(CoQueueNextData, data); > > } > > > > void qemu_co_queue_init(CoQueue *queue) > > { > > QTAILQ_INIT(&queue->entries); > > - > > - if (!unlock_bh) { > > - unlock_bh = qemu_bh_new(qemu_co_queue_next_bh, NULL); > > - } > > + queue->ctx = NULL; > > What about adding an accessor for qemu_aio_context and using it? Then > you can just use aio_bh_new in qemu_co_queue_do_restart.
Your wish is my command. I'll add this patch to the threadpool series where I've already introduced qemu_get_aio_context(). Stefan
