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)