Google extension to improve qemu-nvme performance

Review static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val) · line 805

Quoted 10 lines · Ming Lin

(quoted diff) +5 -1

@@ -726,7 +798,11 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
         }
 
         start_sqs = nvme_cq_full(cq) ? 1 : 0;
-        cq->head = new_head;
+        /* When the mapped pointer memory area is setup, we don't rely on
+         * the MMIO written values to update the head pointer. */
+        if (!cq->db_addr) {
+            cq->head = new_head;
+        }

You are still checking

        if (new_head >= cq->size) {
            return;
        }

above. I think this is incorrect when the extension is present, and furthermore it's the only case where val is being used.

If you're not using val, you could use ioeventfd for the MMIO. An ioeventfd cuts the MMIO cost by at least 55% and up to 70%. Here are quick and dirty measurements from kvm-unit-tests's vmexit.flat benchmark, on two very different machines:

Haswell-EP Ivy Bridge i7
MMIO memory write 5100 -> 2250 (55%) 7000 -> 3000 (58%)
I/O port write 3800 -> 1150 (70%) 4100 -> 1800 (57%)

You would need to allocate two eventfds for each qid, one for the sq and one for the cq. Also, processing the queues is now bounced to the QEMU iothread, so you can probably get rid of sq->timer and cq->timer.

Paolo

Quoted 27 lines · Paolo Bonzini

Review static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val) · line 805

Quoted 10 lines · Ming Lin

(quoted diff) +5 -1

@@ -726,7 +798,11 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
         }
 
         start_sqs = nvme_cq_full(cq) ? 1 : 0;
-        cq->head = new_head;
+        /* When the mapped pointer memory area is setup, we don't rely on
+         * the MMIO written values to update the head pointer. */
+        if (!cq->db_addr) {
+            cq->head = new_head;
+        }

You are still checking

        if (new_head >= cq->size) {
            return;
        }

above. I think this is incorrect when the extension is present, and furthermore it's the only case where val is being used.

If you're not using val, you could use ioeventfd for the MMIO. An ioeventfd cuts the MMIO cost by at least 55% and up to 70%. Here are quick and dirty measurements from kvm-unit-tests's vmexit.flat benchmark, on two very different machines:

Quoted 7 lines

Haswell-EP Ivy Bridge i7
MMIO memory write 5100 -> 2250 (55%) 7000 -> 3000 (58%)
I/O port write 3800 -> 1150 (70%) 4100 -> 1800 (57%)

You would need to allocate two eventfds for each qid, one for the sq and one for the cq. Also, processing the queues is now bounced to the QEMU iothread, so you can probably get rid of sq->timer and cq->timer.

Here is a quick try.
Too late now, I'll test it morning.

Do you see obvious problem?

hw/block/nvme.c +7 -0

index 3e1c38d..d28690d 100644
@@ -543,6 +543,44 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
     return NVME_SUCCESS;
 }
 
+static void nvme_cq_notifier(EventNotifier *e)
+{
+    NvmeCQueue *cq =
+        container_of(e, NvmeCQueue, notifier);
+
+    nvme_post_cqes(cq);
+}

hw/block/nvme.c +9 -0

+}
+
+static void nvme_sq_notifier(EventNotifier *e)
+{
+    NvmeSQueue *sq =
+        container_of(e, NvmeSQueue, notifier);
+
+    nvme_process_sq(sq);
+}

hw/block/nvme.c +2 -0

+}
 
 
+
 static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)

 {
     uint64_t db_addr = le64_to_cpu(cmd->prp1);

hw/block/nvme.c +3 -1

@@ -565,6 +603,7 @@ static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
             /* Submission queue tail pointer location, 2 * QID * stride. */
             sq->db_addr = db_addr + 2 * i * 4;
             sq->eventidx_addr = eventidx_addr + 2 * i * 4;
+            nvme_init_sq_eventfd(sq);
         }
 
         if (cq != NULL) {

@@ -572,6 +611,7 @@ static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
              */
             cq->db_addr = db_addr + (2 * i + 1) * 4;
             cq->eventidx_addr = eventidx_addr + (2 * i + 1) * 4;
+            nvme_init_cq_eventfd(cq);
         }
     }
     return NVME_SUCCESS;

