[Qemu-devel] [PATCH 0/3] virtio: disable notifications in blk and scsi

[Qemu-devel] [PATCH 0/3] virtio: disable notifications in blk and scsi

[Stefan Hajnoczi]

Disabling notifications during virtqueue processing reduces the number of
exits.  The virtio-net device already uses virtio_queue_set_notifications() but
virtio-blk and virtio-scsi do not.

The following benchmark shows a 15% reduction in virtio-blk-pci MMIO exits:

  (host)$ qemu-system-x86_64 \
              -enable-kvm -m 1024 -cpu host \
              -drive if=virtio,id=drive0,file=f24.img,format=raw,\
                     cache=none,aio=native
  (guest)$ fio # jobs=4, iodepth=8, direct=1, randread
  (host)$ sudo perf record -a -e kvm:kvm_fast_mmio

Number of kvm_fast_mmio events:
Unpatched: 685k
Patched: 592k (-15%, lower is better)

Note that a workload with iodepth=1 and a single thread will not benefit - this
is a batching optimization.  The effect should be strongest with large iodepth
and multiple threads submitting I/O.  The guest I/O scheduler also affects the
optimization.

Stefan Hajnoczi (3):
  virtio: add missing vdev->broken check
  virtio-blk: suppress virtqueue kick during processing
  virtio-scsi: suppress virtqueue kick during processing

 hw/block/virtio-blk.c | 18 ++++++++++++------
 hw/scsi/virtio-scsi.c | 36 +++++++++++++++++++++---------------
 hw/virtio/virtio.c    |  4 ++++
 3 files changed, 37 insertions(+), 21 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] virtio: add missing vdev->broken check

[Stefan Hajnoczi]

virtio_queue_notify_vq() checks vdev->broken before invoking the
handler, virtio_queue_notify_aio_vq() should too.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 55a00cd..a4759bd 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1239,6 +1239,10 @@ static void virtio_queue_notify_aio_vq(VirtQueue *vq)
     if (vq->vring.desc && vq->handle_aio_output) {
         VirtIODevice *vdev = vq->vdev;
 
+        if (unlikely(vq->vdev->broken)) {
+            return;
+        }
+
         trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
         vq->handle_aio_output(vdev, vq);
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/3] virtio: add missing vdev->broken check

[Cornelia Huck]

On Wed, 16 Nov 2016 21:53:07 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> virtio_queue_notify_vq() checks vdev->broken before invoking the
> handler, virtio_queue_notify_aio_vq() should too.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio.c | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

I think this makes sense as a fix independent of your other patches.

Re: [Qemu-devel] [PATCH 1/3] virtio: add missing vdev->broken check

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On Thu, Nov 17, 2016 at 09:31:18AM +0100, Cornelia Huck wrote:
> On Wed, 16 Nov 2016 21:53:07 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > virtio_queue_notify_vq() checks vdev->broken before invoking the
> > handler, virtio_queue_notify_aio_vq() should too.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  hw/virtio/virtio.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> 
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> I think this makes sense as a fix independent of your other patches.

I'm not aware of an actual bug caused by this.  virtio_queue_pop()
returns NULL when the device is broken.

This is more for consistency than correctness, so I didn't add
qemu-stable, -rc1, or send it as a separate fix.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 1/3] virtio: add missing vdev->broken check

[Cornelia Huck]

On Thu, 17 Nov 2016 10:58:30 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Thu, Nov 17, 2016 at 09:31:18AM +0100, Cornelia Huck wrote:
> > On Wed, 16 Nov 2016 21:53:07 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > 
> > > virtio_queue_notify_vq() checks vdev->broken before invoking the
> > > handler, virtio_queue_notify_aio_vq() should too.
> > > 
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  hw/virtio/virtio.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > 
> > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > 
> > I think this makes sense as a fix independent of your other patches.
> 
> I'm not aware of an actual bug caused by this.  virtio_queue_pop()
> returns NULL when the device is broken.
> 
> This is more for consistency than correctness, so I didn't add
> qemu-stable, -rc1, or send it as a separate fix.

