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