Series comparison

 [PULL 0/1] Block patches
-The following changes since commit 41192db338588051f21501abc13743e62b0a5605:
+The following changes since commit 887cba855bb6ff4775256f7968409281350b568c:
-  Merge remote-tracking branch 'remotes/ehabkost-gl/tags/machine-next-pull-request' into staging (2021-01-01 22:57:15 +0000)
+  configure: Fix cross-building for RISCV host (v5) (2023-07-11 17:56:09 +0100)
 are available in the Git repository at:
   https://gitlab.com/stefanha/qemu.git tags/block-pull-request
-for you to fetch changes up to 593621f36b716eb091c4ec791db72dd4461789da:
+for you to fetch changes up to 75dcb4d790bbe5327169fd72b185960ca58e2fa6:
-  readline: Fix possible array index out of bounds in readline_hist_add() (2021-01-04 11:13:39 +0000)
+  virtio-blk: fix host notifier issues during dataplane start/stop (2023-07-12 15:20:32 -0400)
 ----------------------------------------------------------------
 Pull request
 ----------------------------------------------------------------
-Alex Chen (1):
+Stefan Hajnoczi (1):
-  readline: Fix possible array index out of bounds in
+  virtio-blk: fix host notifier issues during dataplane start/stop
     readline_hist_add()
- util/readline.c | 3 +++
+ hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++--------------
-file changed, 3 insertions(+)
+file changed, 38 insertions(+), 29 deletions(-)
 --
-.29.2
+.40.1

-[PULL 1/1] readline: Fix possible array index out of bounds in readline_hist_add()
+[PULL 1/1] virtio-blk: fix host notifier issues during dataplane start/stop
-From: Alex Chen <alex.chen@huawei.com>
+The main loop thread can consume 100% CPU when using --device
 virtio-blk-pci,iothread=<iothread>. ppoll() constantly returns but
 reading virtqueue host notifiers fails with EAGAIN. The file descriptors
 are stale and remain registered with the AioContext because of bugs in
 the virtio-blk dataplane start/stop code.
-When the 'cmdline' is the last entry in 'rs->history' array, there is
+The problem is that the dataplane start/stop code involves drain
-no need to put this entry to the end of the array, partly because it is
+operations, which call virtio_blk_drained_begin() and
-the last entry, and partly because the next operition will lead to array
+virtio_blk_drained_end() at points where the host notifier is not
-index out of bounds.
+operational:
 - In virtio_blk_data_plane_start(), blk_set_aio_context() drains after
   vblk->dataplane_started has been set to true but the host notifier has
   not been attached yet.
 - In virtio_blk_data_plane_stop(), blk_drain() and blk_set_aio_context()
   drain after the host notifier has already been detached but with
   vblk->dataplane_started still set to true.
-Reported-by: Euler Robot <euler.robot@huawei.com>
+I would like to simplify ->ioeventfd_start/stop() to avoid interactions
-Signed-off-by: Alex Chen <alex.chen@huawei.com>
+with drain entirely, but couldn't find a way to do that. Instead, this
-Message-id: 20201203135043.117072-1-alex.chen@huawei.com
+patch accepts the fragile nature of the code and reorders it so that
 vblk->dataplane_started is false during drain operations. This way the
 virtio_blk_drained_begin() and virtio_blk_drained_end() calls don't
 touch the host notifier. The result is that
 virtio_blk_data_plane_start() and virtio_blk_data_plane_stop() have
 complete control over the host notifier and stale file descriptors are
 no longer left in the AioContext.
 This patch fixes the 100% CPU consumption in the main loop thread and
 correctly moves host notifier processing to the IOThread.
 Fixes: 1665d9326fd2 ("virtio-blk: implement BlockDevOps->drained_begin()")
 Reported-by: Lukáš Doktor <ldoktor@redhat.com>
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 Tested-by: Lukas Doktor <ldoktor@redhat.com>
 Message-id: 20230704151527.193586-1-stefanha@redhat.com
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- util/readline.c | 3 +++
+ hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++--------------
-file changed, 3 insertions(+)
+file changed, 38 insertions(+), 29 deletions(-)
-diff --git a/util/readline.c b/util/readline.c
+diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
 index XXXXXXX..XXXXXXX 100644