Fair enough. It was more "we should add this patch even if we don't use
the other patches in this series".

Re: [Qemu-devel] [PATCH 1/3] virtio: add missing vdev->broken check

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

On Thu, Nov 17, 2016 at 01:24:47PM +0100, Cornelia Huck wrote:
> On Thu, 17 Nov 2016 10:58:30 +0000
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
> > On Thu, Nov 17, 2016 at 09:31:18AM +0100, Cornelia Huck wrote:
> > > On Wed, 16 Nov 2016 21:53:07 +0000
> > > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > 
> > > > virtio_queue_notify_vq() checks vdev->broken before invoking the
> > > > handler, virtio_queue_notify_aio_vq() should too.
> > > > 
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > ---
> > > >  hw/virtio/virtio.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > 
> > > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > 
> > > I think this makes sense as a fix independent of your other patches.
> > 
> > I'm not aware of an actual bug caused by this.  virtio_queue_pop()
> > returns NULL when the device is broken.
> > 
> > This is more for consistency than correctness, so I didn't add
> > qemu-stable, -rc1, or send it as a separate fix.
> 
> Fair enough. It was more "we should add this patch even if we don't use
> the other patches in this series".

Okay.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 2/3] virtio-blk: suppress virtqueue kick during processing

[Stefan Hajnoczi]

The guest does not need to kick the virtqueue while we are processing
it.  This reduces the number of vmexits during periods of heavy I/O.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 0c5fd27..50bb0cb 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -588,13 +588,19 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq)
 
     blk_io_plug(s->blk);
 
