| Message ID | 20200804052804.1165291-3-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | aio-posix: keep aio_notify_me disabled during polling | expand |
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
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
diff --git a/util/async.c b/util/async.c index d9f987e133..3e68b5b757 100644 --- a/util/async.c +++ b/util/async.c @@ -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); } 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); } static void aio_timerlist_notify(void *opaque, QEMUClockType type) @@ -445,8 +440,11 @@ static void aio_timerlist_notify(void *opaque, QEMUClockType type) aio_notify(opaque); } -static void aio_context_notifier_dummy_cb(EventNotifier *e) +static void aio_context_notifier_cb(EventNotifier *e) { + AioContext *ctx = container_of(e, AioContext, notifier); + + event_notifier_test_and_clear(&ctx->notifier); } /* Returns true if aio_notify() was called (e.g. a BH was scheduled) */ @@ -508,7 +506,7 @@ AioContext *aio_context_new(Error **errp) aio_set_event_notifier(ctx, &ctx->notifier, false, - aio_context_notifier_dummy_cb, + aio_context_notifier_cb, aio_context_notifier_poll); #ifdef CONFIG_LINUX_AIO ctx->linux_aio = NULL;
aio_notify() does not set ctx->notified when called with ctx->aio_notify_me disabled. In order to poll ctx->notified it is therefore necessary to enable ctx->aio_notify_me. This is suboptimal since expensive event_notifier_set(&ctx->notifier) and event_notifier_test_and_clear(&ctx->notifier) calls required when ctx->aio_notify_me is enabled. Change aio_notify() me so that aio->notified is always set, regardless of ctx->aio_notify_me. This will allow polling to work cheaply with ctx->aio_notify_me disabled. Move the event_notifier_test_and_clear() to the fd handler function (which is now no longer an empty function so "dummy" has been dropped from its name). The next patch takes advantage of this by optimizing polling in util/aio-posix.c. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- util/async.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)