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

[Kevin Wolf <kwolf@redhat.com>]

Am 16.07.2015 um 00:59 hat Paolo Bonzini geschrieben:
> 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. :)

Rather than an extra section in the commit message, it should have been
a comment in the code at least because it's not just the change that is
confusing, but the resulting code is confusing as well.

The reason is that, at least to me, the do..while loop reads almost
like an assertion that count may indeed be 0.

> 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".

Yes, please do that, it should be much clearer.

> > 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.

You changed the AioContext case in this same patch, even if you didn't
quote my comment on that hunk. :-)

Both cases were asserting the return value before.

Kevin