---- a/util/readline.c
+--- a/hw/block/dataplane/virtio-blk.c
-+++ b/util/readline.c
++++ b/hw/block/dataplane/virtio-blk.c
-@@ -XXX,XX +XXX,XX @@ static void readline_hist_add(ReadLineState *rs, const char *cmdline)
+@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
-         }
-         if (strcmp(hist_entry, cmdline) == 0) {
+     memory_region_transaction_commit();
-         same_entry:
-+            if (idx == READLINE_MAX_CMDS - 1) {
+-    /*
-+                return;
+-     * These fields are visible to the IOThread so we rely on implicit barriers
-+            }
+-     * in aio_context_acquire() on the write side and aio_notify_accept() on
-             new_entry = hist_entry;
+-     * the read side.
-             /* Put this entry at the end of history */
+-     */
-             memmove(&rs->history[idx], &rs->history[idx + 1],
+-    s->starting = false;
 -    vblk->dataplane_started = true;
      trace_virtio_blk_data_plane_start(s);
      old_context = blk_get_aio_context(s->conf->conf.blk);
@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
          event_notifier_set(virtio_queue_get_host_notifier(vq));
      }
 +    /*
 +     * These fields must be visible to the IOThread when it processes the
 +     * virtqueue, otherwise it will think dataplane has not started yet.
 +     *
 +     * Make sure ->dataplane_started is false when blk_set_aio_context() is
 +     * called above so that draining does not cause the host notifier to be
 +     * detached/attached prematurely.
 +     */
 +    s->starting = false;
 +    vblk->dataplane_started = true;
 +    smp_wmb(); /* paired with aio_notify_accept() on the read side */
 +
      /* Get this show started by hooking up our callbacks */
      if (!blk_in_drain(s->conf->conf.blk)) {
          aio_context_acquire(s->ctx);
@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
    fail_guest_notifiers:
      vblk->dataplane_disabled = true;
      s->starting = false;
 -    vblk->dataplane_started = true;
      return -ENOSYS;
  }
@@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
          aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s);
      }
 +    /*
 +     * Batch all the host notifiers in a single transaction to avoid
 +     * quadratic time complexity in address_space_update_ioeventfds().
 +     */
 +    memory_region_transaction_begin();
 +
 +    for (i = 0; i < nvqs; i++) {
 +        virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
 +    }
 +
 +    /*
 +     * The transaction expects the ioeventfds to be open when it
 +     * commits. Do it now, before the cleanup loop.
 +     */
 +    memory_region_transaction_commit();
 +
 +    for (i = 0; i < nvqs; i++) {
 +        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
 +    }
 +
 +    /*
 +     * Set ->dataplane_started to false before draining so that host notifiers
 +     * are not detached/attached anymore.
 +     */
 +    vblk->dataplane_started = false;
 +
      aio_context_acquire(s->ctx);
      /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */
@@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
      aio_context_release(s->ctx);
 -    /*
 -     * Batch all the host notifiers in a single transaction to avoid
 -     * quadratic time complexity in address_space_update_ioeventfds().
 -     */
 -    memory_region_transaction_begin();
 -
 -    for (i = 0; i < nvqs; i++) {
 -        virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
 -    }
 -
 -    /*
 -     * The transaction expects the ioeventfds to be open when it
 -     * commits. Do it now, before the cleanup loop.
 -     */
 -    memory_region_transaction_commit();
 -
 -    for (i = 0; i < nvqs; i++) {
 -        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
 -    }
 -
      qemu_bh_cancel(s->bh);
      notify_guest_bh(s); /* final chance to notify guest */
      /* Clean up guest notifier (irq) */
      k->set_guest_notifiers(qbus->parent, nvqs, false);
 -    vblk->dataplane_started = false;
      s->stopping = false;
  }
 --
-.29.2
+.40.1

From: Alex Chen <alex.chen@huawei.com>

