Re: [Qemu-devel] [PATCH 05/16] io: make qio_channel_yield aware of AioContexts

[Paolo Bonzini]

On 18/01/2017 15:58, Stefan Hajnoczi wrote:
> On Fri, Jan 13, 2017 at 02:17:20PM +0100, Paolo Bonzini wrote:
>>  /**
>> + * qio_channel_set_aio_context:
>> + * @ioc: the channel object
>> + * @ctx: the #AioContext to set the handlers on
>> + *
>> + * Request that qio_channel_yield() sets I/O handlers on
>> + * the given #AioContext.  If @ctx is %NULL, qio_channel_yield()
>> + * uses QEMU's main thread event loop.
>> + */
>> +void qio_channel_set_aio_context(QIOChannel *ioc,
>> +                                 AioContext *ctx);
>> +
>> +/**
>> + * qio_channel_detach_aio_context:
>> + * @ioc: the channel object
>> + *
>> + * Disable any I/O handlers set by qio_channel_yield().  With the
>> + * help of aio_co_schedule(), this allows moving a coroutine that was
>> + * paused by qio_channel_yield() to another context.
>> + */
>> +void qio_channel_detach_aio_context(QIOChannel *ioc);
> 
> The block layer's bdrv_set_aio_context() has different semantics.  It
> invokes .detach()/.attach() callbacks and does AioContext locking so the
> function can be called safely even while the block driver is waiting for
> events.
> 
> It's unfortunate to that the block and io channel APIs act differently
> despite having similar names.  Was there a reason to choose different
> semantics?

Hmm, it's true.  I had forgotten that bdrv_set_aio_context exists.

set_aio_context can be called from the block layer attach callback, but
it's not enough alone (you need aio_co_schedule too) so I didn't want to
call the function qio_channel_attach_aio_context.  But maybe it *is* a
better name, I'll go for it in v2.

By the way, v2 will have a better comment on how to use the API:

+ * You can move a #QIOChannel from an #AioContext to another even if
+ * I/O handlers are set for a coroutine.  However, #QIOChannel provides
+ * no synchronization between the calls to qio_channel_yield() and
+ * qio_channel_set_aio_context().
+ *
+ * Therefore you should first call qio_channel_detach_aio_context()
+ * to ensure that the coroutine is not entered concurrently.  Then,
+ * while the coroutine has yielded, call qio_channel_set_aio_context(),
+ * and then aio_co_schedule() to place the coroutine on the new
+ * #AioContext.  The calls to qio_channel_detach_aio_context()
+ * and qio_channel_set_aio_context() should be protected with
+ * aio_context_acquire() and aio_context_release().

The "while the coroutine has yielded" part is currently handled with
aio_context_acquire/aio_context_release (the coroutine cannot run at all
between aio_context_acquire and release).

When they will be gone, some kind of BDRV_POLL_WHILE at the end of
bdrv_detach_aio_context should be enough to ensure that the event loop
is quiescent.

Paolo

>> +
>> +/**
>>   * qio_channel_yield:
>>   * @ioc: the channel object
>>   * @condition: the I/O condition to wait for
>>   *
>> - * Yields execution from the current coroutine until
>> - * the condition indicated by @condition becomes
>> - * available.
>> + * Yields execution from the current coroutine until the condition
>> + * indicated by @condition becomes available.  @condition must
>> + * be either %G_IO_IN or %G_IO_OUT; it cannot contain both.  In
>> + * addition, no two coroutine can be waiting on the same condition
> 
> s/coroutine/coroutines/
>

signature.asc
Description: OpenPGP digital signature