@@ -793,7 +833,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
         }
 
         cq = n->cq[qid];
-        if (new_head >= cq->size) {
+        if (!cq->db_addr && new_head >= cq->size) {
             return;
         }
 

hw/block/nvme.h +2 -0

index 82aeab4..608f202 100644
@@ -667,6 +667,7 @@ typedef struct NvmeSQueue {
      * do not go over this value will not result in MMIO writes (but will
      * still write the tail pointer to the "db_addr" location above). */
     uint64_t    eventidx_addr;
+    EventNotifier notifier;
 } NvmeSQueue;
 
 typedef struct NvmeCQueue {

@@ -689,6 +690,7 @@ typedef struct NvmeCQueue {
      * do not go over this value will not result in MMIO writes (but will
      * still write the head pointer to the "db_addr" location above). */
     uint64_t    eventidx_addr;
+    EventNotifier notifier;
 } NvmeCQueue;
 
 typedef struct NvmeNamespace {

Quoted 3 lines

Paolo

Quoted 56 lines · Ming Lin

Quoted 27 lines · Paolo Bonzini

Review static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val) · line 805

Quoted 10 lines · Ming Lin

(quoted diff) +5 -1

@@ -726,7 +798,11 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
         }
 
         start_sqs = nvme_cq_full(cq) ? 1 : 0;
-        cq->head = new_head;
+        /* When the mapped pointer memory area is setup, we don't rely on
+         * the MMIO written values to update the head pointer. */
+        if (!cq->db_addr) {
+            cq->head = new_head;
+        }

You are still checking

        if (new_head >= cq->size) {
            return;
        }

above. I think this is incorrect when the extension is present, and furthermore it's the only case where val is being used.

If you're not using val, you could use ioeventfd for the MMIO. An ioeventfd cuts the MMIO cost by at least 55% and up to 70%. Here are quick and dirty measurements from kvm-unit-tests's vmexit.flat benchmark, on two very different machines:

Quoted 7 lines

Haswell-EP Ivy Bridge i7
MMIO memory write 5100 -> 2250 (55%) 7000 -> 3000 (58%)
I/O port write 3800 -> 1150 (70%) 4100 -> 1800 (57%)

You would need to allocate two eventfds for each qid, one for the sq and one for the cq. Also, processing the queues is now bounced to the QEMU iothread, so you can probably get rid of sq->timer and cq->timer.

Here is a quick try.
Too late now, I'll test it morning.

Do you see obvious problem?

hw/block/nvme.c +7 -0

index 3e1c38d..d28690d 100644
@@ -543,6 +543,44 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
     return NVME_SUCCESS;
 }
 
+static void nvme_cq_notifier(EventNotifier *e)
+{
+    NvmeCQueue *cq =
+        container_of(e, NvmeCQueue, notifier);
+
+    nvme_post_cqes(cq);
+}

hw/block/nvme.c static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req) · line 562

Quoted 10 lines

hw/block/nvme.c +10 -0

+
+static void nvme_init_cq_eventfd(NvmeCQueue *cq)
+{
+    NvmeCtrl *n = cq->ctrl;
+    uint16_t offset = (cq->cqid*2+1) * NVME_CAP_DSTRD(n->bar.cap);
+
+    event_notifier_init(&cq->notifier, 0);
+    event_notifier_set_handler(&cq->notifier, nvme_cq_notifier);
+    memory_region_add_eventfd(&n->iomem,
+        0x1000 + offset, 4, true, cq->cqid*2+1, &cq->notifier);

should be 0x1000 + offset, 4, false, 0, &cq->notifier

Quoted 13 lines

hw/block/nvme.c +13 -0

+}
+
+static void nvme_sq_notifier(EventNotifier *e)
+{
+    NvmeSQueue *sq =
+        container_of(e, NvmeSQueue, notifier);
+
+    nvme_process_sq(sq);
+}
+
+static void nvme_init_sq_eventfd(NvmeSQueue *sq)
+{
+    NvmeCtrl *n = sq->ctrl;

hw/block/nvme.c static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req) · line 581

Quoted 6 lines

