On Tue, Aug 04, 2020 at 09:12:46AM +0200, Paolo Bonzini wrote:
> On 04/08/20 07:28, Stefan Hajnoczi wrote:
> > @@ -425,19 +425,14 @@ void aio_notify(AioContext *ctx)
> >      smp_mb();
> >      if (atomic_read(&ctx->notify_me)) {
> >          event_notifier_set(&ctx->notifier);
> > -        atomic_mb_set(&ctx->notified, true);
> >      }
> > +
> > +    atomic_mb_set(&ctx->notified, true);
> >  }
> 
> This can be an atomic_set since it's already ordered by the smp_mb()
> (actually a smp_wmb() would be enough for ctx->notified, though not for
> ctx->notify_me).
> 
> >  void aio_notify_accept(AioContext *ctx)
> >  {
> > -    if (atomic_xchg(&ctx->notified, false)
> > -#ifdef WIN32
> > -        || true
> > -#endif
> > -    ) {
> > -        event_notifier_test_and_clear(&ctx->notifier);
> > -    }
> > +    atomic_mb_set(&ctx->notified, false);
> >  }
> 
> I am not sure what this should be.
> 
> - If ctx->notified is cleared earlier it's not a problem, there is just
> a possibility for the other side to set it to true again and cause a
> spurious wakeup
> 
> - if it is cleared later, during the dispatch, there is a possibility
> that it we miss a set:
> 
> 	CPU1				CPU2
> 	------------------------------- ------------------------------
> 	read bottom half flags
> 					set BH_SCHEDULED
> 					set ctx->notified
> 	clear ctx->notified (reordered)
> 
> and the next polling loop misses ctx->notified.
> 
> So the requirement is to write ctx->notified before the dispatching
> phase start.  It would be a "store acquire" but it doesn't exist; I
> would replace it with atomic_set() + smp_mb(), plus a comment saying
> that it pairs with the smp_mb() (which actually could be a smp_wmb()) in
> aio_notify().
> 
> In theory the barrier in aio_bh_dequeue is enough, but I don't
> understand memory_order_seqcst atomics well enough to be sure, so I
> prefer an explicit fence.
> 
> Feel free to include part of this description in aio_notify_accept().

Cool, thanks! Will fix in v2.

Stefan