Skip to content

virtio-blk: handle completions on the queue's dedicated thread #1466

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wkozaczuk wants to merge 1 commit into
base: master
Choose a base branch
from
+108 −162
Open

virtio-blk: handle completions on the queue's dedicated thread #1466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 28 additions & 36 deletions documentation/block-multiqueue.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<mutex>(_num_queues);
```
Expand All @@ -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
Comment on lines +65 to 66
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

Expand All @@ -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
Expand Down
190 changes: 77 additions & 113 deletions drivers/virtio-blk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<u32>(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<mutex>(_num_queues);
auto threads = std::vector<sched::thread*>(_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
Comment on lines +157 to +159
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<msix_binding> 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)) {
Expand All @@ -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(); });
Comment on lines +202 to +206
};
#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
Expand All @@ -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()
Expand Down Expand Up @@ -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<blk_req*>(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.
Comment on lines +308 to +312
*/
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<blk_req*>(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();
}
}

Expand Down
9 changes: 1 addition & 8 deletions drivers/virtio-blk.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;}
Expand All @@ -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() {};
Expand Down
Loading