When the 'cmdline' is the last entry in 'rs->history' array, there is
no need to put this entry to the end of the array, partly because it is
the last entry, and partly because the next operition will lead to array
index out of bounds.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Alex Chen <alex.chen@huawei.com>
Message-id: 20201203135043.117072-1-alex.chen@huawei.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/readline.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/util/readline.c b/util/readline.c
index XXXXXXX..XXXXXXX 100644
--- a/util/readline.c
+++ b/util/readline.c
@@ -XXX,XX +XXX,XX @@ static void readline_hist_add(ReadLineState *rs, const char *cmdline)
         }
         if (strcmp(hist_entry, cmdline) == 0) {
         same_entry:
+            if (idx == READLINE_MAX_CMDS - 1) {
+                return;
+            }
             new_entry = hist_entry;
             /* Put this entry at the end of history */
             memmove(&rs->history[idx], &rs->history[idx + 1],
-- 
2.29.2

The main loop thread can consume 100% CPU when using --device
virtio-blk-pci,iothread=<iothread>. ppoll() constantly returns but
reading virtqueue host notifiers fails with EAGAIN. The file descriptors
are stale and remain registered with the AioContext because of bugs in
the virtio-blk dataplane start/stop code.

The problem is that the dataplane start/stop code involves drain
operations, which call virtio_blk_drained_begin() and
virtio_blk_drained_end() at points where the host notifier is not
operational:
- In virtio_blk_data_plane_start(), blk_set_aio_context() drains after
  vblk->dataplane_started has been set to true but the host notifier has
  not been attached yet.
- In virtio_blk_data_plane_stop(), blk_drain() and blk_set_aio_context()
  drain after the host notifier has already been detached but with
  vblk->dataplane_started still set to true.

I would like to simplify ->ioeventfd_start/stop() to avoid interactions
with drain entirely, but couldn't find a way to do that. Instead, this
patch accepts the fragile nature of the code and reorders it so that
vblk->dataplane_started is false during drain operations. This way the
virtio_blk_drained_begin() and virtio_blk_drained_end() calls don't
touch the host notifier. The result is that
virtio_blk_data_plane_start() and virtio_blk_data_plane_stop() have
complete control over the host notifier and stale file descriptors are
no longer left in the AioContext.

This patch fixes the 100% CPU consumption in the main loop thread and
correctly moves host notifier processing to the IOThread.

Fixes: 1665d9326fd2 ("virtio-blk: implement BlockDevOps->drained_begin()")
Reported-by: Lukáš Doktor <ldoktor@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-by: Lukas Doktor <ldoktor@redhat.com>
Message-id: 20230704151527.193586-1-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 29 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
 
     memory_region_transaction_commit();
 
-    /*
-     * These fields are visible to the IOThread so we rely on implicit barriers
-     * in aio_context_acquire() on the write side and aio_notify_accept() on
-     * the read side.
-     */
-    s->starting = false;
-    vblk->dataplane_started = true;
     trace_virtio_blk_data_plane_start(s);
 
     old_context = blk_get_aio_context(s->conf->conf.blk);
@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
         event_notifier_set(virtio_queue_get_host_notifier(vq));
     }
 
+    /*
+     * These fields must be visible to the IOThread when it processes the
+     * virtqueue, otherwise it will think dataplane has not started yet.
+     *
+     * Make sure ->dataplane_started is false when blk_set_aio_context() is
+     * called above so that draining does not cause the host notifier to be
+     * detached/attached prematurely.
+     */
+    s->starting = false;
+    vblk->dataplane_started = true;
+    smp_wmb(); /* paired with aio_notify_accept() on the read side */
+
     /* Get this show started by hooking up our callbacks */
     if (!blk_in_drain(s->conf->conf.blk)) {
         aio_context_acquire(s->ctx);
@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
   fail_guest_notifiers:
     vblk->dataplane_disabled = true;
     s->starting = false;
-    vblk->dataplane_started = true;
     return -ENOSYS;
 }
 
@@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
         aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s);
     }
 
+    /*
+     * Batch all the host notifiers in a single transaction to avoid
+     * quadratic time complexity in address_space_update_ioeventfds().
+     */
+    memory_region_transaction_begin();
+
+    for (i = 0; i < nvqs; i++) {
+        virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+    }
+
+    /*
+     * The transaction expects the ioeventfds to be open when it
+     * commits. Do it now, before the cleanup loop.
+     */
+    memory_region_transaction_commit();
+
+    for (i = 0; i < nvqs; i++) {
+        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
+    }
+
+    /*
+     * Set ->dataplane_started to false before draining so that host notifiers
+     * are not detached/attached anymore.
+     */
+    vblk->dataplane_started = false;
+
     aio_context_acquire(s->ctx);
 
     /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */
@@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
 
     aio_context_release(s->ctx);
 
-    /*
-     * Batch all the host notifiers in a single transaction to avoid
-     * quadratic time complexity in address_space_update_ioeventfds().
-     */
-    memory_region_transaction_begin();
-
-    for (i = 0; i < nvqs; i++) {
-        virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
-    }
-
-    /*
-     * The transaction expects the ioeventfds to be open when it
-     * commits. Do it now, before the cleanup loop.
-     */
-    memory_region_transaction_commit();
-
-    for (i = 0; i < nvqs; i++) {
-        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
-    }
-
     qemu_bh_cancel(s->bh);
     notify_guest_bh(s); /* final chance to notify guest */
 
     /* Clean up guest notifier (irq) */
     k->set_guest_notifiers(qbus->parent, nvqs, false);
 
-    vblk->dataplane_started = false;
     s->stopping = false;
 }
-- 
2.40.1