| 1 | The following changes since commit 240ab11fb72049d6373cbbec8d788f8e411a00bc: | 1 | The following changes since commit 887cba855bb6ff4775256f7968409281350b568c: |
|---|---|---|---|
| 2 | 2 | ||
| 3 | Merge remote-tracking branch 'remotes/aperard/tags/pull-xen-20190924' into staging (2019-09-24 15:36:31 +0100) | 3 | configure: Fix cross-building for RISCV host (v5) (2023-07-11 17:56:09 +0100) |
| 4 | 4 | ||
| 5 | are available in the Git repository at: | 5 | are available in the Git repository at: |
| 6 | 6 | ||
| 7 | https://github.com/stefanha/qemu.git tags/block-pull-request | 7 | https://gitlab.com/stefanha/qemu.git tags/block-pull-request |
| 8 | 8 | ||
| 9 | for you to fetch changes up to f9a7e3698a737ee75a7b0af34203303df982550f: | 9 | for you to fetch changes up to 75dcb4d790bbe5327169fd72b185960ca58e2fa6: |
| 10 | 10 | ||
| 11 | virtio-blk: schedule virtio_notify_config to run on main context (2019-09-25 18:06:36 +0100) | 11 | virtio-blk: fix host notifier issues during dataplane start/stop (2023-07-12 15:20:32 -0400) |
| 12 | 12 | ||
| 13 | ---------------------------------------------------------------- | 13 | ---------------------------------------------------------------- |
| 14 | Pull request | 14 | Pull request |
| 15 | 15 | ||
| 16 | ---------------------------------------------------------------- | 16 | ---------------------------------------------------------------- |
| 17 | 17 | ||
| 18 | Sergio Lopez (1): | 18 | Stefan Hajnoczi (1): |
| 19 | virtio-blk: schedule virtio_notify_config to run on main context | 19 | virtio-blk: fix host notifier issues during dataplane start/stop |
| 20 | 20 | ||
| 21 | Vladimir Sementsov-Ogievskiy (1): | 21 | hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++-------------- |
| 22 | util/ioc.c: try to reassure Coverity about qemu_iovec_init_extended | 22 | 1 file changed, 38 insertions(+), 29 deletions(-) |
| 23 | |||
| 24 | hw/block/virtio-blk.c | 16 +++++++++++++++- | ||
| 25 | util/iov.c | 3 ++- | ||
| 26 | 2 files changed, 17 insertions(+), 2 deletions(-) | ||
| 27 | 23 | ||
| 28 | -- | 24 | -- |
| 29 | 2.21.0 | 25 | 2.40.1 |
| 30 | |||
| 31 | diff view generated by jsdifflib |
Deleted patch | |||
|---|---|---|---|
| 1 | From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
| 2 | 1 | ||
| 3 | Make it more obvious, that filling qiov corresponds to qiov allocation, | ||
| 4 | which in turn corresponds to total_niov calculation, based on mid_niov | ||
| 5 | (not mid_len). Still add an assertion to show that there should be no | ||
| 6 | difference. | ||
| 7 | |||
| 8 | Reported-by: Coverity (CID 1405302) | ||
| 9 | Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
| 10 | Message-id: 20190910090310.14032-1-vsementsov@virtuozzo.com | ||
| 11 | Suggested-by: Peter Maydell <peter.maydell@linaro.org> | ||
| 12 | Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
| 13 | Message-Id: <20190910090310.14032-1-vsementsov@virtuozzo.com> | ||
| 14 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
| 15 | --- | ||
| 16 | util/iov.c | 3 ++- | ||
| 17 | 1 file changed, 2 insertions(+), 1 deletion(-) | ||
| 18 | |||
| 19 | diff --git a/util/iov.c b/util/iov.c | ||
| 20 | index XXXXXXX..XXXXXXX 100644 | ||
| 21 | --- a/util/iov.c | ||
| 22 | +++ b/util/iov.c | ||
| 23 | @@ -XXX,XX +XXX,XX @@ void qemu_iovec_init_extended( | ||
| 24 | p++; | ||
| 25 | } | ||
| 26 | |||
| 27 | - if (mid_len) { | ||
| 28 | + assert(!mid_niov == !mid_len); | ||
| 29 | + if (mid_niov) { | ||
| 30 | memcpy(p, mid_iov, mid_niov * sizeof(*p)); | ||
| 31 | p[0].iov_base = (uint8_t *)p[0].iov_base + mid_head; | ||
| 32 | p[0].iov_len -= mid_head; | ||
| 33 | -- | ||
| 34 | 2.21.0 | ||
| 35 | |||
| 36 | diff view generated by jsdifflib |
| 1 | From: Sergio Lopez <slp@redhat.com> | 1 | The main loop thread can consume 100% CPU when using --device |
|---|---|---|---|
| 2 | virtio-blk-pci,iothread=<iothread>. ppoll() constantly returns but | ||
| 3 | reading virtqueue host notifiers fails with EAGAIN. The file descriptors | ||
| 4 | are stale and remain registered with the AioContext because of bugs in | ||
| 5 | the virtio-blk dataplane start/stop code. | ||
| 2 | 6 | ||
| 3 | virtio_notify_config() needs to acquire the global mutex, which isn't | 7 | The problem is that the dataplane start/stop code involves drain |
| 4 | allowed from an iothread, and may lead to a deadlock like this: | 8 | operations, which call virtio_blk_drained_begin() and |
| 9 | virtio_blk_drained_end() at points where the host notifier is not | ||
| 10 | operational: | ||
| 11 | - In virtio_blk_data_plane_start(), blk_set_aio_context() drains after | ||
| 12 | vblk->dataplane_started has been set to true but the host notifier has | ||
| 13 | not been attached yet. | ||
| 14 | - In virtio_blk_data_plane_stop(), blk_drain() and blk_set_aio_context() | ||
| 15 | drain after the host notifier has already been detached but with | ||
| 16 | vblk->dataplane_started still set to true. | ||
| 5 | 17 | ||
| 6 | - main thead | 18 | I would like to simplify ->ioeventfd_start/stop() to avoid interactions |
| 7 | * Has acquired: qemu_global_mutex. | 19 | with drain entirely, but couldn't find a way to do that. Instead, this |
| 8 | * Is trying the acquire: iothread AioContext lock via | 20 | patch accepts the fragile nature of the code and reorders it so that |
| 9 | AIO_WAIT_WHILE (after aio_poll). | 21 | vblk->dataplane_started is false during drain operations. This way the |
| 22 | virtio_blk_drained_begin() and virtio_blk_drained_end() calls don't | ||
| 23 | touch the host notifier. The result is that | ||
| 24 | virtio_blk_data_plane_start() and virtio_blk_data_plane_stop() have | ||
| 25 | complete control over the host notifier and stale file descriptors are | ||
| 26 | no longer left in the AioContext. | ||
| 10 | 27 | ||
| 11 | - iothread | 28 | This patch fixes the 100% CPU consumption in the main loop thread and |
| 12 | * Has acquired: AioContext lock. | 29 | correctly moves host notifier processing to the IOThread. |
| 13 | * Is trying to acquire: qemu_global_mutex (via | ||
| 14 | virtio_notify_config->prepare_mmio_access). | ||
| 15 | 30 | ||
| 16 | If virtio_blk_resize() is called from an iothread, schedule | 31 | Fixes: 1665d9326fd2 ("virtio-blk: implement BlockDevOps->drained_begin()") |
| 17 | virtio_notify_config() to be run in the main context BH. | 32 | Reported-by: Lukáš Doktor <ldoktor@redhat.com> |
| 18 | 33 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | |
| 19 | [Removed unnecessary newline as suggested by Kevin Wolf | 34 | Tested-by: Lukas Doktor <ldoktor@redhat.com> |
| 20 | <kwolf@redhat.com>. | 35 | Message-id: 20230704151527.193586-1-stefanha@redhat.com |
| 21 | --Stefan] | ||
| 22 | |||
| 23 | Signed-off-by: Sergio Lopez <slp@redhat.com> | ||
| 24 | Reviewed-by: Kevin Wolf <kwolf@redhat.com> | ||
| 25 | Message-id: 20190916112411.21636-1-slp@redhat.com | ||
| 26 | Message-Id: <20190916112411.21636-1-slp@redhat.com> | ||
| 27 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | 36 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
| 28 | --- | 37 | --- |
| 29 | hw/block/virtio-blk.c | 16 +++++++++++++++- | 38 | hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++-------------- |
| 30 | 1 file changed, 15 insertions(+), 1 deletion(-) | 39 | 1 file changed, 38 insertions(+), 29 deletions(-) |
| 31 | 40 | ||
| 32 | diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c | 41 | diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c |
| 33 | index XXXXXXX..XXXXXXX 100644 | 42 | index XXXXXXX..XXXXXXX 100644 |
| 34 | --- a/hw/block/virtio-blk.c | 43 | --- a/hw/block/dataplane/virtio-blk.c |
| 35 | +++ b/hw/block/virtio-blk.c | 44 | +++ b/hw/block/dataplane/virtio-blk.c |
| 36 | @@ -XXX,XX +XXX,XX @@ | 45 | @@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev) |
| 37 | #include "qemu/iov.h" | 46 | |
| 38 | #include "qemu/module.h" | 47 | memory_region_transaction_commit(); |
| 39 | #include "qemu/error-report.h" | 48 | |
| 40 | +#include "qemu/main-loop.h" | 49 | - /* |
| 41 | #include "trace.h" | 50 | - * These fields are visible to the IOThread so we rely on implicit barriers |
| 42 | #include "hw/block/block.h" | 51 | - * in aio_context_acquire() on the write side and aio_notify_accept() on |
| 43 | #include "hw/qdev-properties.h" | 52 | - * the read side. |
| 44 | @@ -XXX,XX +XXX,XX @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f, | 53 | - */ |
| 45 | return 0; | 54 | - s->starting = false; |
| 55 | - vblk->dataplane_started = true; | ||
| 56 | trace_virtio_blk_data_plane_start(s); | ||
| 57 | |||
| 58 | old_context = blk_get_aio_context(s->conf->conf.blk); | ||
| 59 | @@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev) | ||
| 60 | event_notifier_set(virtio_queue_get_host_notifier(vq)); | ||
| 61 | } | ||
| 62 | |||
| 63 | + /* | ||
| 64 | + * These fields must be visible to the IOThread when it processes the | ||
| 65 | + * virtqueue, otherwise it will think dataplane has not started yet. | ||
| 66 | + * | ||
| 67 | + * Make sure ->dataplane_started is false when blk_set_aio_context() is | ||
| 68 | + * called above so that draining does not cause the host notifier to be | ||
| 69 | + * detached/attached prematurely. | ||
| 70 | + */ | ||
| 71 | + s->starting = false; | ||
| 72 | + vblk->dataplane_started = true; | ||
| 73 | + smp_wmb(); /* paired with aio_notify_accept() on the read side */ | ||
| 74 | + | ||
| 75 | /* Get this show started by hooking up our callbacks */ | ||
| 76 | if (!blk_in_drain(s->conf->conf.blk)) { | ||
| 77 | aio_context_acquire(s->ctx); | ||
| 78 | @@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev) | ||
| 79 | fail_guest_notifiers: | ||
| 80 | vblk->dataplane_disabled = true; | ||
| 81 | s->starting = false; | ||
| 82 | - vblk->dataplane_started = true; | ||
| 83 | return -ENOSYS; | ||
| 46 | } | 84 | } |
| 47 | 85 | ||
| 48 | +static void virtio_resize_cb(void *opaque) | 86 | @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev) |
| 49 | +{ | 87 | aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s); |
| 50 | + VirtIODevice *vdev = opaque; | 88 | } |
| 89 | |||
| 90 | + /* | ||
| 91 | + * Batch all the host notifiers in a single transaction to avoid | ||
| 92 | + * quadratic time complexity in address_space_update_ioeventfds(). | ||
| 93 | + */ | ||
| 94 | + memory_region_transaction_begin(); | ||
| 51 | + | 95 | + |
| 52 | + assert(qemu_get_current_aio_context() == qemu_get_aio_context()); | 96 | + for (i = 0; i < nvqs; i++) { |
| 53 | + virtio_notify_config(vdev); | 97 | + virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false); |
| 54 | +} | 98 | + } |
| 55 | + | 99 | + |
| 56 | static void virtio_blk_resize(void *opaque) | ||
| 57 | { | ||
| 58 | VirtIODevice *vdev = VIRTIO_DEVICE(opaque); | ||
| 59 | |||
| 60 | - virtio_notify_config(vdev); | ||
| 61 | + /* | 100 | + /* |
| 62 | + * virtio_notify_config() needs to acquire the global mutex, | 101 | + * The transaction expects the ioeventfds to be open when it |
| 63 | + * so it can't be called from an iothread. Instead, schedule | 102 | + * commits. Do it now, before the cleanup loop. |
| 64 | + * it to be run in the main context BH. | ||
| 65 | + */ | 103 | + */ |
| 66 | + aio_bh_schedule_oneshot(qemu_get_aio_context(), virtio_resize_cb, vdev); | 104 | + memory_region_transaction_commit(); |
| 105 | + | ||
| 106 | + for (i = 0; i < nvqs; i++) { | ||
| 107 | + virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i); | ||
| 108 | + } | ||
| 109 | + | ||
| 110 | + /* | ||
| 111 | + * Set ->dataplane_started to false before draining so that host notifiers | ||
| 112 | + * are not detached/attached anymore. | ||
| 113 | + */ | ||
| 114 | + vblk->dataplane_started = false; | ||
| 115 | + | ||
| 116 | aio_context_acquire(s->ctx); | ||
| 117 | |||
| 118 | /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */ | ||
| 119 | @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev) | ||
| 120 | |||
| 121 | aio_context_release(s->ctx); | ||
| 122 | |||
| 123 | - /* | ||
| 124 | - * Batch all the host notifiers in a single transaction to avoid | ||
| 125 | - * quadratic time complexity in address_space_update_ioeventfds(). | ||
| 126 | - */ | ||
| 127 | - memory_region_transaction_begin(); | ||
| 128 | - | ||
| 129 | - for (i = 0; i < nvqs; i++) { | ||
| 130 | - virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false); | ||
| 131 | - } | ||
| 132 | - | ||
| 133 | - /* | ||
| 134 | - * The transaction expects the ioeventfds to be open when it | ||
| 135 | - * commits. Do it now, before the cleanup loop. | ||
| 136 | - */ | ||
| 137 | - memory_region_transaction_commit(); | ||
| 138 | - | ||
| 139 | - for (i = 0; i < nvqs; i++) { | ||
| 140 | - virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i); | ||
| 141 | - } | ||
| 142 | - | ||
| 143 | qemu_bh_cancel(s->bh); | ||
| 144 | notify_guest_bh(s); /* final chance to notify guest */ | ||
| 145 | |||
| 146 | /* Clean up guest notifier (irq) */ | ||
| 147 | k->set_guest_notifiers(qbus->parent, nvqs, false); | ||
| 148 | |||
| 149 | - vblk->dataplane_started = false; | ||
| 150 | s->stopping = false; | ||
| 67 | } | 151 | } |
| 68 | |||
| 69 | static const BlockDevOps virtio_block_ops = { | ||
| 70 | -- | 152 | -- |
| 71 | 2.21.0 | 153 | 2.40.1 |
| 72 | 154 | ||
| 73 | 155 | diff view generated by jsdifflib |