Re: [Qemu-devel] [PATCH 2/3] io: make qio_channel_yield aware of AioContexts

[Daniel P. Berrange]

On Wed, Jan 04, 2017 at 04:26:24PM -0500, Paolo Bonzini wrote:
> > > @@ -84,8 +83,8 @@ struct QIOChannel {
> > > +static void qio_channel_set_fd_handlers(QIOChannel *ioc)
> > > +{
> > > +    IOHandler *rd_handler = NULL, *wr_handler = NULL;
> > > +
> > > +    if (ioc->read_coroutine) {
> > > + rd_handler = qio_channel_restart_read;
> > > +    }
> > > +    if (ioc->write_coroutine) {
> > > + rd_handler = qio_channel_restart_write;
> > > +    }
> > > +
> > > +    qio_channel_set_fd_handler(ioc,
> > > +                               ioc->ctx ? ioc->ctx :
> > > iohandler_get_aio_context(),
> > > +                               rd_handler, wr_handler, ioc);
> > > +}
> > 
> > ioc->read_coroutine & ioc->write_coroutine can only be non-NULL during
> > a qio_channel_yield() caller. So it seems that calling
> > qio_channel_set_fd_handlers() from the qio_channel_set_aio_context()
> > method in the previous patch is not required, as those two callback
> > pointers will always be NULL.
> 
> Not necessarily.  You can have one coroutine calling qio_channel_yield(),
> and then the non-coroutine code can call qio_channel_set_aio_context()
> before the coroutine reenters.
> 
> This actually happens in the next patch.  Where the NBD socket is quiescent
> and no response is in flight, such as during a bdrv_drain_begin/end()
> section, the "coroutine that receives NBD headers" has yielded.  This
> is also the time when set_aio_context can be called.

Ok, that's a little surprising :-) Can you document on qio_channel_yield
that its permitted to yield and set an aio context while waiting.

> > > +    if (condition == G_IO_IN) {
> > > +        ioc->read_coroutine = qemu_coroutine_self();
> > > +    } else if (condition == G_IO_OUT) {
> > > +        ioc->write_coroutine = qemu_coroutine_self();
> > > +    } else {
> > > +        abort();
> > > +    }
> > 
> > Do we really need this to be an either/or/abort ? It looks like
> > the qio_channel_set_fd_handlers() method is happy top have both
> > read_coroutine & write_coroutine set.
> 
> The idea is that this would be called by a coroutine after a
> recv or send that returns EAGAIN (with G_IO_IN for recv and
> G_IO_OUT for send).  If not exclusive, you'd have to check
> for ioc->read_coroutine == ioc->write_coroutine in the handler.
> Not a big deal, I can do it, but it adds an edge case and I
> didn't see a use for it.

Yep, it feels unlikely. Tht said, it looks similar to the case
where you have two coroutines using the same channel, and one
does a yield(G_IO_IN) and the other does a yield(G_IO_OUT) while
the first is still waiting, which feels like a more plausible
scenario that could actually happen. So perhaps we do need to
consider it

> > If it does need to be exclusive though, can you update the API
> > docs for this method to mention that.
> 
> Sure.
> 
> Thanks for the speedy review!
> 
> Paolo

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/ :|