Re: [Qemu-devel] [PATCH] AioContext: fix broken ctx->dispatching optimization

[Paolo Bonzini]

On 15/07/2015 22:14, Kevin Wolf wrote:
> > index 233d8f5..ae7c6cf 100644
> > --- a/aio-win32.c
> > +++ b/aio-win32.c
> > @@ -318,11 +313,11 @@ bool aio_poll(AioContext *ctx, bool blocking)
> >      first = true;
> >  
> >      /* wait until next event */
> > -    while (count > 0) {
> > +    do {
> >          HANDLE event;
> >          int ret;
> >  
> > -        timeout = blocking
> > +        timeout = blocking && !have_select_revents
> >              ? qemu_timeout_ns_to_ms(aio_compute_timeout(ctx)) : 0;
> >          if (timeout) {
> >              aio_context_release(ctx);
>
> Here I'm not sure why you switched to a do..while loop? It's not obvious
> to me how the change from aio_set_dispatching() to ctx->notify_me is
> related to this.

I sort of expected a reviewer to ask me about this change.  I would have
explained this in the commit message, but it was already long enough. :)

The count on entry is never zero, because the ctx->notifier is always
registered.

I changed the while to do...while so that it's obvious that
ctx->notify_me is always decremented.

In retrospect I could have added simply

    /* ctx->notifier is always registered.  */
    assert(count > 0);

above the "do".

> With this information, I understand that what has changed is that the
> return value of g_main_context_iteration() changes from true before this
> patch (had the aio_notify() from aio_set_fd_handler() pending) to false
> after the patch (aio_notify() doesn't inject an event any more).
> 
> This should mean that like above we can assert that the first iteration
> returns false, i.e. reverse the assertion (and indeed, with this
> change the test still passes for me).

I was a bit undecided about this.  In the end I decided that the calls
to aio_poll/g_main_context_iteration were just to put the AioContext in
a known state, and the assertions on the return value of g_assert were
not really important.  For this reason, the while loop seemed to express
the intentions best, and I made it consistent between the AioContext and
GSource cases.

Paolo