Series comparison

-[Qemu-devel] [PULL 0/1] Block patches
+[PULL 0/1] Block patches
-The following changes since commit 4100a344eb3d50d88f9da85cae334afc47aee134:
+The following changes since commit 98b2e3c9ab3abfe476a2b02f8f51813edb90e72d:
-  Merge remote-tracking branch 'remotes/sstabellini/tags/xen-20170202' into staging (2017-02-03 12:31:40 +0000)
+  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2019-10-08 16:08:35 +0100)
-are available in the git repository at:
+are available in the Git repository at:
-  git://github.com/stefanha/qemu.git tags/block-pull-request
+  https://github.com/stefanha/qemu.git tags/block-pull-request
-for you to fetch changes up to cdd7abfdba9287a289c404dfdcb02316f9ffee7d:
+for you to fetch changes up to 69de48445a0d6169f1e2a6c5bfab994e1c810e33:
-  iothread: enable AioContext polling by default (2017-02-03 14:23:38 +0000)
+  test-bdrv-drain: fix iothread_join() hang (2019-10-14 09:48:01 +0100)
 ----------------------------------------------------------------
+Pull request
 ----------------------------------------------------------------
 Stefan Hajnoczi (1):
-  iothread: enable AioContext polling by default
+  test-bdrv-drain: fix iothread_join() hang
- iothread.c | 14 ++++++++++++++
+ tests/iothread.c | 10 ++++++++--
-file changed, 14 insertions(+)
+file changed, 8 insertions(+), 2 deletions(-)
 --
-.9.3
+.21.0

-[Qemu-devel] [PULL 1/1] iothread: enable AioContext polling by default
+[PULL 1/1] test-bdrv-drain: fix iothread_join() hang
-IOThread AioContexts are likely to consist only of event sources like
+tests/test-bdrv-drain can hang in tests/iothread.c:iothread_run():
 virtqueue ioeventfds and LinuxAIO completion eventfds that are pollable
 from userspace (without system calls).
-We recently merged the AioContext polling feature but didn't enable it
+  while (!atomic_read(&iothread->stopping)) {
-by default yet.  I have gone back over the performance data on the
+      aio_poll(iothread->ctx, true);
-mailing list and picked a default polling value that gave good results.
+  }
-Let's enable AioContext polling by default so users don't have another
+The iothread_join() function works as follows:
 switch they need to set manually.  If performance regressions are found
 we can still disable this for the QEMU 2.9 release.
-Cc: Paolo Bonzini <pbonzini@redhat.com>
+  void iothread_join(IOThread *iothread)
-Cc: Christian Borntraeger <borntraeger@de.ibm.com>
+  {
-Cc: Karl Rister <krister@redhat.com>
+      iothread->stopping = true;
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
+      aio_notify(iothread->ctx);
-Message-id: 20170126170119.27876-1-stefanha@redhat.com
+      qemu_thread_join(&iothread->thread);
 If iothread_run() checks iothread->stopping before the iothread_join()
 thread sets stopping to true, then aio_notify() may be optimized away
 and iothread_run() hangs forever in aio_poll().
 The correct way to change iothread->stopping is from a BH that executes
 within iothread_run().  This ensures that iothread->stopping is checked
 after we set it to true.
 This was already fixed for ./iothread.c (note this is a different source
 file!) by commit 2362a28ea11c145e1a13ae79342d76dc118a72a6 ("iothread:
 fix iothread_stop() race condition"), but not for tests/iothread.c.
 Fixes: 0c330a734b51c177ab8488932ac3b0c4d63a718a
        ("aio: introduce aio_co_schedule and aio_co_wake")
 Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
 Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
 Message-Id: <20191003100103.331-1-stefanha@redhat.com>
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- iothread.c | 14 ++++++++++++++
+ tests/iothread.c | 10 ++++++++--
-file changed, 14 insertions(+)
+file changed, 8 insertions(+), 2 deletions(-)
-diff --git a/iothread.c b/iothread.c
+diff --git a/tests/iothread.c b/tests/iothread.c
 index XXXXXXX..XXXXXXX 100644
---- a/iothread.c
+--- a/tests/iothread.c
-+++ b/iothread.c
++++ b/tests/iothread.c
-@@ -XXX,XX +XXX,XX @@ typedef ObjectClass IOThreadClass;
+@@ -XXX,XX +XXX,XX @@ static void *iothread_run(void *opaque)
- #define IOTHREAD_CLASS(klass) \
+     return NULL;
-    OBJECT_CLASS_CHECK(IOThreadClass, klass, TYPE_IOTHREAD)
+ }
-+/* Benchmark results from 2016 on NVMe SSD drives show max polling times around
+-void iothread_join(IOThread *iothread)
-+ * 16-32 microseconds yield IOPS improvements for both iodepth=1 and iodepth=32
++static void iothread_stop_bh(void *opaque)
-+ * workloads.
+ {
-+ */
++    IOThread *iothread = opaque;
 +#define IOTHREAD_POLL_MAX_NS_DEFAULT 32768ULL
 +
- static __thread IOThread *my_iothread;
+     iothread->stopping = true;
+-    aio_notify(iothread->ctx);
  AioContext *qemu_get_current_aio_context(void)
@@ -XXX,XX +XXX,XX @@ static int iothread_stop(Object *object, void *opaque)
      return 0;
  }
 +static void iothread_instance_init(Object *obj)
 +{
 +    IOThread *iothread = IOTHREAD(obj);
 +
 +    iothread->poll_max_ns = IOTHREAD_POLL_MAX_NS_DEFAULT;
 +}
 +