-    while ((req = virtio_blk_get_request(s, vq))) {
-        if (virtio_blk_handle_request(req, &mrb)) {
-            virtqueue_detach_element(req->vq, &req->elem, 0);
-            virtio_blk_free_request(req);
-            break;
+    do {
+        virtio_queue_set_notification(vq, 0);
+
+        while ((req = virtio_blk_get_request(s, vq))) {
+            if (virtio_blk_handle_request(req, &mrb)) {
+                virtqueue_detach_element(req->vq, &req->elem, 0);
+                virtio_blk_free_request(req);
+                break;
+            }
         }
-    }
+
+        virtio_queue_set_notification(vq, 1);
+    } while (!virtio_queue_empty(vq));
 
     if (mrb.num_reqs) {
         virtio_blk_submit_multireq(s->blk, &mrb);
-- 
2.7.4

[Qemu-devel] [PATCH 3/3] virtio-scsi: suppress virtqueue kick during processing

[Stefan Hajnoczi]

The guest does not need to kick the virtqueue while we are processing
it.  This reduces the number of vmexits during periods of heavy I/O.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/scsi/virtio-scsi.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 3e5ae6a..4d23a78 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -578,26 +578,32 @@ static void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req)
 void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
 {
     VirtIOSCSIReq *req, *next;
-    int ret;
+    int ret = 0;
 
     QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
 
-    while ((req = virtio_scsi_pop_req(s, vq))) {
-        ret = virtio_scsi_handle_cmd_req_prepare(s, req);
-        if (!ret) {
-            QTAILQ_INSERT_TAIL(&reqs, req, next);
-        } else if (ret == -EINVAL) {
-            /* The device is broken and shouldn't process any request */
-            while (!QTAILQ_EMPTY(&reqs)) {
-                req = QTAILQ_FIRST(&reqs);
-                QTAILQ_REMOVE(&reqs, req, next);
-                blk_io_unplug(req->sreq->dev->conf.blk);
-                scsi_req_unref(req->sreq);
-                virtqueue_detach_element(req->vq, &req->elem, 0);
-                virtio_scsi_free_req(req);
+    do {
+        virtio_queue_set_notification(vq, 0);
+
+        while ((req = virtio_scsi_pop_req(s, vq))) {
+            ret = virtio_scsi_handle_cmd_req_prepare(s, req);
+            if (!ret) {
+                QTAILQ_INSERT_TAIL(&reqs, req, next);
+            } else if (ret == -EINVAL) {
+                /* The device is broken and shouldn't process any request */
+                while (!QTAILQ_EMPTY(&reqs)) {
+                    req = QTAILQ_FIRST(&reqs);
+                    QTAILQ_REMOVE(&reqs, req, next);
+                    blk_io_unplug(req->sreq->dev->conf.blk);
+                    scsi_req_unref(req->sreq);
+                    virtqueue_detach_element(req->vq, &req->elem, 0);
+                    virtio_scsi_free_req(req);
+                }
             }
         }
-    }
+
+        virtio_queue_set_notification(vq, 1);
+    } while (ret != -EINVAL && !virtio_queue_empty(vq));
 
     QTAILQ_FOREACH_SAFE(req, &reqs, next, next) {
         virtio_scsi_handle_cmd_req_submit(s, req);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/3] virtio: disable notifications in blk and scsi

[Michael S. Tsirkin]

On Wed, Nov 16, 2016 at 09:53:06PM +0000, Stefan Hajnoczi wrote:
> Disabling notifications during virtqueue processing reduces the number of
> exits.  The virtio-net device already uses virtio_queue_set_notifications() but
> virtio-blk and virtio-scsi do not.
> 
> The following benchmark shows a 15% reduction in virtio-blk-pci MMIO exits:
> 
>   (host)$ qemu-system-x86_64 \
>               -enable-kvm -m 1024 -cpu host \
>               -drive if=virtio,id=drive0,file=f24.img,format=raw,\
>                      cache=none,aio=native
>   (guest)$ fio # jobs=4, iodepth=8, direct=1, randread
>   (host)$ sudo perf record -a -e kvm:kvm_fast_mmio
> 
> Number of kvm_fast_mmio events:
> Unpatched: 685k
> Patched: 592k (-15%, lower is better)

Any chance to see a gain in actual benchmark numbers?
This is important to make sure we are not just
shifting overhead around.


> Note that a workload with iodepth=1 and a single thread will not benefit - this
> is a batching optimization.  The effect should be strongest with large iodepth
> and multiple threads submitting I/O.  The guest I/O scheduler also affects the
> optimization.
> 
> Stefan Hajnoczi (3):
>   virtio: add missing vdev->broken check
>   virtio-blk: suppress virtqueue kick during processing
>   virtio-scsi: suppress virtqueue kick during processing
> 
>  hw/block/virtio-blk.c | 18 ++++++++++++------
>  hw/scsi/virtio-scsi.c | 36 +++++++++++++++++++++---------------
>  hw/virtio/virtio.c    |  4 ++++
>  3 files changed, 37 insertions(+), 21 deletions(-)
> 
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH 0/3] virtio: disable notifications in blk and scsi

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

On Thu, Nov 17, 2016 at 12:17:57AM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 16, 2016 at 09:53:06PM +0000, Stefan Hajnoczi wrote:
> > Disabling notifications during virtqueue processing reduces the number of
> > exits.  The virtio-net device already uses virtio_queue_set_notifications() but
> > virtio-blk and virtio-scsi do not.
> > 
> > The following benchmark shows a 15% reduction in virtio-blk-pci MMIO exits:
> > 
> >   (host)$ qemu-system-x86_64 \
> >               -enable-kvm -m 1024 -cpu host \
> >               -drive if=virtio,id=drive0,file=f24.img,format=raw,\
> >                      cache=none,aio=native
> >   (guest)$ fio # jobs=4, iodepth=8, direct=1, randread
> >   (host)$ sudo perf record -a -e kvm:kvm_fast_mmio
> > 
> > Number of kvm_fast_mmio events:
> > Unpatched: 685k
> > Patched: 592k (-15%, lower is better)
> 
> Any chance to see a gain in actual benchmark numbers?
> This is important to make sure we are not just
> shifting overhead around.

Good idea.  I reran this morning without any tracing and compared
against bare metal.

Total reads for a 30-second 4 KB random read benchmark with 4 processes
x iodepth=8:

Bare metal: 26440 MB
Unpatched:  19799 MB
Patched:    21252 MB

Patched vs Unpatched: +7% improvement
Patched vs Bare metal: 20% virtualization overhead

The disk image is a 8 GB raw file on XFS on LVM on dm-crypt on a Samsung
MZNLN256HCHP 256 GB SATA SSD.  This is just my laptop.

Seems like a worthwhile improvement to me.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/3] virtio: disable notifications in blk and scsi

[Michael S. Tsirkin]

On Thu, Nov 17, 2016 at 01:27:49PM +0000, Stefan Hajnoczi wrote:
> On Thu, Nov 17, 2016 at 12:17:57AM +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 16, 2016 at 09:53:06PM +0000, Stefan Hajnoczi wrote:
> > > Disabling notifications during virtqueue processing reduces the number of
> > > exits.  The virtio-net device already uses virtio_queue_set_notifications() but
> > > virtio-blk and virtio-scsi do not.
> > > 
> > > The following benchmark shows a 15% reduction in virtio-blk-pci MMIO exits:
> > > 
> > >   (host)$ qemu-system-x86_64 \
> > >               -enable-kvm -m 1024 -cpu host \
> > >               -drive if=virtio,id=drive0,file=f24.img,format=raw,\
> > >                      cache=none,aio=native
> > >   (guest)$ fio # jobs=4, iodepth=8, direct=1, randread
> > >   (host)$ sudo perf record -a -e kvm:kvm_fast_mmio
> > > 
> > > Number of kvm_fast_mmio events:
> > > Unpatched: 685k
> > > Patched: 592k (-15%, lower is better)
> > 
> > Any chance to see a gain in actual benchmark numbers?
> > This is important to make sure we are not just
> > shifting overhead around.
> 
> Good idea.  I reran this morning without any tracing and compared
> against bare metal.
> 
> Total reads for a 30-second 4 KB random read benchmark with 4 processes
> x iodepth=8:
> 
> Bare metal: 26440 MB
> Unpatched:  19799 MB
> Patched:    21252 MB
> 
> Patched vs Unpatched: +7% improvement
> Patched vs Bare metal: 20% virtualization overhead
> 
> The disk image is a 8 GB raw file on XFS on LVM on dm-crypt on a Samsung
> MZNLN256HCHP 256 GB SATA SSD.  This is just my laptop.
> 
> Seems like a worthwhile improvement to me.
> 
> Stefan

Sure. Pls remember to ping or re-post after the release.

Re: [Qemu-devel] [PATCH 0/3] virtio: disable notifications in blk and scsi

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

On Thu, Nov 17, 2016 at 07:38:45PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 17, 2016 at 01:27:49PM +0000, Stefan Hajnoczi wrote:
> > On Thu, Nov 17, 2016 at 12:17:57AM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Nov 16, 2016 at 09:53:06PM +0000, Stefan Hajnoczi wrote:
> > > > Disabling notifications during virtqueue processing reduces the number of
> > > > exits.  The virtio-net device already uses virtio_queue_set_notifications() but
> > > > virtio-blk and virtio-scsi do not.
> > > > 
> > > > The following benchmark shows a 15% reduction in virtio-blk-pci MMIO exits:
> > > > 
> > > >   (host)$ qemu-system-x86_64 \
> > > >               -enable-kvm -m 1024 -cpu host \
> > > >               -drive if=virtio,id=drive0,file=f24.img,format=raw,\
> > > >                      cache=none,aio=native
> > > >   (guest)$ fio # jobs=4, iodepth=8, direct=1, randread
> > > >   (host)$ sudo perf record -a -e kvm:kvm_fast_mmio
> > > > 
> > > > Number of kvm_fast_mmio events:
> > > > Unpatched: 685k
> > > > Patched: 592k (-15%, lower is better)
> > > 
> > > Any chance to see a gain in actual benchmark numbers?
> > > This is important to make sure we are not just
> > > shifting overhead around.
> > 
> > Good idea.  I reran this morning without any tracing and compared
> > against bare metal.
> > 
> > Total reads for a 30-second 4 KB random read benchmark with 4 processes
> > x iodepth=8:
> > 
> > Bare metal: 26440 MB
> > Unpatched:  19799 MB
> > Patched:    21252 MB
> > 
> > Patched vs Unpatched: +7% improvement
> > Patched vs Bare metal: 20% virtualization overhead
> > 
> > The disk image is a 8 GB raw file on XFS on LVM on dm-crypt on a Samsung
> > MZNLN256HCHP 256 GB SATA SSD.  This is just my laptop.
> > 
> > Seems like a worthwhile improvement to me.
> > 
> > Stefan
> 
> Sure. Pls remember to ping or re-post after the release.

How about a -next tree?

I've found that useful for block, net, and tracing in the past.  Most of
the time it means patch authors can rest assured their patches will be
merged without further action.  It allows development of features that
depend on out-of-tree patches.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/3] virtio: disable notifications in blk and scsi

[Michael S. Tsirkin]

On Fri, Nov 18, 2016 at 10:58:47AM +0000, Stefan Hajnoczi wrote:
> On Thu, Nov 17, 2016 at 07:38:45PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 17, 2016 at 01:27:49PM +0000, Stefan Hajnoczi wrote:
> > > On Thu, Nov 17, 2016 at 12:17:57AM +0200, Michael S. Tsirkin wrote:
> > > > On Wed, Nov 16, 2016 at 09:53:06PM +0000, Stefan Hajnoczi wrote:
> > > > > Disabling notifications during virtqueue processing reduces the number of
> > > > > exits.  The virtio-net device already uses virtio_queue_set_notifications() but
> > > > > virtio-blk and virtio-scsi do not.
> > > > > 
> > > > > The following benchmark shows a 15% reduction in virtio-blk-pci MMIO exits:
> > > > > 
> > > > >   (host)$ qemu-system-x86_64 \
> > > > >               -enable-kvm -m 1024 -cpu host \
> > > > >               -drive if=virtio,id=drive0,file=f24.img,format=raw,\
> > > > >                      cache=none,aio=native
> > > > >   (guest)$ fio # jobs=4, iodepth=8, direct=1, randread
> > > > >   (host)$ sudo perf record -a -e kvm:kvm_fast_mmio
> > > > > 
> > > > > Number of kvm_fast_mmio events:
> > > > > Unpatched: 685k
> > > > > Patched: 592k (-15%, lower is better)
> > > > 
> > > > Any chance to see a gain in actual benchmark numbers?
> > > > This is important to make sure we are not just
> > > > shifting overhead around.
> > > 
> > > Good idea.  I reran this morning without any tracing and compared
> > > against bare metal.
> > > 
> > > Total reads for a 30-second 4 KB random read benchmark with 4 processes
> > > x iodepth=8:
> > > 
> > > Bare metal: 26440 MB
> > > Unpatched:  19799 MB
> > > Patched:    21252 MB
> > > 
> > > Patched vs Unpatched: +7% improvement
> > > Patched vs Bare metal: 20% virtualization overhead
> > > 
> > > The disk image is a 8 GB raw file on XFS on LVM on dm-crypt on a Samsung
> > > MZNLN256HCHP 256 GB SATA SSD.  This is just my laptop.
> > > 
> > > Seems like a worthwhile improvement to me.
> > > 
> > > Stefan
> > 
> > Sure. Pls remember to ping or re-post after the release.
> 
> How about a -next tree?

-next would make sense if we did Linus style short merge
cycles followed by a long stabilization period.

With current QEMU style -next seems counter-productive, we do freezes in
particular so people focus on stabilization, with -next everyone except
maintainers just keeps going as usual, and maintainers must handle
double the load.

> I've found that useful for block, net, and tracing in the past.  Most of
> the time it means patch authors can rest assured their patches will be
> merged without further action.  It allows development of features that
> depend on out-of-tree patches.
> 
> Stefan

Less work for authors, more work for me ... I'd rather distribute the load.

-- 
MST

Re: [Qemu-devel] [PATCH 0/3] virtio: disable notifications in blk and scsi

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3007 bytes --]