hw/block/nvme.c +6 -0

+    uint16_t offset = sq->sqid * 2 * NVME_CAP_DSTRD(n->bar.cap);
+
+    event_notifier_init(&sq->notifier, 0);
+    event_notifier_set_handler(&sq->notifier, nvme_sq_notifier);
+    memory_region_add_eventfd(&n->iomem,
+        0x1000 + offset, 4, true, sq->sqid * 2, &sq->notifier);

likewise should be 0x1000 + offset, 4, false, 0, &sq->notifier

[    1.752129] Freeing unused kernel memory: 420K (ffff880001b97000 - ffff880001c00000)
[    1.986573] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x30e5c9bbf83, max_idle_ns: 440795378954 ns
[    1.988187] clocksource: Switched to clocksource tsc
[    3.235423] clocksource: timekeeping watchdog: Marking clocksource 'tsc' as unstable because the skew is too large:
[    3.358713] clocksource:                       'refined-jiffies' wd_now: fffeddf3 wd_last: fffedd76 mask: ffffffff
[    3.410013] clocksource:                       'tsc' cs_now: 3c121d4ec cs_last: 340888eb7 mask: ffffffffffffffff
[    3.450026] clocksource: Switched to clocksource refined-jiffies
[    7.696769] Adding 392188k swap on /dev/vda5.  Priority:-1 extents:1 across:392188k 
[    7.902174] EXT4-fs (vda1): re-mounted. Opts: (null)
[    8.734178] EXT4-fs (vda1): re-mounted. Opts: errors=remount-ro

Then it doesn't response input for almost 1 minute. Without this patch, kernel loads quickly.

void memory_region_add_eventfd(MemoryRegion *mr,

                               hwaddr addr,
                               unsigned size,
                               bool match_data,
                               uint64_t data,
                               EventNotifier *e)

Could you help to explain what "match_data" and "data" mean?

Quoted 11 lines · Ming Lin

[    1.752129] Freeing unused kernel memory: 420K (ffff880001b97000 - ffff880001c00000)
[    1.986573] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x30e5c9bbf83, max_idle_ns: 440795378954 ns
[    1.988187] clocksource: Switched to clocksource tsc
[    3.235423] clocksource: timekeeping watchdog: Marking clocksource 'tsc' as unstable because the skew is too large:
[    3.358713] clocksource:                       'refined-jiffies' wd_now: fffeddf3 wd_last: fffedd76 mask: ffffffff
[    3.410013] clocksource:                       'tsc' cs_now: 3c121d4ec cs_last: 340888eb7 mask: ffffffffffffffff
[    3.450026] clocksource: Switched to clocksource refined-jiffies
[    7.696769] Adding 392188k swap on /dev/vda5.  Priority:-1 extents:1 across:392188k 
[    7.902174] EXT4-fs (vda1): re-mounted. Opts: (null)
[    8.734178] EXT4-fs (vda1): re-mounted. Opts: errors=remount-ro

Quoted 2 lines

Then it doesn't response input for almost 1 minute. Without this patch, kernel loads quickly.

Interesting. I guess there's time to debug it, since QEMU 2.6 is still a few months away. In the meanwhile we can apply your patch as is, apart from disabling the "if (new_head >= cq->size)" and the similar one for "if (new_ tail >= sq->size".

But, I have a possible culprit. In your nvme_cq_notifier you are not doing the equivalent of:

	start_sqs = nvme_cq_full(cq) ? 1 : 0;
        cq->head = new_head;
        if (start_sqs) {
            NvmeSQueue *sq;
            QTAILQ_FOREACH(sq, &cq->sq_list, entry) {
                timer_mod(sq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
            }
            timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
        }

Instead, you are just calling nvme_post_cqes, which is the equivalent of

	timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);

Adding a loop to nvme_cq_notifier, and having it call nvme_process_sq, might fix the weird 1-minute delay.

Quoted 34 lines · Paolo Bonzini

Quoted 11 lines · Ming Lin

