Re: [Qemu-devel] [RFC PATCH] async: Fix aio_notify_accept

[Fam Zheng]

On Fri, 08/03 19:08, Paolo Bonzini wrote:
> On 03/08/2018 17:49, Fam Zheng wrote:
> >  void aio_notify_accept(AioContext *ctx)
> >  {
> > -    if (atomic_xchg(&ctx->notified, false)) {
> > +    /* If ctx->notify_me >= 2, another aio_poll() is waiting which may 
> > need the
> > +     * ctx->notifier event to wake up, so don't already clear it just 
> > because "we" are
> > +     * done iterating. */
> > +    if (atomic_read(&ctx->notify_me) < 2
> > +        && atomic_xchg(&ctx->notified, false)) {
> >          event_notifier_test_and_clear(&ctx->notifier);
> >      }
> >  }
> 
> Ok, it's somewhat reassuring to see from the BZ that the aio_poll in the
> main thread (in bdrv_set_aio_context) is non-blocking, and that it isn't
> about nested aio_poll.
> 
> Then it's not possible to have a busy wait there, because sooner or
> later the bottom halves will be exhausted and aio_wait will return false
> (no progress).
> 
> I'm convinced that the idea in your patch---skipping
> aio_notify_accept---is correct, it's the ctx->notify_me test that I
> cannot understand.  I'm not saying it's wrong, but it's tricky.  So we
> need to improve the comments, the commit message, the way we achieve the
> fix, or all three.
> 
> As to the comments and commit message: the BZ is a very good source of
> information.  The comment on the main thread stealing the aio_notify was
> very clear.

Yes, it was late Friday night and I wanted to send the patch before the long
weekend :)

> 
> As to how to fix it, first of all, we should be clear on the invariants.
>  It would be nice to assert that, if not
> in_aio_context_home_thread(ctx), blocking must be false.  Two concurrent
> blocking aio_polls will steal aio_notify from one another, so
> intuitively that assertion should be true, and using AIO_WAIT_WHILE
> takes care of it.
> 
> Second, if blocking is false, do we need to call aio_notify_accept at
> all?  If not, and if we combine this with the assertion above, only the
> I/O thread will call aio_notify_accept, and the main loop will never
> steal the notification.  So that should fix the bug.

Yes, I think this is a better idea. I'll try it.

Fam