virtio-blk: handle completions on the queue's dedicated thread - #1466
Open
wkozaczuk wants to merge 1 commit into
Open
virtio-blk: handle completions on the queue's dedicated thread#1466wkozaczuk wants to merge 1 commit into
wkozaczuk wants to merge 1 commit into
Conversation
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.
There was a problem hiding this comment.
Pull request overview
This PR refactors OSv’s virtio-blk multiqueue completion handling to use one completion thread per virtqueue (instead of a single shared completion thread), and updates the run script/documentation to align with the new behavior and the desired default queue count.
Changes:
- Create and pin one virtio-blk completion thread per queue, and route MSI-X queue interrupts to the corresponding thread.
- Rework completion draining logic to be per-queue (
req_done(int qid)) usingvirtio_driver::wait_for_queue(). - Update
scripts/run.pyto always pass an explicitnum-queuesto QEMU (defaulting to 1) and refresh multiqueue documentation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| scripts/run.py | Always emits an explicit num-queues=... for virtio-blk devices (default 1). |
| drivers/virtio-blk.hh | Updates completion handler API to be per-queue (req_done(int qid)). |
| drivers/virtio-blk.cc | Implements per-queue completion threads and MSI-X bindings; updates completion draining logic. |
| documentation/block-multiqueue.md | Updates multiqueue design/behavior documentation to reflect the new completion model. |
Suppressed comments (3)
drivers/virtio-blk.cc:217
- On the MMIO (aarch64 SPI) path, the interrupt handler always wakes only the first completion thread. If multiple queues are active, other queues’ completions will not be processed. Consider waking all per-queue completion threads (and disabling interrupts on all queues) when
_num_queues > 1.
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(); },
[=] { single_thread->wake_with_irq_disabled(); });
drivers/virtio-blk.cc:223
- On the MMIO (GSI) path, only the first completion thread is woken. If multiple queues are active, completions on queues >0 will not be drained and I/O may hang. Consider disabling interrupts for all queues and waking all completion threads when
_num_queues > 1.
int_factory.create_gsi_edge_interrupt = [this,single_thread]() {
return new gsi_edge_interrupt(
_dev.get_irq(),
[=] { if (this->ack_irq()) single_thread->wake_with_irq_disabled(); });
documentation/block-multiqueue.md:89
- This paragraph has a stray closing parenthesis and its description of the non-MSI-X/MMIO completion path doesn’t match the current implementation (there’s no longer an “all-queue drain” thread). Please update the wording to reflect the actual behavior.
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.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _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<u32>(virtio_driver::_num_queues, sched::cpus.size()); |
Comment on lines
+702
to
+703
| # Add multiqueue support for virtio-blk | ||
| cmdargs.virtio_blk_queue_suffix = ",num-queues=%d" % cmdargs.virtio_blk_queues |
Comment on lines
+157
to
+159
| // 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 |
| 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 }); |
Comment on lines
+202
to
+206
| 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(); }); |
Comment on lines
+308
to
+312
| * 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. |
Comment on lines
+65
to
66
| 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This changes virtio-blk driver to create as many completion threads as there are virt queues. So if there are more than a single queue, interrupts delivered are serviced by its own dedicated queue instead of a single shared one. The idea is to minimize the 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 a single completion thread handle all queues.
Finally, this modifies
run.pyto explicitly set the number of virt queues to 1 as advertised instead of defaulting to the number of cpus.It turns out that multiple queues (>1) do not really lead to higher throughput, as these results demonstrate: