Re: [Qemu-devel] [PATCH 05/16] io: make qio_channel_yield aware of AioContexts
On 01/18/2017 10:43 AM, Paolo Bonzini wrote: > 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 s/from an/from one/ > + * 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(). > + * -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 05/16] io: make qio_channel_yield aware of AioContexts
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
Re: [Qemu-devel] [PATCH 05/16] io: make qio_channel_yield aware of AioContexts
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? > + > +/** > * 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: PGP signature
Re: [Qemu-devel] [PATCH 05/16] io: make qio_channel_yield aware of AioContexts
On Mon, Jan 16, 2017 at 12:55:59PM +, Daniel P. Berrange wrote:
> On Mon, Jan 16, 2017 at 07:38:24PM +0800, Fam Zheng wrote:
> > On Fri, 01/13 14:17, Paolo Bonzini wrote:
> > > Support separate coroutines for reading and writing, and place the
> > > read/write handlers on the AioContext that the QIOChannel is registered
> > > with.
> > >
> > > Signed-off-by: Paolo Bonzini
> > > ---
> > > include/io/channel.h | 37 ++
> > > io/channel.c | 86
> > > ++
> > > tests/Makefile.include | 2 +-
> > > 3 files changed, 96 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/include/io/channel.h b/include/io/channel.h
> > > index 665edd7..d7bad94 100644
> > > --- a/include/io/channel.h
> > > +++ b/include/io/channel.h
> > > @@ -23,6 +23,7 @@
> > >
> > > #include "qemu-common.h"
> > > #include "qom/object.h"
> > > +#include "qemu/coroutine.h"
> > > #include "block/aio.h"
> > >
> > > #define TYPE_QIO_CHANNEL "qio-channel"
> > > @@ -59,8 +60,6 @@ typedef gboolean (*QIOChannelFunc)(QIOChannel *ioc,
> > > GIOCondition condition,
> > > gpointer data);
> > >
> > > -typedef struct QIOChannelRestart QIOChannelRestart;
> > > -
> > > /**
> > > * QIOChannel:
> > > *
> > > @@ -84,8 +83,8 @@ struct QIOChannel {
> > > unsigned int features; /* bitmask of QIOChannelFeatures */
> > > char *name;
> > > AioContext *ctx;
> > > -QIOChannelRestart *read_coroutine;
> > > -QIOChannelRestart *write_coroutine;
> > > +Coroutine *read_coroutine;
> > > +Coroutine *write_coroutine;
> > > #ifdef _WIN32
> > > HANDLE event; /* For use with GSource on Win32 */
> > > #endif
> > > @@ -508,13 +507,37 @@ guint qio_channel_add_watch(QIOChannel *ioc,
> > >
> > >
> > > /**
> > > + * 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);
> > > +
> > > +/**
> > > * 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
> > > + * and channel at the same time.
> > > *
> > > * This must only be called from coroutine context
> > > */
> > > diff --git a/io/channel.c b/io/channel.c
> > > index ce470d7..1e043bf 100644
> > > --- a/io/channel.c
> > > +++ b/io/channel.c
> > > @@ -21,7 +21,7 @@
> > > #include "qemu/osdep.h"
> > > #include "io/channel.h"
> > > #include "qapi/error.h"
> > > -#include "qemu/coroutine.h"
> > > +#include "qemu/main-loop.h"
> > >
> > > bool qio_channel_has_feature(QIOChannel *ioc,
> > > QIOChannelFeature feature)
> > > @@ -238,36 +238,80 @@ off_t qio_channel_io_seek(QIOChannel *ioc,
> > > }
> > >
> > >
> > > -typedef struct QIOChannelYieldData QIOChannelYieldData;
> > > -struct QIOChannelYieldData {
> > > -QIOChannel *ioc;
> > > -Coroutine *co;
> > > -};
> > > +static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc);
> > > +
> > > +static void qio_channel_restart_read(void *opaque)
> > > +{
> > > +QIOChannel *ioc = opaque;
> > > +Coroutine *co = ioc->read_coroutine;
> > >
> > > +ioc->read_coroutine = NULL;
> > > +qio_channel_set_aio_fd_handlers(ioc);
> > > +aio_co_wake(co);
> > > +}
> > >
> > > -static gboolean qio_channel_yield_enter(QIOChannel *ioc,
> > > -GIOCondition condition,
> > > -gpointer opaque)
> > > +static void qio_channel_restart_write(void *opaque)
> > > {
> > > -QIOChannelYieldData *data = opaque;
> > > -qemu_coroutine_enter(data->co);
> > > -return FALSE;
> > > +QIOChannel *ioc = opaque;
> > > +Coroutine *co = ioc->write_coroutine;
> > > +
> > > +ioc->write_coroutine = NULL;
> > > +q
Re: [Qemu-devel] [PATCH 05/16] io: make qio_channel_yield aware of AioContexts
On 16/01/2017 13:58, Daniel P. Berrange wrote: >> + * 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. >> + */ > Can you note that it is explicitly permitted to call this while > inside a qio_channel_yield(). Yes: * 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(). Paolo
Re: [Qemu-devel] [PATCH 05/16] io: make qio_channel_yield aware of AioContexts
On Mon, Jan 16, 2017 at 08:47:28PM +0800, Fam Zheng wrote:
> On Mon, 01/16 13:24, Paolo Bonzini wrote:
> >
> >
> > On 16/01/2017 12:38, Fam Zheng wrote:
> > >> +void qio_channel_detach_aio_context(QIOChannel *ioc)
> > >> +{
> > >> +ioc->read_coroutine = NULL;
> > >> +ioc->write_coroutine = NULL;
> > >> +qio_channel_set_aio_fd_handlers(ioc);
> > >> +ioc->ctx = NULL;
> > >
> > > Why is qio_channel_set_aio_fd_handler not needed here?
> >
> > Because there are no read_coroutine and write_coroutine anymore. The
> > caller needs to schedule them on the right AioContext after calling
> > qio_channel_set_aio_context. See nbd_client_attach_aio_context in the
> > next patch for an example.
> >
> > >> -tests/test-char$(EXESUF): tests/test-char.o qemu-char.o qemu-timer.o
> > >> $(test-util-obj-y) $(qtest-obj-y) $(test-io-obj-y)
> > >> +tests/test-char$(EXESUF): tests/test-char.o qemu-char.o qemu-timer.o
> > >> $(test-util-obj-y) $(qtest-obj-y) $(test-io-obj-y) $(test-block-obj-y)
> > >
> > > I guess this is a hint for moving coroutine code into a lower level
> > > library like
> > > util.
> >
> > Coroutine, or AioContext? The reason for this is that io/ now uses
> > aio_co_wake.
>
> Or both. It just feels a bit odd to see *char* depend on *block*, maybe
> there're
> more such dependencies to come even outside tests/?
>
> Not necessarily for this series, of course.
On the contrary, I think it should be in this series - we shouldn't
introduce a dependancy on the block layer from io layer.
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o-http://search.cpan.org/~danberr/ :|
Re: [Qemu-devel] [PATCH 05/16] io: make qio_channel_yield aware of AioContexts
On Fri, Jan 13, 2017 at 02:17:20PM +0100, Paolo Bonzini wrote:
> Support separate coroutines for reading and writing, and place the
> read/write handlers on the AioContext that the QIOChannel is registered
> with.
>
> Signed-off-by: Paolo Bonzini
> ---
> include/io/channel.h | 37 ++
> io/channel.c | 86
> ++
> tests/Makefile.include | 2 +-
> 3 files changed, 96 insertions(+), 29 deletions(-)
>
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 665edd7..d7bad94 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -23,6 +23,7 @@
>
> #include "qemu-common.h"
> #include "qom/object.h"
> +#include "qemu/coroutine.h"
> #include "block/aio.h"
>
> #define TYPE_QIO_CHANNEL "qio-channel"
> @@ -59,8 +60,6 @@ typedef gboolean (*QIOChannelFunc)(QIOChannel *ioc,
> GIOCondition condition,
> gpointer data);
>
> -typedef struct QIOChannelRestart QIOChannelRestart;
> -
> /**
> * QIOChannel:
> *
> @@ -84,8 +83,8 @@ struct QIOChannel {
> unsigned int features; /* bitmask of QIOChannelFeatures */
> char *name;
> AioContext *ctx;
> -QIOChannelRestart *read_coroutine;
> -QIOChannelRestart *write_coroutine;
> +Coroutine *read_coroutine;
> +Coroutine *write_coroutine;
Need to squash in part of previous patch here.
> #ifdef _WIN32
> HANDLE event; /* For use with GSource on Win32 */
> #endif
> @@ -508,13 +507,37 @@ guint qio_channel_add_watch(QIOChannel *ioc,
>
>
> /**
> + * 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.
> + */
Can you note that it is explicitly permitted to call this while
inside a qio_channel_yield().
> +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);
> diff --git a/io/channel.c b/io/channel.c
> index ce470d7..1e043bf 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -21,7 +21,7 @@
> #include "qemu/osdep.h"
> #include "io/channel.h"
> #include "qapi/error.h"
> -#include "qemu/coroutine.h"
> +#include "qemu/main-loop.h"
>
> bool qio_channel_has_feature(QIOChannel *ioc,
> QIOChannelFeature feature)
> @@ -238,36 +238,80 @@ off_t qio_channel_io_seek(QIOChannel *ioc,
> }
>
>
> -typedef struct QIOChannelYieldData QIOChannelYieldData;
> -struct QIOChannelYieldData {
> -QIOChannel *ioc;
> -Coroutine *co;
> -};
> +static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc);
> +
> +static void qio_channel_restart_read(void *opaque)
> +{
> +QIOChannel *ioc = opaque;
> +Coroutine *co = ioc->read_coroutine;
>
> +ioc->read_coroutine = NULL;
> +qio_channel_set_aio_fd_handlers(ioc);
> +aio_co_wake(co);
> +}
>
> -static gboolean qio_channel_yield_enter(QIOChannel *ioc,
> -GIOCondition condition,
> -gpointer opaque)
> +static void qio_channel_restart_write(void *opaque)
> {
> -QIOChannelYieldData *data = opaque;
> -qemu_coroutine_enter(data->co);
> -return FALSE;
> +QIOChannel *ioc = opaque;
> +Coroutine *co = ioc->write_coroutine;
> +
> +ioc->write_coroutine = NULL;
> +qio_channel_set_aio_fd_handlers(ioc);
> +aio_co_wake(co);
> }
>
> +static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc)
> +{
> +IOHandler *rd_handler = NULL, *wr_handler = NULL;
> +AioContext *ctx;
> +
> +if (ioc->read_coroutine) {
> + rd_handler = qio_channel_restart_read;
> +}
> +if (ioc->write_coroutine) {
> + rd_handler = qio_channel_restart_write;
> +}
Tab damage.
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o-http://search.cpan.org/~danberr/ :|
Re: [Qemu-devel] [PATCH 05/16] io: make qio_channel_yield aware of AioContexts
On Mon, Jan 16, 2017 at 07:38:24PM +0800, Fam Zheng wrote:
> On Fri, 01/13 14:17, Paolo Bonzini wrote:
> > Support separate coroutines for reading and writing, and place the
> > read/write handlers on the AioContext that the QIOChannel is registered
> > with.
> >
> > Signed-off-by: Paolo Bonzini
> > ---
> > include/io/channel.h | 37 ++
> > io/channel.c | 86
> > ++
> > tests/Makefile.include | 2 +-
> > 3 files changed, 96 insertions(+), 29 deletions(-)
> >
> > diff --git a/include/io/channel.h b/include/io/channel.h
> > index 665edd7..d7bad94 100644
> > --- a/include/io/channel.h
> > +++ b/include/io/channel.h
> > @@ -23,6 +23,7 @@
> >
> > #include "qemu-common.h"
> > #include "qom/object.h"
> > +#include "qemu/coroutine.h"
> > #include "block/aio.h"
> >
> > #define TYPE_QIO_CHANNEL "qio-channel"
> > @@ -59,8 +60,6 @@ typedef gboolean (*QIOChannelFunc)(QIOChannel *ioc,
> > GIOCondition condition,
> > gpointer data);
> >
> > -typedef struct QIOChannelRestart QIOChannelRestart;
> > -
> > /**
> > * QIOChannel:
> > *
> > @@ -84,8 +83,8 @@ struct QIOChannel {
> > unsigned int features; /* bitmask of QIOChannelFeatures */
> > char *name;
> > AioContext *ctx;
> > -QIOChannelRestart *read_coroutine;
> > -QIOChannelRestart *write_coroutine;
> > +Coroutine *read_coroutine;
> > +Coroutine *write_coroutine;
> > #ifdef _WIN32
> > HANDLE event; /* For use with GSource on Win32 */
> > #endif
> > @@ -508,13 +507,37 @@ guint qio_channel_add_watch(QIOChannel *ioc,
> >
> >
> > /**
> > + * 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);
> > +
> > +/**
> > * 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
> > + * and channel at the same time.
> > *
> > * This must only be called from coroutine context
> > */
> > diff --git a/io/channel.c b/io/channel.c
> > index ce470d7..1e043bf 100644
> > --- a/io/channel.c
> > +++ b/io/channel.c
> > @@ -21,7 +21,7 @@
> > #include "qemu/osdep.h"
> > #include "io/channel.h"
> > #include "qapi/error.h"
> > -#include "qemu/coroutine.h"
> > +#include "qemu/main-loop.h"
> >
> > bool qio_channel_has_feature(QIOChannel *ioc,
> > QIOChannelFeature feature)
> > @@ -238,36 +238,80 @@ off_t qio_channel_io_seek(QIOChannel *ioc,
> > }
> >
> >
> > -typedef struct QIOChannelYieldData QIOChannelYieldData;
> > -struct QIOChannelYieldData {
> > -QIOChannel *ioc;
> > -Coroutine *co;
> > -};
> > +static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc);
> > +
> > +static void qio_channel_restart_read(void *opaque)
> > +{
> > +QIOChannel *ioc = opaque;
> > +Coroutine *co = ioc->read_coroutine;
> >
> > +ioc->read_coroutine = NULL;
> > +qio_channel_set_aio_fd_handlers(ioc);
> > +aio_co_wake(co);
> > +}
> >
> > -static gboolean qio_channel_yield_enter(QIOChannel *ioc,
> > -GIOCondition condition,
> > -gpointer opaque)
> > +static void qio_channel_restart_write(void *opaque)
> > {
> > -QIOChannelYieldData *data = opaque;
> > -qemu_coroutine_enter(data->co);
> > -return FALSE;
> > +QIOChannel *ioc = opaque;
> > +Coroutine *co = ioc->write_coroutine;
> > +
> > +ioc->write_coroutine = NULL;
> > +qio_channel_set_aio_fd_handlers(ioc);
> > +aio_co_wake(co);
> > }
> >
> > +static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc)
> > +{
> > +IOHandler *rd_handler = NULL, *wr_handler = NULL;
> > +AioContext *ctx;
> > +
> > +if (ioc->read_coroutine) {
> > + rd_handler = qio_channel_restart_read;
>
> s/\t/
Re: [Qemu-devel] [PATCH 05/16] io: make qio_channel_yield aware of AioContexts
On Mon, 01/16 13:24, Paolo Bonzini wrote:
>
>
> On 16/01/2017 12:38, Fam Zheng wrote:
> >> +void qio_channel_detach_aio_context(QIOChannel *ioc)
> >> +{
> >> +ioc->read_coroutine = NULL;
> >> +ioc->write_coroutine = NULL;
> >> +qio_channel_set_aio_fd_handlers(ioc);
> >> +ioc->ctx = NULL;
> >
> > Why is qio_channel_set_aio_fd_handler not needed here?
>
> Because there are no read_coroutine and write_coroutine anymore. The
> caller needs to schedule them on the right AioContext after calling
> qio_channel_set_aio_context. See nbd_client_attach_aio_context in the
> next patch for an example.
>
> >> -tests/test-char$(EXESUF): tests/test-char.o qemu-char.o qemu-timer.o
> >> $(test-util-obj-y) $(qtest-obj-y) $(test-io-obj-y)
> >> +tests/test-char$(EXESUF): tests/test-char.o qemu-char.o qemu-timer.o
> >> $(test-util-obj-y) $(qtest-obj-y) $(test-io-obj-y) $(test-block-obj-y)
> >
> > I guess this is a hint for moving coroutine code into a lower level library
> > like
> > util.
>
> Coroutine, or AioContext? The reason for this is that io/ now uses
> aio_co_wake.
Or both. It just feels a bit odd to see *char* depend on *block*, maybe there're
more such dependencies to come even outside tests/?
Not necessarily for this series, of course.
Fam
Re: [Qemu-devel] [PATCH 05/16] io: make qio_channel_yield aware of AioContexts
On 16/01/2017 12:38, Fam Zheng wrote:
>> +void qio_channel_detach_aio_context(QIOChannel *ioc)
>> +{
>> +ioc->read_coroutine = NULL;
>> +ioc->write_coroutine = NULL;
>> +qio_channel_set_aio_fd_handlers(ioc);
>> +ioc->ctx = NULL;
>
> Why is qio_channel_set_aio_fd_handler not needed here?
Because there are no read_coroutine and write_coroutine anymore. The
caller needs to schedule them on the right AioContext after calling
qio_channel_set_aio_context. See nbd_client_attach_aio_context in the
next patch for an example.
>> -tests/test-char$(EXESUF): tests/test-char.o qemu-char.o qemu-timer.o
>> $(test-util-obj-y) $(qtest-obj-y) $(test-io-obj-y)
>> +tests/test-char$(EXESUF): tests/test-char.o qemu-char.o qemu-timer.o
>> $(test-util-obj-y) $(qtest-obj-y) $(test-io-obj-y) $(test-block-obj-y)
>
> I guess this is a hint for moving coroutine code into a lower level library
> like
> util.
Coroutine, or AioContext? The reason for this is that io/ now uses
aio_co_wake.
Paolo
Re: [Qemu-devel] [PATCH 05/16] io: make qio_channel_yield aware of AioContexts
On Fri, 01/13 14:17, Paolo Bonzini wrote:
> Support separate coroutines for reading and writing, and place the
> read/write handlers on the AioContext that the QIOChannel is registered
> with.
>
> Signed-off-by: Paolo Bonzini
> ---
> include/io/channel.h | 37 ++
> io/channel.c | 86
> ++
> tests/Makefile.include | 2 +-
> 3 files changed, 96 insertions(+), 29 deletions(-)
>
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 665edd7..d7bad94 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -23,6 +23,7 @@
>
> #include "qemu-common.h"
> #include "qom/object.h"
> +#include "qemu/coroutine.h"
> #include "block/aio.h"
>
> #define TYPE_QIO_CHANNEL "qio-channel"
> @@ -59,8 +60,6 @@ typedef gboolean (*QIOChannelFunc)(QIOChannel *ioc,
> GIOCondition condition,
> gpointer data);
>
> -typedef struct QIOChannelRestart QIOChannelRestart;
> -
> /**
> * QIOChannel:
> *
> @@ -84,8 +83,8 @@ struct QIOChannel {
> unsigned int features; /* bitmask of QIOChannelFeatures */
> char *name;
> AioContext *ctx;
> -QIOChannelRestart *read_coroutine;
> -QIOChannelRestart *write_coroutine;
> +Coroutine *read_coroutine;
> +Coroutine *write_coroutine;
> #ifdef _WIN32
> HANDLE event; /* For use with GSource on Win32 */
> #endif
> @@ -508,13 +507,37 @@ guint qio_channel_add_watch(QIOChannel *ioc,
>
>
> /**
> + * 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);
> +
> +/**
> * 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
> + * and channel at the same time.
> *
> * This must only be called from coroutine context
> */
> diff --git a/io/channel.c b/io/channel.c
> index ce470d7..1e043bf 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -21,7 +21,7 @@
> #include "qemu/osdep.h"
> #include "io/channel.h"
> #include "qapi/error.h"
> -#include "qemu/coroutine.h"
> +#include "qemu/main-loop.h"
>
> bool qio_channel_has_feature(QIOChannel *ioc,
> QIOChannelFeature feature)
> @@ -238,36 +238,80 @@ off_t qio_channel_io_seek(QIOChannel *ioc,
> }
>
>
> -typedef struct QIOChannelYieldData QIOChannelYieldData;
> -struct QIOChannelYieldData {
> -QIOChannel *ioc;
> -Coroutine *co;
> -};
> +static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc);
> +
> +static void qio_channel_restart_read(void *opaque)
> +{
> +QIOChannel *ioc = opaque;
> +Coroutine *co = ioc->read_coroutine;
>
> +ioc->read_coroutine = NULL;
> +qio_channel_set_aio_fd_handlers(ioc);
> +aio_co_wake(co);
> +}
>
> -static gboolean qio_channel_yield_enter(QIOChannel *ioc,
> -GIOCondition condition,
> -gpointer opaque)
> +static void qio_channel_restart_write(void *opaque)
> {
> -QIOChannelYieldData *data = opaque;
> -qemu_coroutine_enter(data->co);
> -return FALSE;
> +QIOChannel *ioc = opaque;
> +Coroutine *co = ioc->write_coroutine;
> +
> +ioc->write_coroutine = NULL;
> +qio_channel_set_aio_fd_handlers(ioc);
> +aio_co_wake(co);
> }
>
> +static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc)
> +{
> +IOHandler *rd_handler = NULL, *wr_handler = NULL;
> +AioContext *ctx;
> +
> +if (ioc->read_coroutine) {
> + rd_handler = qio_channel_restart_read;
s/\t//
> +}
> +if (ioc->write_coroutine) {
> + rd_handler = qio_channel_restart_write;
s/\t//
> +}
> +
> +ctx = ioc->ctx ? ioc->ctx : iohandler_get_aio_context();
> +qio_channel_set_aio_fd_handler(ioc, ctx, rd_handler, wr_handler, ioc);
> +}
> +
> +void qio_channel_set_aio_context(QIOChannel *ioc,
> +
[Qemu-devel] [PATCH 05/16] io: make qio_channel_yield aware of AioContexts
Support separate coroutines for reading and writing, and place the
read/write handlers on the AioContext that the QIOChannel is registered
with.
Signed-off-by: Paolo Bonzini
---
include/io/channel.h | 37 ++
io/channel.c | 86 ++
tests/Makefile.include | 2 +-
3 files changed, 96 insertions(+), 29 deletions(-)
diff --git a/include/io/channel.h b/include/io/channel.h
index 665edd7..d7bad94 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -23,6 +23,7 @@
#include "qemu-common.h"
#include "qom/object.h"
+#include "qemu/coroutine.h"
#include "block/aio.h"
#define TYPE_QIO_CHANNEL "qio-channel"
@@ -59,8 +60,6 @@ typedef gboolean (*QIOChannelFunc)(QIOChannel *ioc,
GIOCondition condition,
gpointer data);
-typedef struct QIOChannelRestart QIOChannelRestart;
-
/**
* QIOChannel:
*
@@ -84,8 +83,8 @@ struct QIOChannel {
unsigned int features; /* bitmask of QIOChannelFeatures */
char *name;
AioContext *ctx;
-QIOChannelRestart *read_coroutine;
-QIOChannelRestart *write_coroutine;
+Coroutine *read_coroutine;
+Coroutine *write_coroutine;
#ifdef _WIN32
HANDLE event; /* For use with GSource on Win32 */
#endif
@@ -508,13 +507,37 @@ guint qio_channel_add_watch(QIOChannel *ioc,
/**
+ * 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);
+
+/**
* 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
+ * and channel at the same time.
*
* This must only be called from coroutine context
*/
diff --git a/io/channel.c b/io/channel.c
index ce470d7..1e043bf 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -21,7 +21,7 @@
#include "qemu/osdep.h"
#include "io/channel.h"
#include "qapi/error.h"
-#include "qemu/coroutine.h"
+#include "qemu/main-loop.h"
bool qio_channel_has_feature(QIOChannel *ioc,
QIOChannelFeature feature)
@@ -238,36 +238,80 @@ off_t qio_channel_io_seek(QIOChannel *ioc,
}
-typedef struct QIOChannelYieldData QIOChannelYieldData;
-struct QIOChannelYieldData {
-QIOChannel *ioc;
-Coroutine *co;
-};
+static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc);
+
+static void qio_channel_restart_read(void *opaque)
+{
+QIOChannel *ioc = opaque;
+Coroutine *co = ioc->read_coroutine;
+ioc->read_coroutine = NULL;
+qio_channel_set_aio_fd_handlers(ioc);
+aio_co_wake(co);
+}
-static gboolean qio_channel_yield_enter(QIOChannel *ioc,
-GIOCondition condition,
-gpointer opaque)
+static void qio_channel_restart_write(void *opaque)
{
-QIOChannelYieldData *data = opaque;
-qemu_coroutine_enter(data->co);
-return FALSE;
+QIOChannel *ioc = opaque;
+Coroutine *co = ioc->write_coroutine;
+
+ioc->write_coroutine = NULL;
+qio_channel_set_aio_fd_handlers(ioc);
+aio_co_wake(co);
}
+static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc)
+{
+IOHandler *rd_handler = NULL, *wr_handler = NULL;
+AioContext *ctx;
+
+if (ioc->read_coroutine) {
+ rd_handler = qio_channel_restart_read;
+}
+if (ioc->write_coroutine) {
+ rd_handler = qio_channel_restart_write;
+}
+
+ctx = ioc->ctx ? ioc->ctx : iohandler_get_aio_context();
+qio_channel_set_aio_fd_handler(ioc, ctx, rd_handler, wr_handler, ioc);
+}
+
+void qio_channel_set_aio_context(QIOChannel *ioc,
+ AioContext *ctx)
+{
+AioContext *old_ctx;
+if (ioc->ctx == ctx) {
+return;
+}
+
+old_ctx = ioc->ctx ? ioc->ctx : iohandler_get_aio_context();
+qio_channel_set_aio_fd_handler(ioc, old_ctx, NULL, NULL, NULL);
+ioc->ctx = ctx;
+qio_channel_set_aio_fd_handlers(ioc);
+}
+
+void qio_channel_detach_aio_context(QIOChannel *ioc