On Fri, Nov 18, 2016 at 04:21:33PM +0200, Michael S. Tsirkin wrote:
> On Fri, Nov 18, 2016 at 10:58:47AM +0000, Stefan Hajnoczi wrote:
> > On Thu, Nov 17, 2016 at 07:38:45PM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Nov 17, 2016 at 01:27:49PM +0000, Stefan Hajnoczi wrote:
> > > > On Thu, Nov 17, 2016 at 12:17:57AM +0200, Michael S. Tsirkin wrote:
> > > > > On Wed, Nov 16, 2016 at 09:53:06PM +0000, Stefan Hajnoczi wrote:
> > > > > > Disabling notifications during virtqueue processing reduces the number of
> > > > > > exits.  The virtio-net device already uses virtio_queue_set_notifications() but
> > > > > > virtio-blk and virtio-scsi do not.
> > > > > > 
> > > > > > The following benchmark shows a 15% reduction in virtio-blk-pci MMIO exits:
> > > > > > 
> > > > > >   (host)$ qemu-system-x86_64 \
> > > > > >               -enable-kvm -m 1024 -cpu host \
> > > > > >               -drive if=virtio,id=drive0,file=f24.img,format=raw,\
> > > > > >                      cache=none,aio=native
> > > > > >   (guest)$ fio # jobs=4, iodepth=8, direct=1, randread
> > > > > >   (host)$ sudo perf record -a -e kvm:kvm_fast_mmio
> > > > > > 
> > > > > > Number of kvm_fast_mmio events:
> > > > > > Unpatched: 685k
> > > > > > Patched: 592k (-15%, lower is better)
> > > > > 
> > > > > Any chance to see a gain in actual benchmark numbers?
> > > > > This is important to make sure we are not just
> > > > > shifting overhead around.
> > > > 
> > > > Good idea.  I reran this morning without any tracing and compared
> > > > against bare metal.
> > > > 
> > > > Total reads for a 30-second 4 KB random read benchmark with 4 processes
> > > > x iodepth=8:
> > > > 
> > > > Bare metal: 26440 MB
> > > > Unpatched:  19799 MB
> > > > Patched:    21252 MB
> > > > 
> > > > Patched vs Unpatched: +7% improvement
> > > > Patched vs Bare metal: 20% virtualization overhead
> > > > 
> > > > The disk image is a 8 GB raw file on XFS on LVM on dm-crypt on a Samsung
> > > > MZNLN256HCHP 256 GB SATA SSD.  This is just my laptop.
> > > > 
> > > > Seems like a worthwhile improvement to me.
> > > > 
> > > > Stefan
> > > 
> > > Sure. Pls remember to ping or re-post after the release.
> > 
> > How about a -next tree?
> 
> -next would make sense if we did Linus style short merge
> cycles followed by a long stabilization period.
> 
> With current QEMU style -next seems counter-productive, we do freezes in
> particular so people focus on stabilization, with -next everyone except
> maintainers just keeps going as usual, and maintainers must handle
> double the load.
> 
> > I've found that useful for block, net, and tracing in the past.  Most of
> > the time it means patch authors can rest assured their patches will be
> > merged without further action.  It allows development of features that
> > depend on out-of-tree patches.
> > 
> > Stefan
> 
> Less work for authors, more work for me ... I'd rather distribute the load.

