[PATCH 0/3] aio-posix: keep aio_notify_me disabled during polling

[PATCH 0/3] aio-posix: keep aio_notify_me disabled during polling

[Stefan Hajnoczi]

This patch series eliminates ctx->notifier EventNotifier activity when
aio_poll() is in polling mode. There is no need to use the EventNotifier since
a polling handler can detect that aio_notify() has been called by monitoring a
field in memory instead.

Optimizing out the EventNotifier calls improves null-co random read 4KB
iodepth=1 IOPS by 18%.

aio_compute_timeout() is now called twice if aio_poll() needs to block, which
means an extra qemu_clock_get_ns() call is made when there is an active timer.
An alternative would be to set timeout = 0 if ctx->notified is true before
blocking, but going around the event loop again could slow things down more.

I have not modified docs/spin/aio_notify*.promela because I'm not familiar with
the SPIN model checker.

Stefan Hajnoczi (3):
  async: rename event_notifier_dummy_cb/poll()
  async: always set ctx->notified in aio_notify()
  aio-posix: keep aio_notify_me disabled during polling

 util/aio-posix.c | 56 ++++++++++++++++++++++++------------------------
 util/async.c     | 22 +++++++++----------
 2 files changed, 38 insertions(+), 40 deletions(-)

-- 
2.26.2

[PATCH 1/3] async: rename event_notifier_dummy_cb/poll()

[Stefan Hajnoczi]

The event_notifier_*() prefix can be confused with the EventNotifier
APIs that are also called event_notifier_*().

Rename the functions to aio_context_notifier_*() to make it clear that
they relate to the AioContext::notifier field.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/async.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/util/async.c b/util/async.c
index 1319eee3bc..d9f987e133 100644
--- a/util/async.c
+++ b/util/async.c
@@ -445,12 +445,12 @@ static void aio_timerlist_notify(void *opaque, QEMUClockType type)
     aio_notify(opaque);
 }
 
-static void event_notifier_dummy_cb(EventNotifier *e)
+static void aio_context_notifier_dummy_cb(EventNotifier *e)
 {
 }
 
 /* Returns true if aio_notify() was called (e.g. a BH was scheduled) */
-static bool event_notifier_poll(void *opaque)
+static bool aio_context_notifier_poll(void *opaque)
 {
     EventNotifier *e = opaque;
     AioContext *ctx = container_of(e, AioContext, notifier);
@@ -508,8 +508,8 @@ AioContext *aio_context_new(Error **errp)
 
     aio_set_event_notifier(ctx, &ctx->notifier,
                            false,
-                           event_notifier_dummy_cb,
-                           event_notifier_poll);
+                           aio_context_notifier_dummy_cb,
+                           aio_context_notifier_poll);
 #ifdef CONFIG_LINUX_AIO
     ctx->linux_aio = NULL;
 #endif
-- 
2.26.2

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

[Stefan Hajnoczi]

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

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;
-- 
2.26.2

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

[Paolo Bonzini]

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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2058 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/3] aio-posix: keep aio_notify_me disabled during polling

[Stefan Hajnoczi]

Polling only monitors the ctx->notified field and does not need the
ctx->notifier EventNotifier to be signalled. Keep ctx->aio_notify_me
disabled while polling to avoid unnecessary EventNotifier syscalls.

This optimization improves virtio-blk 4KB random read performance by
18%. The following results are with an IOThread and the null-co block
driver:

Test         IOPS   Error
Before  244518.62 ± 1.20%
After   290706.11 ± 0.44%

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/aio-posix.c | 56 ++++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/util/aio-posix.c b/util/aio-posix.c
index 1b2a3af65b..078ec15890 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -464,9 +464,6 @@ static bool remove_idle_poll_handlers(AioContext *ctx, int64_t now)
  *
  * Polls for a given time.
  *
- * Note that ctx->notify_me must be non-zero so this function can detect
- * aio_notify().
- *
  * Note that the caller must have incremented ctx->list_lock.
  *
  * Returns: true if progress was made, false otherwise
@@ -476,7 +473,6 @@ static bool run_poll_handlers(AioContext *ctx, int64_t max_ns, int64_t *timeout)
     bool progress;
     int64_t start_time, elapsed_time;
 
