Re: [PATCH 2/3] async: always set ctx->notified in aio_notify()

[Paolo Bonzini <pbonzini@redhat.com>]

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

Paolo