Okay.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/3] virtio: disable notifications in blk and scsi

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]

On Thu, Nov 17, 2016 at 07:38:45PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 17, 2016 at 01:27:49PM +0000, Stefan Hajnoczi wrote:
> > On Thu, Nov 17, 2016 at 12:17:57AM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Nov 16, 2016 at 09:53:06PM +0000, Stefan Hajnoczi wrote:
> > > > Disabling notifications during virtqueue processing reduces the number of
> > > > exits.  The virtio-net device already uses virtio_queue_set_notifications() but
> > > > virtio-blk and virtio-scsi do not.
> > > > 
> > > > The following benchmark shows a 15% reduction in virtio-blk-pci MMIO exits:
> > > > 
> > > >   (host)$ qemu-system-x86_64 \
> > > >               -enable-kvm -m 1024 -cpu host \
> > > >               -drive if=virtio,id=drive0,file=f24.img,format=raw,\
> > > >                      cache=none,aio=native
> > > >   (guest)$ fio # jobs=4, iodepth=8, direct=1, randread
> > > >   (host)$ sudo perf record -a -e kvm:kvm_fast_mmio
> > > > 
> > > > Number of kvm_fast_mmio events:
> > > > Unpatched: 685k
> > > > Patched: 592k (-15%, lower is better)
> > > 
> > > Any chance to see a gain in actual benchmark numbers?
> > > This is important to make sure we are not just
> > > shifting overhead around.
> > 
> > Good idea.  I reran this morning without any tracing and compared
> > against bare metal.
> > 
> > Total reads for a 30-second 4 KB random read benchmark with 4 processes
> > x iodepth=8:
> > 
> > Bare metal: 26440 MB
> > Unpatched:  19799 MB
> > Patched:    21252 MB
> > 
> > Patched vs Unpatched: +7% improvement
> > Patched vs Bare metal: 20% virtualization overhead
> > 
> > The disk image is a 8 GB raw file on XFS on LVM on dm-crypt on a Samsung
> > MZNLN256HCHP 256 GB SATA SSD.  This is just my laptop.
> > 
> > Seems like a worthwhile improvement to me.
> > 
> > Stefan
> 
> Sure. Pls remember to ping or re-post after the release.

