From 56e64ddcc982c788e8cb69e0ff6c449b42851055 Mon Sep 17 00:00:00 2001 From: Waldemar Kozaczuk Date: Sun, 2 Aug 2026 00:32:06 -0400 Subject: [PATCH] virtio-blk: handle completions on the queue's dedicated thread This changes virtio-blk driver to create as many completion threads as there are virt queues. So if there are more than single queue, interrupts delivered are serviced by its own dedicated queue instead of a single shared one. The idea is to minimize number of IPIs because hopefully a submission thread would be woken by a completion thread running on the same cpu. This also reverts some of the changes made to have single completion thread handle all queues. Finally, this modifies `run.py` to explicitly set number of virt queues to 1 as advertised instead of defulting to number of cpus. --- documentation/block-multiqueue.md | 64 +++++----- drivers/virtio-blk.cc | 190 ++++++++++++------------------ drivers/virtio-blk.hh | 9 +- scripts/run.py | 7 +- 4 files changed, 108 insertions(+), 162 deletions(-) diff --git a/documentation/block-multiqueue.md b/documentation/block-multiqueue.md index 55cdef1ce..5d3c6422b 100644 --- a/documentation/block-multiqueue.md +++ b/documentation/block-multiqueue.md @@ -3,13 +3,13 @@ ## Overview The virtio-blk driver can negotiate `VIRTIO_BLK_F_MQ` with the host and -operate multiple virtqueues, one selected per submitting CPU. This lets +operate multiple virtqueues, one selected per submitting CPU. This lets several vCPUs submit block I/O in parallel against independent rings instead of serialising on a single queue lock. The implementation lives entirely in `drivers/virtio-blk.cc` and `drivers/virtio-blk.hh`; there is no separate generic block-multiqueue -framework. Each queue is an ordinary virtio virtqueue guarded by its own +framework. Each queue is an ordinary virtio virtqueue guarded by its own mutex. ## Queue assignment @@ -28,13 +28,13 @@ on the common path. When the host offers `VIRTIO_BLK_F_MQ`, the driver reads `num_queues` from the device config and `probe_virt_queues()` sets up that many -virtqueues. The probed count is authoritative: a per-queue lock vector -(`_queue_locks`) is sized to match, and a single completion thread services -all of them. +virtqueues. The probed count is authoritative: a per-queue lock vector +(`_queue_locks`) is sized to match, and each queue is serviced by its +dedicated completion thread. ```c probe_virt_queues(); -_num_queues = virtio_driver::_num_queues; +_num_queues = std::min(virtio_driver::_num_queues, sched::cpus.size()); ... _queue_locks = std::vector(_num_queues); ``` @@ -59,42 +59,34 @@ mapped to the same queue (more vCPUs than queues) share that queue's lock. With MSI-X the transport assigns one interrupt vector per virtqueue (`setup_queue()` maps queue index i to MSI-X entry i), so a completion on any -queue raises that queue's own vector. We register a per-queue ISR for every -vector; each ISR disables its own queue's interrupts and wakes a single shared -completion thread. That thread sleeps until any queue's used ring has pending -completions (re-arming interrupts on every queue and re-checking before it +queue raises that queue's own vector. We register a per-queue ISR for every +vector; each ISR disables its own queue's interrupts and wakes its dedicated +completion thread. That thread sleeps until its queue's used ring has pending +completions (re-arming interrupts on thus queue and re-checking before it sleeps, so a completion arriving during the check is never missed), and when -woken it drains every queue, taking each queue's lock so it cannot race with -that queue's owner: +woken it drains this queue, without having to take the queue's lock because +it races only on a lock-less `_used_ring_host_head` counter: ```c -for (int q = 0; q < _num_queues; q++) { - WITH_LOCK(_queue_locks[q]) { - drain_queue(get_virt_queue(q)); - get_virt_queue(q)->wakeup_waiter(); +void blk::req_done(int qid) +{ + auto* queue = get_virt_queue(qid); + blk_req* req; + + while (1) { + virtio_driver::wait_for_queue(queue, &vring::used_ring_not_empty); + + .. + // wake up the requesting thread in case the ring was full before + queue->wakeup_waiter(); } } ``` -So all queues (vrings) and their MSI-X vectors are fully used; the single -consumer thread is only the *completion-side* fan-in, not a per-queue -bottleneck (submission is lock-per-queue and runs on the owning CPU). On the -non-MSI-X / MMIO path a single shared IRQ line fans into the same all-queue -drain. - -The wake predicate checks every queue's used ring and re-arms interrupts on -all queues when none have work, so a completion landing on any queue wakes -the single thread. - -### Known limitation - -Because only one MSI/legacy interrupt vector is wired, completions for all -queues are processed from the CPU that takes the interrupt (typically -CPU 0), which then reaches across queues on the owners' behalf. A future -change can register a per-queue interrupt (as the NVMe driver does via -`driver::register_io_interrupt()`), so each queue's completions land on -its owning CPU and the cross-queue drain loop can be dropped. The -submission path already scales; only completion handling is centralised. +So all queues (vrings) and their MSI-X vectors are fully used; each queue +is serviced by its dedicated consumer thread, submission is lock-per-queue +and runs on the owning CPU). On the non-MSI-X / MMIO path a single shared +IRQ line fans into the same single-queue drain. ## Configuration @@ -111,7 +103,7 @@ qemu-system-x86_64 \ `scripts/run.py` exposes this through the `--virtio-blk-queues` option so test images can request multiple queues without hand-editing the QEMU -command line. With one vCPU or one queue the driver behaves exactly as the +command line. With one vCPU or one queue the driver behaves exactly as the original single-queue path. ## Testing diff --git a/drivers/virtio-blk.cc b/drivers/virtio-blk.cc index af3324d9a..a03d4e46b 100644 --- a/drivers/virtio-blk.cc +++ b/drivers/virtio-blk.cc @@ -132,48 +132,64 @@ blk::blk(virtio_device& virtio_dev) // read_config() recorded the device's advertised num_queues, but // probe_virt_queues() determines how many virtqueues actually exist and - // stores that in the base class _num_queues. Use the probed count as the + // stores that in the base class _num_queues. + // Use the minimum of probed count and number of cpus as the // authoritative value so make_request()/req_done() never index a queue the - // base class did not set up. - _num_queues = virtio_driver::_num_queues; + // base class did not set up. Also, there is no point of creating more queues + // than number of cpus, as make_request() would never submit requests there. + _num_queues = std::min(virtio_driver::_num_queues, sched::cpus.size()); - // Resize per-queue lock vector now that _num_queues is known. + // Resize per-queue lock vector and create as many threads + // now that _num_queues is known. _queue_locks = std::vector(_num_queues); + auto threads = std::vector(_num_queues); // Enable indirect descriptors on every queue so large multi-segment // requests use a single descriptor entry on whichever queue // make_request() selects. + bool multi_queue = _num_queues > 1; for (int qid = 0; qid < _num_queues; qid++) { get_virt_queue(qid)->set_use_indirect(true); + auto *t = sched::thread::make( + [this,qid] { this->req_done(qid); }, + sched::thread::attr().name("virtio-blk" + (_num_queues > 1 ? std::to_string(qid) : "") + "_req_done")); + t->start(); + // It only makes sense and is actuall necessary to pin the consumer + // threads when there are more than one queue so that each is serviced + // on different cpu to make it thread-safe + if (multi_queue) { + auto cpu = sched::cpus[qid % sched::cpus.size()]; + sched::thread::pin(t, cpu); + } + threads[qid] = t; } - - // One completion thread services all virtqueues: it wakes when any queue's - // MSI-X vector fires, then drains every queue. - sched::thread* t = sched::thread::make( - [this] { this->req_done(); }, - sched::thread::attr().name("virtio-blk")); - t->start(); + // + // Use 1st thread when only single queue is available + auto single_thread = threads[0]; // With VIRTIO_BLK_F_MQ, setup_queue() maps queue index i -> MSI-X entry i // (1:1), so a completion on queue i raises entry i's interrupt, not queue // 0's; register an ISR for every queue's vector or completions on queues - // 1..N-1 are never serviced and I/O steered there hangs. Each ISR disables - // its own queue's interrupts and wakes the single completion thread, which - // then drains all queues. (The config-change MSI-X vector is left at + // 1..N-1 are never serviced and I/O steered there hangs. Each ISR disables + // its own queue's interrupts and wakes its dedicated completion thread, which + // then drains the queue. (The config-change MSI-X vector is left at // VIRTIO_MSI_NO_VECTOR, as it is for all OSv virtio devices - virtio-blk // reads capacity via the config space, not a config-change interrupt.) interrupt_factory int_factory; #if CONF_drivers_pci - int_factory.register_msi_bindings = [this, t](interrupt_manager &msi) { + int_factory.register_msi_bindings = [this, threads](interrupt_manager &msi) { std::vector bindings; bindings.reserve(_num_queues); for (int qid = 0; qid < _num_queues; qid++) { + // Use a dedicated thread to be woken up when + // an interrupt for the relevant queue is delivered auto* q = get_virt_queue(qid); - bindings.push_back({ (unsigned)qid, [q] { q->disable_interrupts(); }, t }); + auto* t = threads[qid]; + bindings.push_back({ (unsigned)qid, [q,t] { q->disable_interrupts(); }, t }); } // easy_register() needs one MSI-X vector per binding; if the device // advertised more queues than it has MSI-X entries it returns false - // having registered NONE, which would silently hang all I/O. Fail + // having registered NONE, which would silently hang all I/O. Fail // loudly at probe instead - a device that negotiates F_MQ is expected // to expose at least num_queues (+1 config) vectors. if (!msi.easy_register(bindings)) { @@ -183,28 +199,28 @@ blk::blk(virtio_device& virtio_dev) } }; - int_factory.create_pci_interrupt = [this,t](pci::device &pci_dev) { + int_factory.create_pci_interrupt = [this,single_thread](pci::device &pci_dev) { return new pci_interrupt( pci_dev, [=] { return this->ack_irq(); }, - [=] { t->wake_with_irq_disabled(); }); + [=] { single_thread->wake_with_irq_disabled(); }); }; #endif #if CONF_drivers_mmio #ifdef __aarch64__ - int_factory.create_spi_edge_interrupt = [this,t]() { + int_factory.create_spi_edge_interrupt = [this,single_thread]() { return new spi_interrupt( gic::irq_type::IRQ_TYPE_EDGE, _dev.get_irq(), [=] { return this->ack_irq(); }, - [=] { t->wake_with_irq_disabled(); }); + [=] { single_thread->wake_with_irq_disabled(); }); }; #else - int_factory.create_gsi_edge_interrupt = [this,t]() { + int_factory.create_gsi_edge_interrupt = [this,single_thread]() { return new gsi_edge_interrupt( _dev.get_irq(), - [=] { if (this->ack_irq()) t->wake_with_irq_disabled(); }); + [=] { if (this->ack_irq()) single_thread->wake_with_irq_disabled(); }); }; #endif #endif @@ -225,7 +241,8 @@ blk::blk(virtio_device& virtio_dev) dev->max_io_size = _config.seg_max ? (_config.seg_max - 1) * mmu::page_size : UINT_MAX; read_partition_table(dev); - debugf("virtio-blk: Add blk device instances %d as %s, devsize=%lld\n", _id, dev_name.c_str(), dev->size); + debugf("virtio-blk: Add blk device instances %d as %s with %d queue%s, devsize=%lld\n", + _id, dev_name.c_str(), _num_queues, (_num_queues > 1 ? "s" : ""), dev->size); } blk::~blk() @@ -285,103 +302,50 @@ void blk::read_config() } /* - * drain_queue() — pull all completed requests off one virtqueue ring. - * Returns the number of completions processed. - */ -int blk::drain_queue(vring* queue) -{ - int n = 0; - u32 len; - blk_req* req; - - while ((req = static_cast(queue->get_buf_elem(&len))) != nullptr) { - if (req->bio) { - switch (req->res.status) { - case VIRTIO_BLK_S_OK: - trace_virtio_blk_req_ok(req->bio, req->hdr.sector, - req->bio->bio_bcount, req->hdr.type); - biodone(req->bio, true); - break; - case VIRTIO_BLK_S_UNSUPP: - trace_virtio_blk_req_unsupp(req->bio, req->hdr.sector, - req->bio->bio_bcount, req->hdr.type); - biodone(req->bio, false); - break; - default: - trace_virtio_blk_req_err(req->bio, req->hdr.sector, - req->bio->bio_bcount, req->hdr.type); - biodone(req->bio, false); - break; - } - } - delete req; - queue->get_buf_finalize(); - n++; - } - return n; -} - -/* - * req_done() — single completion thread for all virtqueues. + * req_done() — completion thread for single virtqueue. * - * virtio-blk uses one shared interrupt, so a completion landing on any queue - * wakes this thread. It sleeps until at least one queue's used ring is - * non-empty, then drains every queue. Each queue's lock is held only while - * that queue is drained, so make_request() on other CPUs/queues can proceed - * concurrently. + * virtio-blk uses one interrupt per queue, so a completion landing on that queue + * wakes its thread. It sleeps until this queue's used ring is non-empty, + * then drains it. No lock needs to be held while the queue is drained, + * as the req_done() for each queue is handled on different cpu, and also + * is thread-safe when interacting with producer threads calling make_request() + * because it races only on the lock-free _used_ring_host_head counter. */ -bool blk::any_queue_not_empty() +void blk::req_done(int qid) { - for (int q = 0; q < _num_queues; q++) { - auto* ring = get_virt_queue(q); - if (ring->used_ring_not_empty()) { - ring->disable_interrupts(); - return true; - } - } - // Nothing pending: re-arm interrupts on every queue, then re-check to - // close the race where a completion arrives between the loop above and - // enabling interrupts here. - for (int q = 0; q < _num_queues; q++) { - get_virt_queue(q)->enable_interrupts(); - } - for (int q = 0; q < _num_queues; q++) { - auto* ring = get_virt_queue(q); - if (ring->used_ring_not_empty()) { - ring->disable_interrupts(); - return true; - } - } - return false; -} + auto* queue = get_virt_queue(qid); + blk_req* req; -void blk::req_done() -{ while (1) { - sched::thread::wait_until([this] { return this->any_queue_not_empty(); }); + + virtio_driver::wait_for_queue(queue, &vring::used_ring_not_empty); trace_virtio_blk_wake(); - for (int q = 0; q < _num_queues; q++) { - // Do NOT take _queue_locks[q] here. make_request() holds that lock - // across vring::add_buf_wait(), which SLEEPS when the ring is full - // waiting for this completion thread to advance _used_ring_host_head - // (via get_buf_finalize) so the producer can GC descriptors and make - // room. If we grabbed the same lock we would block on the sleeping - // producer while it waits on us -> permanent deadlock (seen ~1-in-3 - // under heavy ZFS checkpoint write load at -smp1: z_wr_iss stuck in - // add_buf_wait holding the queue lock, virtio-blk req_done stuck on - // that lock, disk progress = 0 forever). The - // completion drain is a single-consumer path (only this one req_done - // thread runs it) and races the producer's get_buf_gc only on the - // u16 _used_ring_host_head counter -- the same lock-free - // producer/consumer split the original single-queue driver used - // before per-queue submission locks were added. The per-queue lock - // still serialises concurrent make_request producers; it must not - // gate completions. - auto* q_ring = get_virt_queue(q); - drain_queue(q_ring); - q_ring->wakeup_waiter(); + u32 len; + while((req = static_cast(queue->get_buf_elem(&len))) != nullptr) { + if (req->bio) { + switch (req->res.status) { + case VIRTIO_BLK_S_OK: + trace_virtio_blk_req_ok(req->bio, req->hdr.sector, req->bio->bio_bcount, req->hdr.type); + biodone(req->bio, true); + break; + case VIRTIO_BLK_S_UNSUPP: + trace_virtio_blk_req_unsupp(req->bio, req->hdr.sector, req->bio->bio_bcount, req->hdr.type); + biodone(req->bio, false); + break; + default: + trace_virtio_blk_req_err(req->bio, req->hdr.sector, req->bio->bio_bcount, req->hdr.type); + biodone(req->bio, false); + break; + } + } + + delete req; + queue->get_buf_finalize(); } + + // wake up the requesting thread in case the ring was full before + queue->wakeup_waiter(); } } diff --git a/drivers/virtio-blk.hh b/drivers/virtio-blk.hh index f3b4778d5..404c93006 100644 --- a/drivers/virtio-blk.hh +++ b/drivers/virtio-blk.hh @@ -150,7 +150,7 @@ public: int make_request(struct bio*); - void req_done(); + void req_done(int qid); int64_t size(); void set_readonly() {_ro = true;} @@ -159,15 +159,8 @@ public: bool ack_irq(); static hw_driver* probe(hw_device* dev); - - /* Pull all completed requests off one virtqueue ring. */ - static int drain_queue(vring* queue); private: - /* Wake predicate for the completion thread: true if any queue's used - * ring has completions pending. */ - bool any_queue_not_empty(); - struct blk_req { blk_req(struct bio* b) :bio(b) {}; ~blk_req() {}; diff --git a/scripts/run.py b/scripts/run.py index a2ec649f6..0ec24aaa8 100755 --- a/scripts/run.py +++ b/scripts/run.py @@ -699,11 +699,8 @@ def main(options): else: cmdargs.virtio_device_suffix = "" - # Add multiqueue support for virtio-blk if specified - if cmdargs.virtio_blk_queues > 1: - cmdargs.virtio_blk_queue_suffix = ",num-queues=%d" % cmdargs.virtio_blk_queues - else: - cmdargs.virtio_blk_queue_suffix = "" + # Add multiqueue support for virtio-blk + cmdargs.virtio_blk_queue_suffix = ",num-queues=%d" % cmdargs.virtio_blk_queues if cmdargs.networking and cmdargs.tap and (cmdargs.execute == None or '--ip=' not in cmdargs.execute): process = subprocess.run(["ip", "address", "show", cmdargs.tap], stdout=subprocess.PIPE)