Re: [Qemu-devel] [PATCH 01/16] aio: introduce aio_co_schedule and aio_co_wake

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3329 bytes --]

On Wed, Jan 18, 2017 at 04:40:29PM +0100, Paolo Bonzini wrote:
> On 18/01/2017 15:33, Stefan Hajnoczi wrote:
> > On Fri, Jan 13, 2017 at 02:17:16PM +0100, Paolo Bonzini wrote:
> >> +static void co_schedule_bh_cb(void *opaque)
> >> +{
> >> +    AioContext *ctx = opaque;
> >> +    QSLIST_HEAD(, Coroutine) straight, reversed;
> >> +
> >> +    QSLIST_MOVE_ATOMIC(&reversed, &ctx->scheduled_coroutines);
> >> +    QSLIST_INIT(&straight);
> >> +
> >> +    while (!QSLIST_EMPTY(&reversed)) {
> >> +        Coroutine *co = QSLIST_FIRST(&reversed);
> >> +        QSLIST_REMOVE_HEAD(&reversed, co_scheduled_next);
> >> +        QSLIST_INSERT_HEAD(&straight, co, co_scheduled_next);
> >> +    }
> >> +
> >> +    while (!QSLIST_EMPTY(&straight)) {
> >> +        Coroutine *co = QSLIST_FIRST(&straight);
> >> +        QSLIST_REMOVE_HEAD(&straight, co_scheduled_next);
> >> +        trace_aio_co_schedule_bh_cb(ctx, co);
> >> +        qemu_coroutine_enter(co);
> >> +    }
> >> +}
> > 
> > ctx->scheduled_coroutines is a specialized CoQueue.  Was there no way to
> > modify and then use CoQueue instead of open coding it?
> 
> First of all, I'm trying to avoid a circular dependency when CoQueue can
> use aio_co_schedule (indirectly through aio_co_wake) after patch 7.
> 
> Secondarily, co_schedule_bh_cb can perform a single pass on
> ctx->scheduled_coroutines because it will be rescheduled by
> aio_co_schedule.  The same is not true for qemu_co_queue_restart_all.
> 
> Also, CoQueue can have multiple consumers, while scheduled_coroutines
> cannot.  Currently, CoQueue needs no thread-safety because it's
> protected by AioContext and/or by the non-preemptive nature of
> coroutines.  Later, it's going to be protected by an external CoMutex,
> just like a mutex/condvar pair.  scheduled_coroutines is different in
> this respect.

Thanks for explaining.

> >> +void aio_co_wake(struct Coroutine *co)
> >> +{
> >> +    AioContext *ctx;
> >> +
> >> +    /* Read coroutine before co->ctx.  Matches smp_wmb in
> >> +     * qemu_coroutine_enter.
> >> +     */
> >> +    smp_read_barrier_depends();
> >> +    ctx = atomic_read(&co->ctx);
> >> +
> >> +    if (ctx != qemu_get_current_aio_context()) {
> >> +        aio_co_schedule(ctx, co);
> >> +        return;
> >> +    }
> >> +
> >> +    if (qemu_in_coroutine()) {
> >> +        Coroutine *self = qemu_coroutine_self();
> >> +        assert(self != co);
> >> +        QSIMPLEQ_INSERT_TAIL(&self->co_queue_wakeup, co, co_queue_next);
> >> +    } else {
> >> +        aio_context_acquire(ctx);
> >> +        qemu_coroutine_enter(co);
> >> +        aio_context_release(ctx);
> > 
> > Why is it necessary to acquire AioContext here?  We're already in ctx.
> 
> We're in its thread, but we've not necessarily acquired it yet.
> aio_co_wake is called "aio_*" because it's a central place for
> AioContext to acquire itself for coroutines.  This way, coroutines only
> care about CoMutexes, and not about AioContext.  This was "highly
> recommended" :) by Kevin last year and it's the main change since the
> previous posting
> (https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg05416.html for
> example).

I see.  That also explains why we don't need to acquire AioContext if
we're already in a coroutine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]