:p
atchew
Login
v2: - Rename blk_io_plug() to defer_call() and move it to util/ so the net subsystem can use it [Ilya] - Add defer_call_begin()/end() to thread_pool_completion_bh() to match Linux AIO and io_uring completion batching Replace the seldom-used virtio-blk notification BH mechanism with blk_io_plug(). This is part of an effort to enable the multi-queue block layer in virtio-blk. The notification BH was not multi-queue friendly. The blk_io_plug() mechanism improves fio rw=randread bs=4k iodepth=64 numjobs=8 IOPS by ~9% with a single IOThread and 8 vCPUs (this is not even a multi-queue block layer configuration) compared to no completion batching. iodepth=1 decreases by ~1% but this could be noise. Benchmark details are available here: https://gitlab.com/stefanha/virt-playbooks/-/tree/blk_io_plug-irqfd Stefan Hajnoczi (4): block: rename blk_io_plug_call() API to defer_call() util/defer-call: move defer_call() to util/ virtio: use defer_call() in virtio_irqfd_notify() virtio-blk: remove batch notification BH MAINTAINERS | 3 +- include/qemu/defer-call.h | 15 +++ include/sysemu/block-backend-io.h | 4 - block/blkio.c | 9 +- block/io_uring.c | 11 ++- block/linux-aio.c | 9 +- block/nvme.c | 5 +- block/plug.c | 159 ------------------------------ hw/block/dataplane/virtio-blk.c | 48 +-------- hw/block/dataplane/xen-block.c | 11 ++- hw/block/virtio-blk.c | 5 +- hw/scsi/virtio-scsi.c | 7 +- hw/virtio/virtio.c | 11 ++- util/defer-call.c | 156 +++++++++++++++++++++++++++++ util/thread-pool.c | 5 + block/meson.build | 1 - util/meson.build | 1 + 17 files changed, 227 insertions(+), 233 deletions(-) create mode 100644 include/qemu/defer-call.h delete mode 100644 block/plug.c create mode 100644 util/defer-call.c -- 2.41.0
There is a batching mechanism for virtio-blk Used Buffer Notifications that is no longer needed because the previous commit added batching to virtio_notify_irqfd(). Note that this mechanism was rarely used in practice because it is only enabled when EVENT_IDX is not negotiated by the driver. Modern drivers enable EVENT_IDX. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/block/dataplane/virtio-blk.c | 48 +-------------------------------- 1 file changed, 1 insertion(+), 47 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 @@ struct VirtIOBlockDataPlane { VirtIOBlkConf *conf; VirtIODevice *vdev; - QEMUBH *bh; /* bh for guest notification */ - unsigned long *batch_notify_vqs; - bool batch_notifications; /* Note that these EventNotifiers are assigned by value. This is * fine as long as you do not call event_notifier_cleanup on them @@ -XXX,XX +XXX,XX @@ struct VirtIOBlockDataPlane { /* Raise an interrupt to signal guest, if necessary */ void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s, VirtQueue *vq) { - if (s->batch_notifications) { - set_bit(virtio_get_queue_index(vq), s->batch_notify_vqs); - qemu_bh_schedule(s->bh); - } else { - virtio_notify_irqfd(s->vdev, vq); - } -} - -static void notify_guest_bh(void *opaque) -{ - VirtIOBlockDataPlane *s = opaque; - unsigned nvqs = s->conf->num_queues; - unsigned long bitmap[BITS_TO_LONGS(nvqs)]; - unsigned j; - - memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap)); - memset(s->batch_notify_vqs, 0, sizeof(bitmap)); - - for (j = 0; j < nvqs; j += BITS_PER_LONG) { - unsigned long bits = bitmap[j / BITS_PER_LONG]; - - while (bits != 0) { - unsigned i = j + ctzl(bits); - VirtQueue *vq = virtio_get_queue(s->vdev, i); - - virtio_notify_irqfd(s->vdev, vq); - - bits &= bits - 1; /* clear right-most bit */ - } - } + virtio_notify_irqfd(s->vdev, vq); } /* Context: QEMU global mutex held */ @@ -XXX,XX +XXX,XX @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, } else { s->ctx = qemu_get_aio_context(); } - s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s, - &DEVICE(vdev)->mem_reentrancy_guard); - s->batch_notify_vqs = bitmap_new(conf->num_queues); *dataplane = s; @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s) vblk = VIRTIO_BLK(s->vdev); assert(!vblk->dataplane_started); - g_free(s->batch_notify_vqs); - qemu_bh_delete(s->bh); if (s->iothread) { object_unref(OBJECT(s->iothread)); } @@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev) s->starting = true; - if (!virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) { - s->batch_notifications = true; - } else { - s->batch_notifications = false; - } - /* Set up guest notifier (irq) */ r = k->set_guest_notifiers(qbus->parent, nvqs, true); if (r != 0) { @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev) aio_context_release(s->ctx); - 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); -- 2.41.0
v3: - Add comment pointing to API documentation in .c file [Philippe] - Add virtio_notify_irqfd_deferred_fn trace event [Ilya] - Remove outdated #include [Ilya] v2: - Rename blk_io_plug() to defer_call() and move it to util/ so the net subsystem can use it [Ilya] - Add defer_call_begin()/end() to thread_pool_completion_bh() to match Linux AIO and io_uring completion batching Replace the seldom-used virtio-blk notification BH mechanism with blk_io_plug(). This is part of an effort to enable the multi-queue block layer in virtio-blk. The notification BH was not multi-queue friendly. The blk_io_plug() mechanism improves fio rw=randread bs=4k iodepth=64 numjobs=8 IOPS by ~9% with a single IOThread and 8 vCPUs (this is not even a multi-queue block layer configuration) compared to no completion batching. iodepth=1 decreases by ~1% but this could be noise. Benchmark details are available here: https://gitlab.com/stefanha/virt-playbooks/-/tree/blk_io_plug-irqfd Stefan Hajnoczi (4): block: rename blk_io_plug_call() API to defer_call() util/defer-call: move defer_call() to util/ virtio: use defer_call() in virtio_irqfd_notify() virtio-blk: remove batch notification BH MAINTAINERS | 3 +- include/qemu/defer-call.h | 16 +++ include/sysemu/block-backend-io.h | 4 - block/blkio.c | 9 +- block/io_uring.c | 11 ++- block/linux-aio.c | 9 +- block/nvme.c | 5 +- block/plug.c | 159 ------------------------------ hw/block/dataplane/virtio-blk.c | 48 +-------- hw/block/dataplane/xen-block.c | 11 ++- hw/block/virtio-blk.c | 5 +- hw/scsi/virtio-scsi.c | 7 +- hw/virtio/virtio.c | 13 ++- util/defer-call.c | 156 +++++++++++++++++++++++++++++ util/thread-pool.c | 5 + block/meson.build | 1 - hw/virtio/trace-events | 1 + util/meson.build | 1 + 18 files changed, 231 insertions(+), 233 deletions(-) create mode 100644 include/qemu/defer-call.h delete mode 100644 block/plug.c create mode 100644 util/defer-call.c -- 2.41.0
Prepare to move the blk_io_plug_call() API out of the block layer so that other subsystems call use this deferred call mechanism. Rename it to defer_call() but leave the code in block/plug.c. The next commit will move the code out of the block layer. Suggested-by: Ilya Maximets <i.maximets@ovn.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Paul Durrant <paul@xen.org> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- include/sysemu/block-backend-io.h | 6 +- block/blkio.c | 8 +-- block/io_uring.c | 4 +- block/linux-aio.c | 4 +- block/nvme.c | 4 +- block/plug.c | 109 +++++++++++++++--------------- hw/block/dataplane/xen-block.c | 10 +-- hw/block/virtio-blk.c | 4 +- hw/scsi/virtio-scsi.c | 6 +- 9 files changed, 76 insertions(+), 79 deletions(-) diff --git a/include/sysemu/block-backend-io.h b/include/sysemu/block-backend-io.h index XXXXXXX..XXXXXXX 100644 --- a/include/sysemu/block-backend-io.h +++ b/include/sysemu/block-backend-io.h @@ -XXX,XX +XXX,XX @@ void blk_iostatus_set_err(BlockBackend *blk, int error); int blk_get_max_iov(BlockBackend *blk); int blk_get_max_hw_iov(BlockBackend *blk); -void blk_io_plug(void); -void blk_io_unplug(void); -void blk_io_plug_call(void (*fn)(void *), void *opaque); +void defer_call_begin(void); +void defer_call_end(void); +void defer_call(void (*fn)(void *), void *opaque); AioContext *blk_get_aio_context(BlockBackend *blk); BlockAcctStats *blk_get_stats(BlockBackend *blk); diff --git a/block/blkio.c b/block/blkio.c index XXXXXXX..XXXXXXX 100644 --- a/block/blkio.c +++ b/block/blkio.c @@ -XXX,XX +XXX,XX @@ static void blkio_detach_aio_context(BlockDriverState *bs) } /* - * Called by blk_io_unplug() or immediately if not plugged. Called without - * blkio_lock. + * Called by defer_call_end() or immediately if not in a deferred section. + * Called without blkio_lock. */ -static void blkio_unplug_fn(void *opaque) +static void blkio_deferred_fn(void *opaque) { BDRVBlkioState *s = opaque; @@ -XXX,XX +XXX,XX @@ static void blkio_submit_io(BlockDriverState *bs) { BDRVBlkioState *s = bs->opaque; - blk_io_plug_call(blkio_unplug_fn, s); + defer_call(blkio_deferred_fn, s); } static int coroutine_fn diff --git a/block/io_uring.c b/block/io_uring.c index XXXXXXX..XXXXXXX 100644 --- a/block/io_uring.c +++ b/block/io_uring.c @@ -XXX,XX +XXX,XX @@ static void ioq_init(LuringQueue *io_q) io_q->blocked = false; } -static void luring_unplug_fn(void *opaque) +static void luring_deferred_fn(void *opaque) { LuringState *s = opaque; trace_luring_unplug_fn(s, s->io_q.blocked, s->io_q.in_queue, @@ -XXX,XX +XXX,XX @@ static int luring_do_submit(int fd, LuringAIOCB *luringcb, LuringState *s, return ret; } - blk_io_plug_call(luring_unplug_fn, s); + defer_call(luring_deferred_fn, s); } return 0; } diff --git a/block/linux-aio.c b/block/linux-aio.c index XXXXXXX..XXXXXXX 100644 --- a/block/linux-aio.c +++ b/block/linux-aio.c @@ -XXX,XX +XXX,XX @@ static uint64_t laio_max_batch(LinuxAioState *s, uint64_t dev_max_batch) return max_batch; } -static void laio_unplug_fn(void *opaque) +static void laio_deferred_fn(void *opaque) { LinuxAioState *s = opaque; @@ -XXX,XX +XXX,XX @@ static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset, if (s->io_q.in_queue >= laio_max_batch(s, dev_max_batch)) { ioq_submit(s); } else { - blk_io_plug_call(laio_unplug_fn, s); + defer_call(laio_deferred_fn, s); } } diff --git a/block/nvme.c b/block/nvme.c index XXXXXXX..XXXXXXX 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -XXX,XX +XXX,XX @@ static void nvme_trace_command(const NvmeCmd *cmd) } } -static void nvme_unplug_fn(void *opaque) +static void nvme_deferred_fn(void *opaque) { NVMeQueuePair *q = opaque; @@ -XXX,XX +XXX,XX @@ static void nvme_submit_command(NVMeQueuePair *q, NVMeRequest *req, q->need_kick++; qemu_mutex_unlock(&q->lock); - blk_io_plug_call(nvme_unplug_fn, q); + defer_call(nvme_deferred_fn, q); } static void nvme_admin_cmd_sync_cb(void *opaque, int ret) diff --git a/block/plug.c b/block/plug.c index XXXXXXX..XXXXXXX 100644 --- a/block/plug.c +++ b/block/plug.c @@ -XXX,XX +XXX,XX @@ /* SPDX-License-Identifier: GPL-2.0-or-later */ /* - * Block I/O plugging + * Deferred calls * * Copyright Red Hat. * - * This API defers a function call within a blk_io_plug()/blk_io_unplug() + * This API defers a function call within a defer_call_begin()/defer_call_end() * section, allowing multiple calls to batch up. This is a performance * optimization that is used in the block layer to submit several I/O requests * at once instead of individually: * - * blk_io_plug(); <-- start of plugged region + * defer_call_begin(); <-- start of section * ... - * blk_io_plug_call(my_func, my_obj); <-- deferred my_func(my_obj) call - * blk_io_plug_call(my_func, my_obj); <-- another - * blk_io_plug_call(my_func, my_obj); <-- another + * defer_call(my_func, my_obj); <-- deferred my_func(my_obj) call + * defer_call(my_func, my_obj); <-- another + * defer_call(my_func, my_obj); <-- another * ... - * blk_io_unplug(); <-- end of plugged region, my_func(my_obj) is called once - * - * This code is actually generic and not tied to the block layer. If another - * subsystem needs this functionality, it could be renamed. + * defer_call_end(); <-- end of section, my_func(my_obj) is called once */ #include "qemu/osdep.h" @@ -XXX,XX +XXX,XX @@ #include "qemu/thread.h" #include "sysemu/block-backend.h" -/* A function call that has been deferred until unplug() */ +/* A function call that has been deferred until defer_call_end() */ typedef struct { void (*fn)(void *); void *opaque; -} UnplugFn; +} DeferredCall; /* Per-thread state */ typedef struct { - unsigned count; /* how many times has plug() been called? */ - GArray *unplug_fns; /* functions to call at unplug time */ -} Plug; + unsigned nesting_level; + GArray *deferred_call_array; +} DeferCallThreadState; -/* Use get_ptr_plug() to fetch this thread-local value */ -QEMU_DEFINE_STATIC_CO_TLS(Plug, plug); +/* Use get_ptr_defer_call_thread_state() to fetch this thread-local value */ +QEMU_DEFINE_STATIC_CO_TLS(DeferCallThreadState, defer_call_thread_state); /* Called at thread cleanup time */ -static void blk_io_plug_atexit(Notifier *n, void *value) +static void defer_call_atexit(Notifier *n, void *value) { - Plug *plug = get_ptr_plug(); - g_array_free(plug->unplug_fns, TRUE); + DeferCallThreadState *thread_state = get_ptr_defer_call_thread_state(); + g_array_free(thread_state->deferred_call_array, TRUE); } /* This won't involve coroutines, so use __thread */ -static __thread Notifier blk_io_plug_atexit_notifier; +static __thread Notifier defer_call_atexit_notifier; /** - * blk_io_plug_call: + * defer_call: * @fn: a function pointer to be invoked * @opaque: a user-defined argument to @fn() * - * Call @fn(@opaque) immediately if not within a blk_io_plug()/blk_io_unplug() - * section. + * Call @fn(@opaque) immediately if not within a + * defer_call_begin()/defer_call_end() section. * * Otherwise defer the call until the end of the outermost - * blk_io_plug()/blk_io_unplug() section in this thread. If the same + * defer_call_begin()/defer_call_end() section in this thread. If the same * @fn/@opaque pair has already been deferred, it will only be called once upon - * blk_io_unplug() so that accumulated calls are batched into a single call. + * defer_call_end() so that accumulated calls are batched into a single call. * * The caller must ensure that @opaque is not freed before @fn() is invoked. */ -void blk_io_plug_call(void (*fn)(void *), void *opaque) +void defer_call(void (*fn)(void *), void *opaque) { - Plug *plug = get_ptr_plug(); + DeferCallThreadState *thread_state = get_ptr_defer_call_thread_state(); - /* Call immediately if we're not plugged */ - if (plug->count == 0) { + /* Call immediately if we're not deferring calls */ + if (thread_state->nesting_level == 0) { fn(opaque); return; } - GArray *array = plug->unplug_fns; + GArray *array = thread_state->deferred_call_array; if (!array) { - array = g_array_new(FALSE, FALSE, sizeof(UnplugFn)); - plug->unplug_fns = array; - blk_io_plug_atexit_notifier.notify = blk_io_plug_atexit; - qemu_thread_atexit_add(&blk_io_plug_atexit_notifier); + array = g_array_new(FALSE, FALSE, sizeof(DeferredCall)); + thread_state->deferred_call_array = array; + defer_call_atexit_notifier.notify = defer_call_atexit; + qemu_thread_atexit_add(&defer_call_atexit_notifier); } - UnplugFn *fns = (UnplugFn *)array->data; - UnplugFn new_fn = { + DeferredCall *fns = (DeferredCall *)array->data; + DeferredCall new_fn = { .fn = fn, .opaque = opaque, }; @@ -XXX,XX +XXX,XX @@ void blk_io_plug_call(void (*fn)(void *), void *opaque) } /** - * blk_io_plug: Defer blk_io_plug_call() functions until blk_io_unplug() + * defer_call_begin: Defer defer_call() functions until defer_call_end() * - * blk_io_plug/unplug are thread-local operations. This means that multiple - * threads can simultaneously call plug/unplug, but the caller must ensure that - * each unplug() is called in the same thread of the matching plug(). + * defer_call_begin() and defer_call_end() are thread-local operations. The + * caller must ensure that each defer_call_begin() has a matching + * defer_call_end() in the same thread. * - * Nesting is supported. blk_io_plug_call() functions are only called at the - * outermost blk_io_unplug(). + * Nesting is supported. defer_call() functions are only called at the + * outermost defer_call_end(). */ -void blk_io_plug(void) +void defer_call_begin(void) { - Plug *plug = get_ptr_plug(); + DeferCallThreadState *thread_state = get_ptr_defer_call_thread_state(); - assert(plug->count < UINT32_MAX); + assert(thread_state->nesting_level < UINT32_MAX); - plug->count++; + thread_state->nesting_level++; } /** - * blk_io_unplug: Run any pending blk_io_plug_call() functions + * defer_call_end: Run any pending defer_call() functions * - * There must have been a matching blk_io_plug() call in the same thread prior - * to this blk_io_unplug() call. + * There must have been a matching defer_call_begin() call in the same thread + * prior to this defer_call_end() call. */ -void blk_io_unplug(void) +void defer_call_end(void) { - Plug *plug = get_ptr_plug(); + DeferCallThreadState *thread_state = get_ptr_defer_call_thread_state(); - assert(plug->count > 0); + assert(thread_state->nesting_level > 0); - if (--plug->count > 0) { + if (--thread_state->nesting_level > 0) { return; } - GArray *array = plug->unplug_fns; + GArray *array = thread_state->deferred_call_array; if (!array) { return; } - UnplugFn *fns = (UnplugFn *)array->data; + DeferredCall *fns = (DeferredCall *)array->data; for (guint i = 0; i < array->len; i++) { fns[i].fn(fns[i].opaque); diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/dataplane/xen-block.c +++ b/hw/block/dataplane/xen-block.c @@ -XXX,XX +XXX,XX @@ static int xen_block_get_request(XenBlockDataPlane *dataplane, /* * Threshold of in-flight requests above which we will start using - * blk_io_plug()/blk_io_unplug() to batch requests. + * defer_call_begin()/defer_call_end() to batch requests. */ #define IO_PLUG_THRESHOLD 1 @@ -XXX,XX +XXX,XX @@ static bool xen_block_handle_requests(XenBlockDataPlane *dataplane) * is below us. */ if (inflight_atstart > IO_PLUG_THRESHOLD) { - blk_io_plug(); + defer_call_begin(); } while (rc != rp) { /* pull request from ring */ @@ -XXX,XX +XXX,XX @@ static bool xen_block_handle_requests(XenBlockDataPlane *dataplane) if (inflight_atstart > IO_PLUG_THRESHOLD && batched >= inflight_atstart) { - blk_io_unplug(); + defer_call_end(); } xen_block_do_aio(request); if (inflight_atstart > IO_PLUG_THRESHOLD) { if (batched >= inflight_atstart) { - blk_io_plug(); + defer_call_begin(); batched = 0; } else { batched++; @@ -XXX,XX +XXX,XX @@ static bool xen_block_handle_requests(XenBlockDataPlane *dataplane) } } if (inflight_atstart > IO_PLUG_THRESHOLD) { - blk_io_unplug(); + defer_call_end(); } return done_something; diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -XXX,XX +XXX,XX @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq) bool suppress_notifications = virtio_queue_get_notification(vq); aio_context_acquire(blk_get_aio_context(s->blk)); - blk_io_plug(); + defer_call_begin(); do { if (suppress_notifications) { @@ -XXX,XX +XXX,XX @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq) virtio_blk_submit_multireq(s, &mrb); } - blk_io_unplug(); + defer_call_end(); aio_context_release(blk_get_aio_context(s->blk)); } diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -XXX,XX +XXX,XX @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req) return -ENOBUFS; } scsi_req_ref(req->sreq); - blk_io_plug(); + defer_call_begin(); object_unref(OBJECT(d)); return 0; } @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req) if (scsi_req_enqueue(sreq)) { scsi_req_continue(sreq); } - blk_io_unplug(); + defer_call_end(); scsi_req_unref(sreq); } @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq) while (!QTAILQ_EMPTY(&reqs)) { req = QTAILQ_FIRST(&reqs); QTAILQ_REMOVE(&reqs, req, next); - blk_io_unplug(); + defer_call_end(); scsi_req_unref(req->sreq); virtqueue_detach_element(req->vq, &req->elem, 0); virtio_scsi_free_req(req); -- 2.41.0
The networking subsystem may wish to use defer_call(), so move the code to util/ where it can be reused. As a reminder of what defer_call() does: This API defers a function call within a defer_call_begin()/defer_call_end() section, allowing multiple calls to batch up. This is a performance optimization that is used in the block layer to submit several I/O requests at once instead of individually: defer_call_begin(); <-- start of section ... defer_call(my_func, my_obj); <-- deferred my_func(my_obj) call defer_call(my_func, my_obj); <-- another defer_call(my_func, my_obj); <-- another ... defer_call_end(); <-- end of section, my_func(my_obj) is called once Suggested-by: Ilya Maximets <i.maximets@ovn.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- MAINTAINERS | 3 ++- include/qemu/defer-call.h | 16 ++++++++++++++++ include/sysemu/block-backend-io.h | 4 ---- block/blkio.c | 1 + block/io_uring.c | 1 + block/linux-aio.c | 1 + block/nvme.c | 1 + hw/block/dataplane/xen-block.c | 1 + hw/block/virtio-blk.c | 1 + hw/scsi/virtio-scsi.c | 1 + block/plug.c => util/defer-call.c | 2 +- block/meson.build | 1 - util/meson.build | 1 + 13 files changed, 27 insertions(+), 7 deletions(-) create mode 100644 include/qemu/defer-call.h rename block/plug.c => util/defer-call.c (99%) diff --git a/MAINTAINERS b/MAINTAINERS index XXXXXXX..XXXXXXX 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -XXX,XX +XXX,XX @@ S: Supported F: util/async.c F: util/aio-*.c F: util/aio-*.h +F: util/defer-call.c F: util/fdmon-*.c F: block/io.c -F: block/plug.c F: migration/block* F: include/block/aio.h F: include/block/aio-wait.h +F: include/qemu/defer-call.h F: scripts/qemugdb/aio.py F: tests/unit/test-fdmon-epoll.c T: git https://github.com/stefanha/qemu.git block diff --git a/include/qemu/defer-call.h b/include/qemu/defer-call.h new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/include/qemu/defer-call.h @@ -XXX,XX +XXX,XX @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Deferred calls + * + * Copyright Red Hat. + */ + +#ifndef QEMU_DEFER_CALL_H +#define QEMU_DEFER_CALL_H + +/* See documentation in util/defer-call.c */ +void defer_call_begin(void); +void defer_call_end(void); +void defer_call(void (*fn)(void *), void *opaque); + +#endif /* QEMU_DEFER_CALL_H */ diff --git a/include/sysemu/block-backend-io.h b/include/sysemu/block-backend-io.h index XXXXXXX..XXXXXXX 100644 --- a/include/sysemu/block-backend-io.h +++ b/include/sysemu/block-backend-io.h @@ -XXX,XX +XXX,XX @@ void blk_iostatus_set_err(BlockBackend *blk, int error); int blk_get_max_iov(BlockBackend *blk); int blk_get_max_hw_iov(BlockBackend *blk); -void defer_call_begin(void); -void defer_call_end(void); -void defer_call(void (*fn)(void *), void *opaque); - AioContext *blk_get_aio_context(BlockBackend *blk); BlockAcctStats *blk_get_stats(BlockBackend *blk); void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk, diff --git a/block/blkio.c b/block/blkio.c index XXXXXXX..XXXXXXX 100644 --- a/block/blkio.c +++ b/block/blkio.c @@ -XXX,XX +XXX,XX @@ #include "block/block_int.h" #include "exec/memory.h" #include "exec/cpu-common.h" /* for qemu_ram_get_fd() */ +#include "qemu/defer-call.h" #include "qapi/error.h" #include "qemu/error-report.h" #include "qapi/qmp/qdict.h" diff --git a/block/io_uring.c b/block/io_uring.c index XXXXXXX..XXXXXXX 100644 --- a/block/io_uring.c +++ b/block/io_uring.c @@ -XXX,XX +XXX,XX @@ #include "block/block.h" #include "block/raw-aio.h" #include "qemu/coroutine.h" +#include "qemu/defer-call.h" #include "qapi/error.h" #include "sysemu/block-backend.h" #include "trace.h" diff --git a/block/linux-aio.c b/block/linux-aio.c index XXXXXXX..XXXXXXX 100644 --- a/block/linux-aio.c +++ b/block/linux-aio.c @@ -XXX,XX +XXX,XX @@ #include "block/raw-aio.h" #include "qemu/event_notifier.h" #include "qemu/coroutine.h" +#include "qemu/defer-call.h" #include "qapi/error.h" #include "sysemu/block-backend.h" diff --git a/block/nvme.c b/block/nvme.c index XXXXXXX..XXXXXXX 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -XXX,XX +XXX,XX @@ #include "qapi/error.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qstring.h" +#include "qemu/defer-call.h" #include "qemu/error-report.h" #include "qemu/main-loop.h" #include "qemu/module.h" diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/dataplane/xen-block.c +++ b/hw/block/dataplane/xen-block.c @@ -XXX,XX +XXX,XX @@ */ #include "qemu/osdep.h" +#include "qemu/defer-call.h" #include "qemu/error-report.h" #include "qemu/main-loop.h" #include "qemu/memalign.h" diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -XXX,XX +XXX,XX @@ */ #include "qemu/osdep.h" +#include "qemu/defer-call.h" #include "qapi/error.h" #include "qemu/iov.h" #include "qemu/module.h" diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -XXX,XX +XXX,XX @@ #include "standard-headers/linux/virtio_ids.h" #include "hw/virtio/virtio-scsi.h" #include "migration/qemu-file-types.h" +#include "qemu/defer-call.h" #include "qemu/error-report.h" #include "qemu/iov.h" #include "qemu/module.h" diff --git a/block/plug.c b/util/defer-call.c similarity index 99% rename from block/plug.c rename to util/defer-call.c index XXXXXXX..XXXXXXX 100644 --- a/block/plug.c +++ b/util/defer-call.c @@ -XXX,XX +XXX,XX @@ #include "qemu/coroutine-tls.h" #include "qemu/notify.h" #include "qemu/thread.h" -#include "sysemu/block-backend.h" +#include "qemu/defer-call.h" /* A function call that has been deferred until defer_call_end() */ typedef struct { diff --git a/block/meson.build b/block/meson.build index XXXXXXX..XXXXXXX 100644 --- a/block/meson.build +++ b/block/meson.build @@ -XXX,XX +XXX,XX @@ block_ss.add(files( 'mirror.c', 'nbd.c', 'null.c', - 'plug.c', 'preallocate.c', 'progress_meter.c', 'qapi.c', diff --git a/util/meson.build b/util/meson.build index XXXXXXX..XXXXXXX 100644 --- a/util/meson.build +++ b/util/meson.build @@ -XXX,XX +XXX,XX @@ util_ss.add(when: 'CONFIG_WIN32', if_true: pathcch) if glib_has_gslice util_ss.add(files('qtree.c')) endif +util_ss.add(files('defer-call.c')) util_ss.add(files('envlist.c', 'path.c', 'module.c')) util_ss.add(files('host-utils.c')) util_ss.add(files('bitmap.c', 'bitops.c')) -- 2.41.0
virtio-blk and virtio-scsi invoke virtio_irqfd_notify() to send Used Buffer Notifications from an IOThread. This involves an eventfd write(2) syscall. Calling this repeatedly when completing multiple I/O requests in a row is wasteful. Use the defer_call() API to batch together virtio_irqfd_notify() calls made during thread pool (aio=threads), Linux AIO (aio=native), and io_uring (aio=io_uring) completion processing. Behavior is unchanged for emulated devices that do not use defer_call_begin()/defer_call_end() since defer_call() immediately invokes the callback when called outside a defer_call_begin()/defer_call_end() region. fio rw=randread bs=4k iodepth=64 numjobs=8 IOPS increases by ~9% with a single IOThread and 8 vCPUs. iodepth=1 decreases by ~1% but this could be noise. Detailed performance data and configuration specifics are available here: https://gitlab.com/stefanha/virt-playbooks/-/tree/blk_io_plug-irqfd This duplicates the BH that virtio-blk uses for batching. The next commit will remove it. Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/io_uring.c | 6 ++++++ block/linux-aio.c | 4 ++++ hw/virtio/virtio.c | 13 ++++++++++++- util/thread-pool.c | 5 +++++ hw/virtio/trace-events | 1 + 5 files changed, 28 insertions(+), 1 deletion(-) diff --git a/block/io_uring.c b/block/io_uring.c index XXXXXXX..XXXXXXX 100644 --- a/block/io_uring.c +++ b/block/io_uring.c @@ -XXX,XX +XXX,XX @@ static void luring_process_completions(LuringState *s) { struct io_uring_cqe *cqes; int total_bytes; + + defer_call_begin(); + /* * Request completion callbacks can run the nested event loop. * Schedule ourselves so the nested event loop will "see" remaining @@ -XXX,XX +XXX,XX @@ end: aio_co_wake(luringcb->co); } } + qemu_bh_cancel(s->completion_bh); + + defer_call_end(); } static int ioq_submit(LuringState *s) diff --git a/block/linux-aio.c b/block/linux-aio.c index XXXXXXX..XXXXXXX 100644 --- a/block/linux-aio.c +++ b/block/linux-aio.c @@ -XXX,XX +XXX,XX @@ static void qemu_laio_process_completions(LinuxAioState *s) { struct io_event *events; + defer_call_begin(); + /* Reschedule so nested event loops see currently pending completions */ qemu_bh_schedule(s->completion_bh); @@ -XXX,XX +XXX,XX @@ static void qemu_laio_process_completions(LinuxAioState *s) * own `for` loop. If we are the last all counters dropped to zero. */ s->event_max = 0; s->event_idx = 0; + + defer_call_end(); } static void qemu_laio_process_completions_and_submit(LinuxAioState *s) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index XXXXXXX..XXXXXXX 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -XXX,XX +XXX,XX @@ #include "qapi/error.h" #include "qapi/qapi-commands-virtio.h" #include "trace.h" +#include "qemu/defer-call.h" #include "qemu/error-report.h" #include "qemu/log.h" #include "qemu/main-loop.h" @@ -XXX,XX +XXX,XX @@ static bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq) } } +/* Batch irqs while inside a defer_call_begin()/defer_call_end() section */ +static void virtio_notify_irqfd_deferred_fn(void *opaque) +{ + EventNotifier *notifier = opaque; + VirtQueue *vq = container_of(notifier, VirtQueue, guest_notifier); + + trace_virtio_notify_irqfd_deferred_fn(vq->vdev, vq); + event_notifier_set(notifier); +} + void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq) { WITH_RCU_READ_LOCK_GUARD() { @@ -XXX,XX +XXX,XX @@ void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq) * to an atomic operation. */ virtio_set_isr(vq->vdev, 0x1); - event_notifier_set(&vq->guest_notifier); + defer_call(virtio_notify_irqfd_deferred_fn, &vq->guest_notifier); } static void virtio_irq(VirtQueue *vq) diff --git a/util/thread-pool.c b/util/thread-pool.c index XXXXXXX..XXXXXXX 100644 --- a/util/thread-pool.c +++ b/util/thread-pool.c @@ -XXX,XX +XXX,XX @@ * GNU GPL, version 2 or (at your option) any later version. */ #include "qemu/osdep.h" +#include "qemu/defer-call.h" #include "qemu/queue.h" #include "qemu/thread.h" #include "qemu/coroutine.h" @@ -XXX,XX +XXX,XX @@ static void thread_pool_completion_bh(void *opaque) ThreadPool *pool = opaque; ThreadPoolElement *elem, *next; + defer_call_begin(); /* cb() may use defer_call() to coalesce work */ + restart: QLIST_FOREACH_SAFE(elem, &pool->head, all, next) { if (elem->state != THREAD_DONE) { @@ -XXX,XX +XXX,XX @@ restart: qemu_aio_unref(elem); } } + + defer_call_end(); } static void thread_pool_cancel(BlockAIOCB *acb) diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events index XXXXXXX..XXXXXXX 100644 --- a/hw/virtio/trace-events +++ b/hw/virtio/trace-events @@ -XXX,XX +XXX,XX @@ virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx) " virtqueue_flush(void *vq, unsigned int count) "vq %p count %u" virtqueue_pop(void *vq, void *elem, unsigned int in_num, unsigned int out_num) "vq %p elem %p in_num %u out_num %u" virtio_queue_notify(void *vdev, int n, void *vq) "vdev %p n %d vq %p" +virtio_notify_irqfd_deferred_fn(void *vdev, void *vq) "vdev %p vq %p" virtio_notify_irqfd(void *vdev, void *vq) "vdev %p vq %p" virtio_notify(void *vdev, void *vq) "vdev %p vq %p" virtio_set_status(void *vdev, uint8_t val) "vdev %p val %u" -- 2.41.0
There is a batching mechanism for virtio-blk Used Buffer Notifications that is no longer needed because the previous commit added batching to virtio_notify_irqfd(). Note that this mechanism was rarely used in practice because it is only enabled when EVENT_IDX is not negotiated by the driver. Modern drivers enable EVENT_IDX. Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/block/dataplane/virtio-blk.c | 48 +-------------------------------- 1 file changed, 1 insertion(+), 47 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 @@ struct VirtIOBlockDataPlane { VirtIOBlkConf *conf; VirtIODevice *vdev; - QEMUBH *bh; /* bh for guest notification */ - unsigned long *batch_notify_vqs; - bool batch_notifications; /* Note that these EventNotifiers are assigned by value. This is * fine as long as you do not call event_notifier_cleanup on them @@ -XXX,XX +XXX,XX @@ struct VirtIOBlockDataPlane { /* Raise an interrupt to signal guest, if necessary */ void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s, VirtQueue *vq) { - if (s->batch_notifications) { - set_bit(virtio_get_queue_index(vq), s->batch_notify_vqs); - qemu_bh_schedule(s->bh); - } else { - virtio_notify_irqfd(s->vdev, vq); - } -} - -static void notify_guest_bh(void *opaque) -{ - VirtIOBlockDataPlane *s = opaque; - unsigned nvqs = s->conf->num_queues; - unsigned long bitmap[BITS_TO_LONGS(nvqs)]; - unsigned j; - - memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap)); - memset(s->batch_notify_vqs, 0, sizeof(bitmap)); - - for (j = 0; j < nvqs; j += BITS_PER_LONG) { - unsigned long bits = bitmap[j / BITS_PER_LONG]; - - while (bits != 0) { - unsigned i = j + ctzl(bits); - VirtQueue *vq = virtio_get_queue(s->vdev, i); - - virtio_notify_irqfd(s->vdev, vq); - - bits &= bits - 1; /* clear right-most bit */ - } - } + virtio_notify_irqfd(s->vdev, vq); } /* Context: QEMU global mutex held */ @@ -XXX,XX +XXX,XX @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, } else { s->ctx = qemu_get_aio_context(); } - s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s, - &DEVICE(vdev)->mem_reentrancy_guard); - s->batch_notify_vqs = bitmap_new(conf->num_queues); *dataplane = s; @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s) vblk = VIRTIO_BLK(s->vdev); assert(!vblk->dataplane_started); - g_free(s->batch_notify_vqs); - qemu_bh_delete(s->bh); if (s->iothread) { object_unref(OBJECT(s->iothread)); } @@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev) s->starting = true; - if (!virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) { - s->batch_notifications = true; - } else { - s->batch_notifications = false; - } - /* Set up guest notifier (irq) */ r = k->set_guest_notifiers(qbus->parent, nvqs, true); if (r != 0) { @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev) aio_context_release(s->ctx); - 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); -- 2.41.0