The block pull request I just sent with AioContext polling includes
these patches.  Just noticed this and remembered this email thread.
Feel free to NACK the pull request if you wish to discuss this series
again: <20170104133414.6524-1-stefanha@redhat.com>

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/3] virtio: disable notifications in blk and scsi

[Christian Borntraeger]

On 11/16/2016 10:53 PM, Stefan Hajnoczi wrote:
> Disabling notifications during virtqueue processing reduces the number of
> exits.  The virtio-net device already uses virtio_queue_set_notifications() but
> virtio-blk and virtio-scsi do not.
> 
> The following benchmark shows a 15% reduction in virtio-blk-pci MMIO exits:
> 
>   (host)$ qemu-system-x86_64 \
>               -enable-kvm -m 1024 -cpu host \
>               -drive if=virtio,id=drive0,file=f24.img,format=raw,\
>                      cache=none,aio=native
>   (guest)$ fio # jobs=4, iodepth=8, direct=1, randread
>   (host)$ sudo perf record -a -e kvm:kvm_fast_mmio
> 
> Number of kvm_fast_mmio events:
> Unpatched: 685k
> Patched: 592k (-15%, lower is better)
> 
> Note that a workload with iodepth=1 and a single thread will not benefit - this
> is a batching optimization.  The effect should be strongest with large iodepth
> and multiple threads submitting I/O.  The guest I/O scheduler also affects the
> optimization.