-    assert(ctx->notify_me);
     assert(qemu_lockcnt_count(&ctx->list_lock) > 0);
 
     trace_run_poll_handlers_begin(ctx, max_ns, *timeout);
@@ -520,8 +516,6 @@ static bool run_poll_handlers(AioContext *ctx, int64_t max_ns, int64_t *timeout)
  * @timeout: timeout for blocking wait, computed by the caller and updated if
  *    polling succeeds.
  *
- * ctx->notify_me must be non-zero so this function can detect aio_notify().
- *
  * Note that the caller must have incremented ctx->list_lock.
  *
  * Returns: true if progress was made, false otherwise
@@ -566,23 +560,6 @@ bool aio_poll(AioContext *ctx, bool blocking)
      */
     assert(in_aio_context_home_thread(ctx));
 
-    /* aio_notify can avoid the expensive event_notifier_set if
-     * everything (file descriptors, bottom halves, timers) will
-     * be re-evaluated before the next blocking poll().  This is
-     * already true when aio_poll is called with blocking == false;
-     * if blocking == true, it is only true after poll() returns,
-     * so disable the optimization now.
-     */
-    if (blocking) {
-        atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
-        /*
-         * Write ctx->notify_me before computing the timeout
-         * (reading bottom half flags, etc.).  Pairs with
-         * smp_mb in aio_notify().
-         */
-        smp_mb();
-    }
-
     qemu_lockcnt_inc(&ctx->list_lock);
 
     if (ctx->poll_max_ns) {
@@ -597,15 +574,38 @@ bool aio_poll(AioContext *ctx, bool blocking)
      * system call---a single round of run_poll_handlers_once suffices.
      */
     if (timeout || ctx->fdmon_ops->need_wait(ctx)) {
+        /*
+         * aio_notify can avoid the expensive event_notifier_set if
+         * everything (file descriptors, bottom halves, timers) will
+         * be re-evaluated before the next blocking poll().  This is
+         * already true when aio_poll is called with blocking == false;
+         * if blocking == true, it is only true after poll() returns,
+         * so disable the optimization now.
+         */
+        if (timeout) {
+            atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
+            /*
+             * Write ctx->notify_me before computing the timeout
+             * (reading bottom half flags, etc.).  Pairs with
+             * smp_mb in aio_notify().
+             */
+            smp_mb();
+
+            /* Check again in case a shorter timer was added */
+            timeout = qemu_soonest_timeout(timeout, aio_compute_timeout(ctx));
+        }
+
         ret = ctx->fdmon_ops->wait(ctx, &ready_list, timeout);
-    }
 
-    if (blocking) {
-        /* Finish the poll before clearing the flag.  */
-        atomic_store_release(&ctx->notify_me, atomic_read(&ctx->notify_me) - 2);
-        aio_notify_accept(ctx);
+        if (timeout) {
+            /* Finish the poll before clearing the flag.  */
+            atomic_store_release(&ctx->notify_me,
+                                 atomic_read(&ctx->notify_me) - 2);
+        }
     }
 
+    aio_notify_accept(ctx);
+
     /* Adjust polling time */
     if (ctx->poll_max_ns) {
         int64_t block_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start;
-- 
2.26.2

Re: [PATCH 3/3] aio-posix: keep aio_notify_me disabled during polling

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2583 bytes --]

On Tue, Aug 04, 2020 at 06:28:04AM +0100, Stefan Hajnoczi wrote:
> @@ -597,15 +574,38 @@ bool aio_poll(AioContext *ctx, bool blocking)
>       * system call---a single round of run_poll_handlers_once suffices.
>       */
>      if (timeout || ctx->fdmon_ops->need_wait(ctx)) {
> +        /*
> +         * aio_notify can avoid the expensive event_notifier_set if
> +         * everything (file descriptors, bottom halves, timers) will
> +         * be re-evaluated before the next blocking poll().  This is
> +         * already true when aio_poll is called with blocking == false;
> +         * if blocking == true, it is only true after poll() returns,
> +         * so disable the optimization now.
> +         */
> +        if (timeout) {
> +            atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
> +            /*
> +             * Write ctx->notify_me before computing the timeout
> +             * (reading bottom half flags, etc.).  Pairs with
> +             * smp_mb in aio_notify().
> +             */
> +            smp_mb();
> +
> +            /* Check again in case a shorter timer was added */
> +            timeout = qemu_soonest_timeout(timeout, aio_compute_timeout(ctx));
> +        }
> +
>          ret = ctx->fdmon_ops->wait(ctx, &ready_list, timeout);
> -    }
>  
> -    if (blocking) {
> -        /* Finish the poll before clearing the flag.  */
> -        atomic_store_release(&ctx->notify_me, atomic_read(&ctx->notify_me) - 2);
> -        aio_notify_accept(ctx);
> +        if (timeout) {
> +            /* Finish the poll before clearing the flag.  */
> +            atomic_store_release(&ctx->notify_me,
> +                                 atomic_read(&ctx->notify_me) - 2);
> +        }
>      }

Hi Paolo,
We can avoid calling aio_compute_timeout() like this, what do you think?

  bool use_notify_me = timeout != 0;

  if (use_notify_me) {
      atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
      /*
       * Write ctx->notify_me before computing the timeout
       * (reading bottom half flags, etc.).  Pairs with
       * smp_mb in aio_notify().
       */
      smp_mb();

      /* Don't block if aio_notify() was called */
      if (atomic_read(ctx->notified)) {
          timeout = 0;
      }
  }

  ret = ctx->fdmon_ops->wait(ctx, &ready_list, timeout);

  if (use_notify_me) {
      /* Finish the poll before clearing the flag.  */
      atomic_store_release(&ctx->notify_me,
                           atomic_read(&ctx->notify_me) - 2);
  }

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] aio-posix: keep aio_notify_me disabled during polling