[    1.752129] Freeing unused kernel memory: 420K (ffff880001b97000 - ffff880001c00000)
[    1.986573] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x30e5c9bbf83, max_idle_ns: 440795378954 ns
[    1.988187] clocksource: Switched to clocksource tsc
[    3.235423] clocksource: timekeeping watchdog: Marking clocksource 'tsc' as unstable because the skew is too large:
[    3.358713] clocksource:                       'refined-jiffies' wd_now: fffeddf3 wd_last: fffedd76 mask: ffffffff
[    3.410013] clocksource:                       'tsc' cs_now: 3c121d4ec cs_last: 340888eb7 mask: ffffffffffffffff
[    3.450026] clocksource: Switched to clocksource refined-jiffies
[    7.696769] Adding 392188k swap on /dev/vda5.  Priority:-1 extents:1 across:392188k 
[    7.902174] EXT4-fs (vda1): re-mounted. Opts: (null)
[    8.734178] EXT4-fs (vda1): re-mounted. Opts: errors=remount-ro

Quoted 2 lines

Then it doesn't response input for almost 1 minute. Without this patch, kernel loads quickly.

Interesting. I guess there's time to debug it, since QEMU 2.6 is still a few months away. In the meanwhile we can apply your patch as is, apart from disabling the "if (new_head >= cq->size)" and the similar one for "if (new_ tail >= sq->size".

But, I have a possible culprit. In your nvme_cq_notifier you are not doing the equivalent of:

	start_sqs = nvme_cq_full(cq) ? 1 : 0;
        cq->head = new_head;
        if (start_sqs) {
            NvmeSQueue *sq;
            QTAILQ_FOREACH(sq, &cq->sq_list, entry) {
                timer_mod(sq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
            }
            timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
        }

Quoted 6 lines

Instead, you are just calling nvme_post_cqes, which is the equivalent of

	timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);

Adding a loop to nvme_cq_notifier, and having it call nvme_process_sq, might fix the weird 1-minute delay.

I found it.

hw/block/nvme.c +2 -0

index 31572f2..f27fd35 100644
@@ -548,6 +548,7 @@ static void nvme_cq_notifier(EventNotifier *e)
     NvmeCQueue *cq =
         container_of(e, NvmeCQueue, notifier);
 
+    event_notifier_test_and_clear(&cq->notifier);
     nvme_post_cqes(cq);
 }
 

@@ -567,6 +568,7 @@ static void nvme_sq_notifier(EventNotifier *e)
     NvmeSQueue *sq =
         container_of(e, NvmeSQueue, notifier);
 
+    event_notifier_test_and_clear(&sq->notifier);
     nvme_process_sq(sq);
 }
 

Here is new performance number:

qemu-nvme + google-ext + eventfd: 294MB/s
virtio-blk: 344MB/s
virtio-scsi: 296MB/s

It's almost same as virtio-scsi. Nice.

Quoted 7 lines

Here is new performance number:

qemu-nvme + google-ext + eventfd: 294MB/s
virtio-blk: 344MB/s
virtio-scsi: 296MB/s

It's almost same as virtio-scsi. Nice.

Looks like "regular MMIO" runs in vcpu thread, while "eventfd MMIO" runs in the main loop thread.

Could you help to explain why eventfd MMIO gets better performance?

call stack: regular MMIO
========================
nvme_mmio_write (qemu/hw/block/nvme.c:921)
memory_region_write_accessor (qemu/memory.c:451)
access_with_adjusted_size (qemu/memory.c:506)
memory_region_dispatch_write (qemu/memory.c:1158)
address_space_rw (qemu/exec.c:2547)
kvm_cpu_exec (qemu/kvm-all.c:1849)
qemu_kvm_cpu_thread_fn (qemu/cpus.c:1050)
start_thread (pthread_create.c:312)
clone

call stack: eventfd MMIO
=========================
nvme_sq_notifier (qemu/hw/block/nvme.c:598)
aio_dispatch (qemu/aio-posix.c:329)
aio_ctx_dispatch (qemu/async.c:232)
g_main_context_dispatch
glib_pollfds_poll (qemu/main-loop.c:213)
os_host_main_loop_wait (qemu/main-loop.c:257)
main_loop_wait (qemu/main-loop.c:504)
main_loop (qemu/vl.c:1920)
main (qemu/vl.c:4682)
__libc_start_main