I have trouble seeing any difference in terms of performances or CPU load (other than 
a reduced number of kicks).
I was expecting some benefit by reducing the spinlock hold times in virtio-blk,
but this needs some more setups to actually find the sweet spot.

Maybe it will show its benefit with the polling thing?
> 
> Stefan Hajnoczi (3):
>   virtio: add missing vdev->broken check
>   virtio-blk: suppress virtqueue kick during processing
>   virtio-scsi: suppress virtqueue kick during processing
> 
>  hw/block/virtio-blk.c | 18 ++++++++++++------
>  hw/scsi/virtio-scsi.c | 36 +++++++++++++++++++++---------------
>  hw/virtio/virtio.c    |  4 ++++
>  3 files changed, 37 insertions(+), 21 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/3] virtio: disable notifications in blk and scsi

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]

On Thu, Nov 17, 2016 at 12:01:30PM +0100, Christian Borntraeger wrote:
> On 11/16/2016 10:53 PM, Stefan Hajnoczi wrote:
> > Disabling notifications during virtqueue processing reduces the number of
> > exits.  The virtio-net device already uses virtio_queue_set_notifications() but
> > virtio-blk and virtio-scsi do not.
> > 
> > The following benchmark shows a 15% reduction in virtio-blk-pci MMIO exits:
> > 
> >   (host)$ qemu-system-x86_64 \
> >               -enable-kvm -m 1024 -cpu host \
> >               -drive if=virtio,id=drive0,file=f24.img,format=raw,\
> >                      cache=none,aio=native
> >   (guest)$ fio # jobs=4, iodepth=8, direct=1, randread
> >   (host)$ sudo perf record -a -e kvm:kvm_fast_mmio
> > 
> > Number of kvm_fast_mmio events:
> > Unpatched: 685k
> > Patched: 592k (-15%, lower is better)
> > 
> > Note that a workload with iodepth=1 and a single thread will not benefit - this
> > is a batching optimization.  The effect should be strongest with large iodepth
> > and multiple threads submitting I/O.  The guest I/O scheduler also affects the
> > optimization.
> 
> I have trouble seeing any difference in terms of performances or CPU load (other than 
> a reduced number of kicks).
> I was expecting some benefit by reducing the spinlock hold times in virtio-blk,
> but this needs some more setups to actually find the sweet spot.