- static void iothread_instance_finalize(Object *obj)
++void iothread_join(IOThread *iothread)
- {
++{
-     IOThread *iothread = IOTHREAD(obj);
++    aio_bh_schedule_oneshot(iothread->ctx, iothread_stop_bh, iothread);
-@@ -XXX,XX +XXX,XX @@ static const TypeInfo iothread_info = {
+     qemu_thread_join(&iothread->thread);
-     .parent = TYPE_OBJECT,
+     qemu_cond_destroy(&iothread->init_done_cond);
-     .class_init = iothread_class_init,
+     qemu_mutex_destroy(&iothread->init_done_lock);
      .instance_size = sizeof(IOThread),
 +    .instance_init = iothread_instance_init,
      .instance_finalize = iothread_instance_finalize,
      .interfaces = (InterfaceInfo[]) {
          {TYPE_USER_CREATABLE},
 --
-.9.3
+.21.0

IOThread AioContexts are likely to consist only of event sources like
virtqueue ioeventfds and LinuxAIO completion eventfds that are pollable
from userspace (without system calls).

We recently merged the AioContext polling feature but didn't enable it
by default yet.  I have gone back over the performance data on the
mailing list and picked a default polling value that gave good results.

Let's enable AioContext polling by default so users don't have another
switch they need to set manually.  If performance regressions are found
we can still disable this for the QEMU 2.9 release.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Karl Rister <krister@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 20170126170119.27876-1-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 iothread.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/iothread.c b/iothread.c
index XXXXXXX..XXXXXXX 100644
--- a/iothread.c
+++ b/iothread.c
@@ -XXX,XX +XXX,XX @@ typedef ObjectClass IOThreadClass;
 #define IOTHREAD_CLASS(klass) \
    OBJECT_CLASS_CHECK(IOThreadClass, klass, TYPE_IOTHREAD)
 
+/* Benchmark results from 2016 on NVMe SSD drives show max polling times around
+ * 16-32 microseconds yield IOPS improvements for both iodepth=1 and iodepth=32
+ * workloads.
+ */
+#define IOTHREAD_POLL_MAX_NS_DEFAULT 32768ULL
+
 static __thread IOThread *my_iothread;
 
 AioContext *qemu_get_current_aio_context(void)
@@ -XXX,XX +XXX,XX @@ static int iothread_stop(Object *object, void *opaque)
     return 0;
 }
 
+static void iothread_instance_init(Object *obj)
+{
+    IOThread *iothread = IOTHREAD(obj);
+
+    iothread->poll_max_ns = IOTHREAD_POLL_MAX_NS_DEFAULT;
+}
+
 static void iothread_instance_finalize(Object *obj)
 {
     IOThread *iothread = IOTHREAD(obj);
@@ -XXX,XX +XXX,XX @@ static const TypeInfo iothread_info = {
     .parent = TYPE_OBJECT,
     .class_init = iothread_class_init,
     .instance_size = sizeof(IOThread),
+    .instance_init = iothread_instance_init,
     .instance_finalize = iothread_instance_finalize,
     .interfaces = (InterfaceInfo[]) {
         {TYPE_USER_CREATABLE},
-- 
2.9.3

tests/test-bdrv-drain can hang in tests/iothread.c:iothread_run():

while (!atomic_read(&iothread->stopping)) {
      aio_poll(iothread->ctx, true);
  }

The iothread_join() function works as follows:

void iothread_join(IOThread *iothread)
  {
      iothread->stopping = true;
      aio_notify(iothread->ctx);
      qemu_thread_join(&iothread->thread);

If iothread_run() checks iothread->stopping before the iothread_join()
thread sets stopping to true, then aio_notify() may be optimized away
and iothread_run() hangs forever in aio_poll().

The correct way to change iothread->stopping is from a BH that executes
within iothread_run().  This ensures that iothread->stopping is checked
after we set it to true.

This was already fixed for ./iothread.c (note this is a different source
file!) by commit 2362a28ea11c145e1a13ae79342d76dc118a72a6 ("iothread:
fix iothread_stop() race condition"), but not for tests/iothread.c.

Fixes: 0c330a734b51c177ab8488932ac3b0c4d63a718a
       ("aio: introduce aio_co_schedule and aio_co_wake")
Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20191003100103.331-1-stefanha@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/iothread.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/iothread.c b/tests/iothread.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/iothread.c
+++ b/tests/iothread.c
@@ -XXX,XX +XXX,XX @@ static void *iothread_run(void *opaque)
     return NULL;
 }
 
-void iothread_join(IOThread *iothread)
+static void iothread_stop_bh(void *opaque)
 {
+    IOThread *iothread = opaque;
+
     iothread->stopping = true;
-    aio_notify(iothread->ctx);
+}
+
+void iothread_join(IOThread *iothread)
+{
+    aio_bh_schedule_oneshot(iothread->ctx, iothread_stop_bh, iothread);
     qemu_thread_join(&iothread->thread);
     qemu_cond_destroy(&iothread->init_done_cond);
     qemu_mutex_destroy(&iothread->init_done_lock);
-- 
2.21.0