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

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

[Stefan Hajnoczi]

v3:
 * Use smp_wmb() in aio_notify_accept() [Paolo]
 * Flatten if statement in aio_poll() [Paolo]

v2:
 * Added smp_mb() in aio_notify_accept() [Paolo]
 * Added comments about memory barrier pairing [Paolo]
 * Eliminated extra aio_compute_timeout() before calling ppoll()

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%.

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 | 47 +++++++++++++++++++++++++----------------------
 util/async.c     | 36 +++++++++++++++++++++++-------------
 2 files changed, 48 insertions(+), 35 deletions(-)

-- 
2.26.2

[PATCH v3 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@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 v3 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. Therefore aio_notify_me needs to be enabled
during polling.

This is suboptimal since expensive event_notifier_set(&ctx->notifier)
and event_notifier_test_and_clear(&ctx->notifier) calls are required
when ctx->aio_notify_me is enabled.

Change aio_notify() so that aio->notified is always set, regardless of
ctx->aio_notify_me. This will make polling cheaper since
ctx->aio_notify_me can remain 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>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/async.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/util/async.c b/util/async.c
index d9f987e133..99e15644d9 100644
--- a/util/async.c
+++ b/util/async.c
@@ -419,25 +419,32 @@ LuringState *aio_get_linux_io_uring(AioContext *ctx)
 
 void aio_notify(AioContext *ctx)
 {
-    /* Write e.g. bh->scheduled before reading ctx->notify_me.  Pairs
+    /*
+     * Write e.g. bh->flags before writing ctx->notified.  Pairs with smp_mb in
+     * aio_notify_accept.
+     */
+    smp_wmb();
+    atomic_set(&ctx->notified, true);
+
+    /*
+     * Write ctx->notified before reading ctx->notify_me.  Pairs
      * with smp_mb in aio_ctx_prepare or aio_poll.
      */
     smp_mb();
     if (atomic_read(&ctx->notify_me)) {
         event_notifier_set(&ctx->notifier);
-        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_set(&ctx->notified, false);
+
+    /*
+     * Write ctx->notified before reading e.g. bh->flags.  Pairs with smp_mb in
+     * aio_notify.
+     */
+    smp_wmb();
 }
 
 static void aio_timerlist_notify(void *opaque, QEMUClockType type)
@@ -445,8 +452,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 +518,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

[PATCH v3 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 | 47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/util/aio-posix.c b/util/aio-posix.c
index 1b2a3af65b..f7f13ebfc2 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
@@ -556,6 +550,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
     AioHandlerList ready_list = QLIST_HEAD_INITIALIZER(ready_list);
     int ret = 0;
     bool progress;
+    bool use_notify_me;
     int64_t timeout;
     int64_t start = 0;
 
@@ -566,33 +561,39 @@ bool aio_poll(AioContext *ctx, bool blocking)
      */
     assert(in_aio_context_home_thread(ctx));
 
-    /* aio_notify can avoid the expensive event_notifier_set if
+    qemu_lockcnt_inc(&ctx->list_lock);
+
+    if (ctx->poll_max_ns) {
+        start = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+    }
+
+    timeout = blocking ? aio_compute_timeout(ctx) : 0;
+    progress = try_poll_mode(ctx, &timeout);
+    assert(!(timeout && progress));
+
+    /*
+     * 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) {
+    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
+         * Write ctx->notify_me before reading ctx->notified.  Pairs with
          * smp_mb in aio_notify().
          */
         smp_mb();
-    }
-
-    qemu_lockcnt_inc(&ctx->list_lock);
 
-    if (ctx->poll_max_ns) {
-        start = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+        /* Don't block if aio_notify() was called */
+        if (atomic_read(&ctx->notified)) {
+            timeout = 0;
+        }
     }
 
-    timeout = blocking ? aio_compute_timeout(ctx) : 0;
-    progress = try_poll_mode(ctx, &timeout);
-    assert(!(timeout && progress));
-
     /* If polling is allowed, non-blocking aio_poll does not need the
      * system call---a single round of run_poll_handlers_once suffices.
      */
@@ -600,12 +601,14 @@ bool aio_poll(AioContext *ctx, bool blocking)
         ret = ctx->fdmon_ops->wait(ctx, &ready_list, timeout);
     }
 
-    if (blocking) {
+    if (use_notify_me) {
         /* Finish the poll before clearing the flag.  */
-        atomic_store_release(&ctx->notify_me, atomic_read(&ctx->notify_me) - 2);
-        aio_notify_accept(ctx);
+        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 v3 2/3] async: always set ctx->notified in aio_notify()

[Paolo Bonzini]

On 06/08/20 15:18, Stefan Hajnoczi wrote:
> +    atomic_set(&ctx->notified, false);
> +
> +    /*
> +     * Write ctx->notified before reading e.g. bh->flags.  Pairs with smp_mb in
> +     * aio_notify.
> +     */
> +    smp_wmb();

Sorry I was not clear: the memory barrier has to be smp_mb(), but the
comment has to say smp_wmb().  No need to repost for this.

Paolo

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

[Paolo Bonzini]

On 06/08/20 15:17, Stefan Hajnoczi wrote:
> v3:
>  * Use smp_wmb() in aio_notify_accept() [Paolo]
>  * Flatten if statement in aio_poll() [Paolo]
> 
> v2:
>  * Added smp_mb() in aio_notify_accept() [Paolo]
>  * Added comments about memory barrier pairing [Paolo]
>  * Eliminated extra aio_compute_timeout() before calling ppoll()
> 
> 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%.
> 
> 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 | 47 +++++++++++++++++++++++++----------------------
>  util/async.c     | 36 +++++++++++++++++++++++-------------
>  2 files changed, 48 insertions(+), 35 deletions(-)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

apart from a nit on patch 2.

Paolo

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

[Stefan Hajnoczi]

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

On Thu, Aug 06, 2020 at 03:45:33PM +0200, Paolo Bonzini wrote:
> On 06/08/20 15:18, Stefan Hajnoczi wrote:
> > +    atomic_set(&ctx->notified, false);
> > +
> > +    /*
> > +     * Write ctx->notified before reading e.g. bh->flags.  Pairs with smp_mb in
> > +     * aio_notify.
> > +     */
> > +    smp_wmb();
> 
> Sorry I was not clear: the memory barrier has to be smp_mb(), but the
> comment has to say smp_wmb().  No need to repost for this.

Ah, right! I'll fix it up when merging. Thanks!

Stefan

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

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

[Stefan Hajnoczi]

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

On Thu, Aug 06, 2020 at 02:17:59PM +0100, Stefan Hajnoczi wrote:
> v3:
>  * Use smp_wmb() in aio_notify_accept() [Paolo]
>  * Flatten if statement in aio_poll() [Paolo]
> 
> v2:
>  * Added smp_mb() in aio_notify_accept() [Paolo]
>  * Added comments about memory barrier pairing [Paolo]
>  * Eliminated extra aio_compute_timeout() before calling ppoll()
> 
> 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%.
> 
> 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 | 47 +++++++++++++++++++++++++----------------------
>  util/async.c     | 36 +++++++++++++++++++++++-------------
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

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