[Paolo Bonzini]

[-- Attachment #1.1: Type: text/plain, Size: 2921 bytes --]

On 04/08/20 12:29, Stefan Hajnoczi wrote:
> On Tue, Aug 04, 2020 at 06:28:04AM +0100, Stefan Hajnoczi wrote:
>> @@ -597,15 +574,38 @@ bool aio_poll(AioContext *ctx, bool blocking)
>>       * system call---a single round of run_poll_handlers_once suffices.
>>       */
>>      if (timeout || ctx->fdmon_ops->need_wait(ctx)) {
>> +        /*
>> +         * aio_notify can avoid the expensive event_notifier_set if
>> +         * everything (file descriptors, bottom halves, timers) will
>> +         * be re-evaluated before the next blocking poll().  This is
>> +         * already true when aio_poll is called with blocking == false;
>> +         * if blocking == true, it is only true after poll() returns,
>> +         * so disable the optimization now.
>> +         */
>> +        if (timeout) {
>> +            atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
>> +            /*
>> +             * Write ctx->notify_me before computing the timeout
>> +             * (reading bottom half flags, etc.).  Pairs with
>> +             * smp_mb in aio_notify().
>> +             */
>> +            smp_mb();
>> +
>> +            /* Check again in case a shorter timer was added */
>> +            timeout = qemu_soonest_timeout(timeout, aio_compute_timeout(ctx));
>> +        }
>> +
>>          ret = ctx->fdmon_ops->wait(ctx, &ready_list, timeout);
>> -    }
>>  
>> -    if (blocking) {
>> -        /* Finish the poll before clearing the flag.  */
>> -        atomic_store_release(&ctx->notify_me, atomic_read(&ctx->notify_me) - 2);
>> -        aio_notify_accept(ctx);
>> +        if (timeout) {
>> +            /* Finish the poll before clearing the flag.  */
>> +            atomic_store_release(&ctx->notify_me,
>> +                                 atomic_read(&ctx->notify_me) - 2);
>> +        }
>>      }
> 
> Hi Paolo,
> We can avoid calling aio_compute_timeout() like this, what do you think?

I don't understand :) except I guess you mean we can avoid the second
call.  Can you post either a complete patch with this squashed, or a 4th
patch (whatever you think is best)?

Paolo

>   bool use_notify_me = timeout != 0;
> 
>   if (use_notify_me) {
>       atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
>       /*
>        * Write ctx->notify_me before computing the timeout
>        * (reading bottom half flags, etc.).  Pairs with
>        * smp_mb in aio_notify().
>        */
>       smp_mb();
> 
>       /* Don't block if aio_notify() was called */
>       if (atomic_read(ctx->notified)) {
>           timeout = 0;
>       }
>   }
> 
>   ret = ctx->fdmon_ops->wait(ctx, &ready_list, timeout);
> 
>   if (use_notify_me) {
>       /* Finish the poll before clearing the flag.  */
>       atomic_store_release(&ctx->notify_me,
>                            atomic_read(&ctx->notify_me) - 2);
>   }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] aio-posix: keep aio_notify_me disabled during polling

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2361 bytes --]