Are you testing on s390 with ccw?  I'm not familiar with the performance
characteristics of the kick under ccw.

> Maybe it will show its benefit with the polling thing?

Yes, I hope it will benefit polling.  I'll build patches for polling on
top of this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/3] virtio: disable notifications in blk and scsi

[Christian Borntraeger]

On 11/18/2016 12:02 PM, Stefan Hajnoczi wrote:
> On Thu, Nov 17, 2016 at 12:01:30PM +0100, Christian Borntraeger wrote:
>> On 11/16/2016 10:53 PM, Stefan Hajnoczi wrote:
>>> Disabling notifications during virtqueue processing reduces the number of
>>> exits.  The virtio-net device already uses virtio_queue_set_notifications() but
>>> virtio-blk and virtio-scsi do not.
>>>
>>> The following benchmark shows a 15% reduction in virtio-blk-pci MMIO exits:
>>>
>>>   (host)$ qemu-system-x86_64 \
>>>               -enable-kvm -m 1024 -cpu host \
>>>               -drive if=virtio,id=drive0,file=f24.img,format=raw,\
>>>                      cache=none,aio=native
>>>   (guest)$ fio # jobs=4, iodepth=8, direct=1, randread
>>>   (host)$ sudo perf record -a -e kvm:kvm_fast_mmio
>>>
>>> Number of kvm_fast_mmio events:
>>> Unpatched: 685k
>>> Patched: 592k (-15%, lower is better)
>>>
>>> Note that a workload with iodepth=1 and a single thread will not benefit - this
>>> is a batching optimization.  The effect should be strongest with large iodepth
>>> and multiple threads submitting I/O.  The guest I/O scheduler also affects the
>>> optimization.
>>
>> I have trouble seeing any difference in terms of performances or CPU load (other than 
>> a reduced number of kicks).
>> I was expecting some benefit by reducing the spinlock hold times in virtio-blk,
>> but this needs some more setups to actually find the sweet spot.
> 
> Are you testing on s390 with ccw?  

Yes

I'm not familiar with the performance
> characteristics of the kick under ccw.

The kick is a diagnose instruction, that exits the guest into the host kernel.
In the host kernel it will notify an eventfd and return back to the guest, 
so in essence it should be the same as x86. I was using host ramdisks, maybe this
has affected the performance.

> 
>> Maybe it will show its benefit with the polling thing?
> 
> Yes, I hope it will benefit polling.  I'll build patches for polling on
> top of this.
>