On Tue, Aug 04, 2020 at 06:53:09PM +0200, Paolo Bonzini wrote:
> On 04/08/20 12:29, Stefan Hajnoczi wrote:
> > On Tue, Aug 04, 2020 at 06:28:04AM +0100, Stefan Hajnoczi wrote:
> >> @@ -597,15 +574,38 @@ bool aio_poll(AioContext *ctx, bool blocking)
> >>       * system call---a single round of run_poll_handlers_once suffices.
> >>       */
> >>      if (timeout || ctx->fdmon_ops->need_wait(ctx)) {
> >> +        /*
> >> +         * aio_notify can avoid the expensive event_notifier_set if
> >> +         * everything (file descriptors, bottom halves, timers) will
> >> +         * be re-evaluated before the next blocking poll().  This is
> >> +         * already true when aio_poll is called with blocking == false;
> >> +         * if blocking == true, it is only true after poll() returns,
> >> +         * so disable the optimization now.
> >> +         */
> >> +        if (timeout) {
> >> +            atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
> >> +            /*
> >> +             * Write ctx->notify_me before computing the timeout
> >> +             * (reading bottom half flags, etc.).  Pairs with
> >> +             * smp_mb in aio_notify().
> >> +             */
> >> +            smp_mb();
> >> +
> >> +            /* Check again in case a shorter timer was added */
> >> +            timeout = qemu_soonest_timeout(timeout, aio_compute_timeout(ctx));
> >> +        }
> >> +
> >>          ret = ctx->fdmon_ops->wait(ctx, &ready_list, timeout);
> >> -    }
> >>  
> >> -    if (blocking) {
> >> -        /* Finish the poll before clearing the flag.  */
> >> -        atomic_store_release(&ctx->notify_me, atomic_read(&ctx->notify_me) - 2);
> >> -        aio_notify_accept(ctx);
> >> +        if (timeout) {
> >> +            /* Finish the poll before clearing the flag.  */
> >> +            atomic_store_release(&ctx->notify_me,
> >> +                                 atomic_read(&ctx->notify_me) - 2);
> >> +        }
> >>      }
> > 
> > Hi Paolo,
> > We can avoid calling aio_compute_timeout() like this, what do you think?
> 
> I don't understand :) except I guess you mean we can avoid the second
> call.  Can you post either a complete patch with this squashed, or a 4th
> patch (whatever you think is best)?

Sure, I'll post a new revision of this series.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] aio-posix: keep aio_notify_me disabled during polling

[Paolo Bonzini]

On 04/08/20 07:28, Stefan Hajnoczi wrote:
> This patch series eliminates ctx->notifier EventNotifier activity when
> aio_poll() is in polling mode. There is no need to use the EventNotifier since
> a polling handler can detect that aio_notify() has been called by monitoring a
> field in memory instead.
> 
> Optimizing out the EventNotifier calls improves null-co random read 4KB
> iodepth=1 IOPS by 18%.
> 
> aio_compute_timeout() is now called twice if aio_poll() needs to block, which
> means an extra qemu_clock_get_ns() call is made when there is an active timer.
> An alternative would be to set timeout = 0 if ctx->notified is true before
> blocking, but going around the event loop again could slow things down more.
> 
> I have not modified docs/spin/aio_notify*.promela because I'm not familiar with
> the SPIN model checker.

I'll take a look.  Looks good apart from more comments, on which I
commented.

Paolo