[PATCH 0/5] virtio: enable blk and scsi multi-queue by default

[PATCH 0/5] virtio: enable blk and scsi multi-queue by default

[Stefan Hajnoczi]

Enabling multi-queue helps performance on SMP guests because the completion
interrupt is handled on the vCPU that submitted the I/O request.  This avoids
IPIs inside the guest.

Note that performance is unchanged in these cases:
1. Uniprocessor guests.  They don't have IPIs.
2. Application threads might be scheduled on the sole vCPU that handles
   completion interrupts purely by chance.  (This is one reason why benchmark
   results can vary noticably between runs.)
3. Users may bind the application to the vCPU that handles completion
   interrupts.

Set the number of queues to the number of vCPUs by default.  Older machine
types continue to default to 1 queue for live migration compatibility.

This patch improves IOPS by 1-4% on an Intel Optane SSD with 4 vCPUs, -drive
aio=native, and fio bs=4k direct=1 rw=randread.

Stefan Hajnoczi (5):
  virtio-scsi: introduce a constant for fixed virtqueues
  virtio: add VirtioDeviceClass->get_num_virtqueues()
  virtio-scsi: default num_queues to -smp N
  virtio-blk: default num_queues to -smp N
  vhost-user-blk: default num_queues to -smp N

 hw/block/vhost-user-blk.c          | 15 +++++++++++++++
 hw/block/virtio-blk.c              | 17 +++++++++++++++++
 hw/core/machine.c                  |  5 +++++
 hw/scsi/vhost-scsi.c               |  2 ++
 hw/scsi/vhost-user-scsi.c          |  4 +++-
 hw/scsi/virtio-scsi.c              | 25 ++++++++++++++++++++++---
 hw/virtio/vhost-scsi-pci.c         |  4 ++--
 hw/virtio/vhost-user-blk-pci.c     |  3 ++-
 hw/virtio/vhost-user-scsi-pci.c    |  4 ++--
 hw/virtio/virtio-blk-pci.c         |  3 ++-
 hw/virtio/virtio-scsi-pci.c        |  4 ++--
 include/hw/virtio/vhost-user-blk.h |  1 +
 include/hw/virtio/virtio-blk.h     |  1 +
 include/hw/virtio/virtio-scsi.h    |  4 ++++
 include/hw/virtio/virtio.h         |  7 +++++++
 15 files changed, 87 insertions(+), 12 deletions(-)

-- 
2.24.1

[PATCH 1/5] virtio-scsi: introduce a constant for fixed virtqueues

[Stefan Hajnoczi]

The event and control virtqueues are always present, regardless of the
multi-queue configuration.  Define a constant so that virtqueue number
calculations are easier to read.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/scsi/vhost-user-scsi.c       | 2 +-
 hw/scsi/virtio-scsi.c           | 7 ++++---
 include/hw/virtio/virtio-scsi.h | 3 +++
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 23f972df59..eb37733bd0 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -115,7 +115,7 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
         goto free_virtio;
     }
 
-    vsc->dev.nvqs = 2 + vs->conf.num_queues;
+    vsc->dev.nvqs = VIRTIO_SCSI_VQ_NUM_FIXED + vs->conf.num_queues;
     vsc->dev.vqs = g_new0(struct vhost_virtqueue, vsc->dev.nvqs);
     vsc->dev.vq_index = 0;
     vsc->dev.backend_features = 0;
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 4bc73a370e..5622dd54e5 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -191,7 +191,7 @@ static void virtio_scsi_save_request(QEMUFile *f, SCSIRequest *sreq)
     VirtIOSCSIReq *req = sreq->hba_private;
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(req->dev);
     VirtIODevice *vdev = VIRTIO_DEVICE(req->dev);
-    uint32_t n = virtio_get_queue_index(req->vq) - 2;
+    uint32_t n = virtio_get_queue_index(req->vq) - VIRTIO_SCSI_VQ_NUM_FIXED;
 
     assert(n < vs->conf.num_queues);
     qemu_put_be32s(f, &n);
@@ -892,10 +892,11 @@ void virtio_scsi_common_realize(DeviceState *dev,
                 sizeof(VirtIOSCSIConfig));
 
     if (s->conf.num_queues == 0 ||
-            s->conf.num_queues > VIRTIO_QUEUE_MAX - 2) {
+            s->conf.num_queues > VIRTIO_QUEUE_MAX - VIRTIO_SCSI_VQ_NUM_FIXED) {
         error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
                          "must be a positive integer less than %d.",
-                   s->conf.num_queues, VIRTIO_QUEUE_MAX - 2);
+                   s->conf.num_queues,
+                   VIRTIO_QUEUE_MAX - VIRTIO_SCSI_VQ_NUM_FIXED);
         virtio_cleanup(vdev);
         return;
     }
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 24e768909d..9f293bcb80 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -36,6 +36,9 @@
 #define VIRTIO_SCSI_MAX_TARGET  255
 #define VIRTIO_SCSI_MAX_LUN     16383
 
+/* Number of virtqueues that are always present */
+#define VIRTIO_SCSI_VQ_NUM_FIXED    2
+
 typedef struct virtio_scsi_cmd_req VirtIOSCSICmdReq;
 typedef struct virtio_scsi_cmd_resp VirtIOSCSICmdResp;
 typedef struct virtio_scsi_ctrl_tmf_req VirtIOSCSICtrlTMFReq;
-- 
2.24.1

[PATCH 2/5] virtio: add VirtioDeviceClass->get_num_virtqueues()

[Stefan Hajnoczi]

The virtio-pci proxy objects have an "nvectors" qdev property to
allocate MSI vectors.  This property defaults to
DEV_NVECTORS_UNSPECIFIED on multi-queue devices and the final value is
based on the number of virtqueues.

The number of virtqueues is typically calculated like this by the
virtio-pci proxy object:

  num_virtqueues = NUM_FIXED_VIRTQUEUES + conf.num_queues

where conf.num_queues is a qdev property to allocate virtqueues.

Add VirtioDeviceClass->get_num_virtqueues() so that this calculation can
be performed by the device instead of the proxy object.  This removes
knowledge of virtqueue layout from the proxy object and allows the
device to implement other formulas for calculating the number of
virtqueues.

This patch has no use on its own but keeping it separate eases backports
if someone wants the virtio-blk patch but not virtio-scsi, or vice
versa.  The next patch uses this new function to automatically set
conf.num_queues to -smp N for automatic multi-queue configuration.

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

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index b69d517496..048c81fcef 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -163,6 +163,13 @@ typedef struct VirtioDeviceClass {
     int (*post_load)(VirtIODevice *vdev);
     const VMStateDescription *vmsd;
     bool (*primary_unplug_pending)(void *opaque);
+
+    /*
+     * Return the number of virtqueues.  Called by transports that need to
+     * allocate per-virtqueue interrupt resources.  This function may be called
+     * before the device is realized.
+     */
+    uint32_t (*get_num_virtqueues)(VirtIODevice *vdev);
 } VirtioDeviceClass;
 
 void virtio_instance_init_common(Object *proxy_obj, void *data,
-- 
2.24.1

[PATCH 3/5] virtio-scsi: default num_queues to -smp N

[Stefan Hajnoczi]

Automatically size the number of request virtqueues to match the number
of vCPUs.  This ensures that completion interrupts are handled on the
same vCPU that submitted the request.  No IPI is necessary to complete
an I/O request and performance is improved.

Remember that virtqueue numbering assumptions are being removed from the
virtio-pci proxy object, so the Control and Event virtqueues are counted
by ->get_num_virtqueues() and we only add 1 for the Configuration Change
interrupt:

     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-        vpci_dev->nvectors = vs->conf.num_queues + 3;
+        vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1;
     }

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/core/machine.c               |  3 +++
 hw/scsi/vhost-scsi.c            |  2 ++
 hw/scsi/vhost-user-scsi.c       |  2 ++
 hw/scsi/virtio-scsi.c           | 18 ++++++++++++++++++
 hw/virtio/vhost-scsi-pci.c      |  4 ++--
 hw/virtio/vhost-user-scsi-pci.c |  4 ++--
 hw/virtio/virtio-scsi-pci.c     |  4 ++--
 include/hw/virtio/virtio-scsi.h |  1 +
 8 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 3e288bfceb..41da19d85b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -30,8 +30,11 @@
 GlobalProperty hw_compat_4_2[] = {
     { "virtio-blk-device", "x-enable-wce-if-config-wce", "off" },
     { "virtio-blk-device", "seg-max-adjust", "off"},
+    { "virtio-scsi-device", "auto_num_queues", "off"},
     { "virtio-scsi-device", "seg_max_adjust", "off"},
     { "vhost-blk-device", "seg_max_adjust", "off"},
+    { "vhost-scsi", "auto_num_queues", "off"},
+    { "vhost-user-scsi", "auto_num_queues", "off"},
     { "usb-host", "suppress-remote-wake", "off" },
     { "usb-redir", "suppress-remote-wake", "off" },
 };
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 26f710d3ec..1dfc269a29 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -273,6 +273,8 @@ static Property vhost_scsi_properties[] = {
     DEFINE_PROP_STRING("wwpn", VirtIOSCSICommon, conf.wwpn),
     DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt, 0),
     DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1),
+    DEFINE_PROP_BOOL("auto_num_queues", VirtIOSCSICommon,
+                     conf.auto_num_queues, true),
     DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size,
                        128),
     DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSICommon, conf.seg_max_adjust,
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index eb37733bd0..92dbdf1042 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -164,6 +164,8 @@ static Property vhost_user_scsi_properties[] = {
     DEFINE_PROP_CHR("chardev", VirtIOSCSICommon, conf.chardev),
     DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt, 0),
     DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1),
+    DEFINE_PROP_BOOL("auto_num_queues", VirtIOSCSICommon,
+                     conf.auto_num_queues, true),
     DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size,
                        128),
     DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors,
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 5622dd54e5..73c6030b22 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -16,6 +16,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "standard-headers/linux/virtio_ids.h"
+#include "hw/boards.h"
 #include "hw/virtio/virtio-scsi.h"
 #include "migration/qemu-file-types.h"
 #include "qemu/error-report.h"
@@ -878,6 +879,18 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = {
     .load_request = virtio_scsi_load_request,
 };
 
+static uint32_t virtio_scsi_get_num_virtqueues(VirtIODevice *vdev)
+{
+    VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
+    uint32_t request_queues = s->conf.num_queues;
+
+    if (s->conf.num_queues == 1 && s->conf.auto_num_queues) {
+        request_queues = current_machine->smp.cpus;
+    }
+
+    return VIRTIO_SCSI_VQ_NUM_FIXED + request_queues;
+}
+
 void virtio_scsi_common_realize(DeviceState *dev,
                                 VirtIOHandleOutput ctrl,
                                 VirtIOHandleOutput evt,
@@ -905,6 +918,8 @@ void virtio_scsi_common_realize(DeviceState *dev,
                    "must be > 2", s->conf.virtqueue_size);
         return;
     }
+    s->conf.num_queues = virtio_scsi_get_num_virtqueues(vdev) -
+                         VIRTIO_SCSI_VQ_NUM_FIXED;
     s->cmd_vqs = g_new0(VirtQueue *, s->conf.num_queues);
     s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
     s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
@@ -959,6 +974,8 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp)
 
 static Property virtio_scsi_properties[] = {
     DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1),
+    DEFINE_PROP_BOOL("auto_num_queues", VirtIOSCSI,
+                     parent_obj.conf.auto_num_queues, true),
     DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI,
                                          parent_obj.conf.virtqueue_size, 128),
     DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI,
@@ -1011,6 +1028,7 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data)
     vdc->reset = virtio_scsi_reset;
     vdc->start_ioeventfd = virtio_scsi_dataplane_start;
     vdc->stop_ioeventfd = virtio_scsi_dataplane_stop;
+    vdc->get_num_virtqueues = virtio_scsi_get_num_virtqueues;
     hc->pre_plug = virtio_scsi_pre_hotplug;
     hc->plug = virtio_scsi_hotplug;
     hc->unplug = virtio_scsi_hotunplug;
diff --git a/hw/virtio/vhost-scsi-pci.c b/hw/virtio/vhost-scsi-pci.c
index e8dfbfc60f..94f64afde4 100644
--- a/hw/virtio/vhost-scsi-pci.c
+++ b/hw/virtio/vhost-scsi-pci.c
@@ -47,10 +47,10 @@ static void vhost_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
 {
     VHostSCSIPCI *dev = VHOST_SCSI_PCI(vpci_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-        vpci_dev->nvectors = vs->conf.num_queues + 3;
+        vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1;
     }
 
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
diff --git a/hw/virtio/vhost-user-scsi-pci.c b/hw/virtio/vhost-user-scsi-pci.c
index ff13af7030..6b518e6d72 100644
--- a/hw/virtio/vhost-user-scsi-pci.c
+++ b/hw/virtio/vhost-user-scsi-pci.c
@@ -53,10 +53,10 @@ static void vhost_user_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
 {
     VHostUserSCSIPCI *dev = VHOST_USER_SCSI_PCI(vpci_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-        vpci_dev->nvectors = vs->conf.num_queues + 3;
+        vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1;
     }
 
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
index 3c55dc19a1..447344869d 100644
--- a/hw/virtio/virtio-scsi-pci.c
+++ b/hw/virtio/virtio-scsi-pci.c
@@ -46,12 +46,12 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
 {
     VirtIOSCSIPCI *dev = VIRTIO_SCSI_PCI(vpci_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
     DeviceState *proxy = DEVICE(vpci_dev);
     char *bus_name;
 
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-        vpci_dev->nvectors = vs->conf.num_queues + 3;
+        vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1;
     }
 
     /*
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 9f293bcb80..672e703bd2 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -50,6 +50,7 @@ typedef struct virtio_scsi_config VirtIOSCSIConfig;
 
 struct VirtIOSCSIConf {
     uint32_t num_queues;
+    bool auto_num_queues;
     uint32_t virtqueue_size;
     bool seg_max_adjust;
     uint32_t max_sectors;
-- 
2.24.1

Re: [PATCH 3/5] virtio-scsi: default num_queues to -smp N

[Cornelia Huck]

On Thu, 16 Jan 2020 10:58:40 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Automatically size the number of request virtqueues to match the number
> of vCPUs.  This ensures that completion interrupts are handled on the
> same vCPU that submitted the request.  No IPI is necessary to complete
> an I/O request and performance is improved.
> 
> Remember that virtqueue numbering assumptions are being removed from the
> virtio-pci proxy object, so the Control and Event virtqueues are counted
> by ->get_num_virtqueues() and we only add 1 for the Configuration Change
> interrupt:
> 
>      if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> -        vpci_dev->nvectors = vs->conf.num_queues + 3;
> +        vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1;
>      }
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/core/machine.c               |  3 +++
>  hw/scsi/vhost-scsi.c            |  2 ++
>  hw/scsi/vhost-user-scsi.c       |  2 ++
>  hw/scsi/virtio-scsi.c           | 18 ++++++++++++++++++
>  hw/virtio/vhost-scsi-pci.c      |  4 ++--
>  hw/virtio/vhost-user-scsi-pci.c |  4 ++--
>  hw/virtio/virtio-scsi-pci.c     |  4 ++--
>  include/hw/virtio/virtio-scsi.h |  1 +
>  8 files changed, 32 insertions(+), 6 deletions(-)
> 

> @@ -878,6 +879,18 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = {
>      .load_request = virtio_scsi_load_request,
>  };
>  
> +static uint32_t virtio_scsi_get_num_virtqueues(VirtIODevice *vdev)
> +{
> +    VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
> +    uint32_t request_queues = s->conf.num_queues;
> +
> +    if (s->conf.num_queues == 1 && s->conf.auto_num_queues) {
> +        request_queues = current_machine->smp.cpus;
> +    }
> +
> +    return VIRTIO_SCSI_VQ_NUM_FIXED + request_queues;

I'm not sure doing this at the device level is the right thing to do.
For now, only virtio-pci will call this function, and there basing the
number of virtqueues off the number of cpus makes sense; but that's a
property of the transport, not of the device.

Consider e.g. a virtio-scsi-ccw device: If we wanted to introduce a way
to automatically pick a good number of virtqueues there, this functions
likely would not return a particularly useful value, as queue interrupt
processing does not really relate to the number of cpus with adapter
interrupts. It's not a problem right now, as only virtio-pci calls
this, but someone looking at this callback is likely getting the
impression that this is a generically useful function.

> +}
> +
>  void virtio_scsi_common_realize(DeviceState *dev,
>                                  VirtIOHandleOutput ctrl,
>                                  VirtIOHandleOutput evt,

Re: [PATCH 3/5] virtio-scsi: default num_queues to -smp N

[Stefan Hajnoczi]

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

On Thu, Jan 16, 2020 at 12:53:49PM +0100, Cornelia Huck wrote:
> On Thu, 16 Jan 2020 10:58:40 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > Automatically size the number of request virtqueues to match the number
> > of vCPUs.  This ensures that completion interrupts are handled on the
> > same vCPU that submitted the request.  No IPI is necessary to complete
> > an I/O request and performance is improved.
> > 
> > Remember that virtqueue numbering assumptions are being removed from the
> > virtio-pci proxy object, so the Control and Event virtqueues are counted
> > by ->get_num_virtqueues() and we only add 1 for the Configuration Change
> > interrupt:
> > 
> >      if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> > -        vpci_dev->nvectors = vs->conf.num_queues + 3;
> > +        vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1;
> >      }
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  hw/core/machine.c               |  3 +++
> >  hw/scsi/vhost-scsi.c            |  2 ++
> >  hw/scsi/vhost-user-scsi.c       |  2 ++
> >  hw/scsi/virtio-scsi.c           | 18 ++++++++++++++++++
> >  hw/virtio/vhost-scsi-pci.c      |  4 ++--
> >  hw/virtio/vhost-user-scsi-pci.c |  4 ++--
> >  hw/virtio/virtio-scsi-pci.c     |  4 ++--
> >  include/hw/virtio/virtio-scsi.h |  1 +
> >  8 files changed, 32 insertions(+), 6 deletions(-)
> > 
> 
> > @@ -878,6 +879,18 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = {
> >      .load_request = virtio_scsi_load_request,
> >  };
> >  
> > +static uint32_t virtio_scsi_get_num_virtqueues(VirtIODevice *vdev)
> > +{
> > +    VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
> > +    uint32_t request_queues = s->conf.num_queues;
> > +
> > +    if (s->conf.num_queues == 1 && s->conf.auto_num_queues) {
> > +        request_queues = current_machine->smp.cpus;
> > +    }
> > +
> > +    return VIRTIO_SCSI_VQ_NUM_FIXED + request_queues;
> 
> I'm not sure doing this at the device level is the right thing to do.
> For now, only virtio-pci will call this function, and there basing the
> number of virtqueues off the number of cpus makes sense; but that's a
> property of the transport, not of the device.
> 
> Consider e.g. a virtio-scsi-ccw device: If we wanted to introduce a way
> to automatically pick a good number of virtqueues there, this functions
> likely would not return a particularly useful value, as queue interrupt
> processing does not really relate to the number of cpus with adapter
> interrupts. It's not a problem right now, as only virtio-pci calls
> this, but someone looking at this callback is likely getting the
> impression that this is a generically useful function.

That's a great point.  Maybe the direction should be reversed so that
the device asks the transport to suggest the optimal number of queues
when auto-num-queues is enabled.

Stefan

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

Re: [PATCH 3/5] virtio-scsi: default num_queues to -smp N

[Cornelia Huck]

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

On Thu, 16 Jan 2020 14:16:54 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Thu, Jan 16, 2020 at 12:53:49PM +0100, Cornelia Huck wrote:
> > On Thu, 16 Jan 2020 10:58:40 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >   
> > > Automatically size the number of request virtqueues to match the number
> > > of vCPUs.  This ensures that completion interrupts are handled on the
> > > same vCPU that submitted the request.  No IPI is necessary to complete
> > > an I/O request and performance is improved.
> > > 
> > > Remember that virtqueue numbering assumptions are being removed from the
> > > virtio-pci proxy object, so the Control and Event virtqueues are counted
> > > by ->get_num_virtqueues() and we only add 1 for the Configuration Change
> > > interrupt:
> > > 
> > >      if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> > > -        vpci_dev->nvectors = vs->conf.num_queues + 3;
> > > +        vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1;
> > >      }
> > > 
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  hw/core/machine.c               |  3 +++
> > >  hw/scsi/vhost-scsi.c            |  2 ++
> > >  hw/scsi/vhost-user-scsi.c       |  2 ++
> > >  hw/scsi/virtio-scsi.c           | 18 ++++++++++++++++++
> > >  hw/virtio/vhost-scsi-pci.c      |  4 ++--
> > >  hw/virtio/vhost-user-scsi-pci.c |  4 ++--
> > >  hw/virtio/virtio-scsi-pci.c     |  4 ++--
> > >  include/hw/virtio/virtio-scsi.h |  1 +
> > >  8 files changed, 32 insertions(+), 6 deletions(-)
> > >   
> >   
> > > @@ -878,6 +879,18 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = {
> > >      .load_request = virtio_scsi_load_request,
> > >  };
> > >  
> > > +static uint32_t virtio_scsi_get_num_virtqueues(VirtIODevice *vdev)
> > > +{
> > > +    VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
> > > +    uint32_t request_queues = s->conf.num_queues;
> > > +
> > > +    if (s->conf.num_queues == 1 && s->conf.auto_num_queues) {
> > > +        request_queues = current_machine->smp.cpus;
> > > +    }
> > > +
> > > +    return VIRTIO_SCSI_VQ_NUM_FIXED + request_queues;  
> > 
> > I'm not sure doing this at the device level is the right thing to do.
> > For now, only virtio-pci will call this function, and there basing the
> > number of virtqueues off the number of cpus makes sense; but that's a
> > property of the transport, not of the device.
> > 
> > Consider e.g. a virtio-scsi-ccw device: If we wanted to introduce a way
> > to automatically pick a good number of virtqueues there, this functions
> > likely would not return a particularly useful value, as queue interrupt
> > processing does not really relate to the number of cpus with adapter
> > interrupts. It's not a problem right now, as only virtio-pci calls
> > this, but someone looking at this callback is likely getting the
> > impression that this is a generically useful function.  
> 
> That's a great point.  Maybe the direction should be reversed so that
> the device asks the transport to suggest the optimal number of queues
> when auto-num-queues is enabled.

Maybe the device also specifying that it needs at least m queues, and
the transport can then return max(m, n) (with n being the number of
cpus in the pci case).

My next problem is that I'm not sure what 'optimal number of queues'
would mean from a ccw viewpoint. "Remaining free bits in the indicator
area" will be way too much in the general case :) Giving an upper limit
is easy, but not a value that we'd want to autoconfigure, and I really
don't want to return 42 all the time. Would it make sense to make this
feature opt-in per transport?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/5] virtio-blk: default num_queues to -smp N

[Stefan Hajnoczi]

Automatically size the number of request virtqueues to match the number
of vCPUs.  This ensures that completion interrupts are handled on the
same vCPU that submitted the request.  No IPI is necessary to complete
an I/O request and performance is improved.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c          | 17 +++++++++++++++++
 hw/core/machine.c              |  1 +
 hw/virtio/virtio-blk-pci.c     |  3 ++-
 include/hw/virtio/virtio-blk.h |  1 +
 4 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 9bee514c4e..8966b5a571 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -18,6 +18,7 @@
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
 #include "trace.h"
+#include "hw/boards.h"
 #include "hw/block/block.h"
 #include "hw/qdev-properties.h"
 #include "sysemu/blockdev.h"
@@ -1119,6 +1120,18 @@ static const BlockDevOps virtio_block_ops = {
     .resize_cb = virtio_blk_resize,
 };
 
+static uint32_t virtio_blk_get_num_virtqueues(VirtIODevice *vdev)
+{
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
+    VirtIOBlkConf *conf = &s->conf;
+
+    if (conf->num_queues == 1 && conf->auto_num_queues) {
+        return current_machine->smp.cpus;
+    }
+
+    return conf->num_queues;
+}
+
 static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -1135,6 +1148,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "Device needs media, but drive is empty");
         return;
     }
+    conf->num_queues = virtio_blk_get_num_virtqueues(vdev);
     if (!conf->num_queues) {
         error_setg(errp, "num-queues property must be larger than 0");
         return;
@@ -1272,6 +1286,8 @@ static Property virtio_blk_properties[] = {
     DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0,
                     true),
     DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1),
+    DEFINE_PROP_BOOL("auto-num-queues", VirtIOBlock,
+                     conf.auto_num_queues, true),
     DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
     DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true),
     DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
@@ -1297,6 +1313,7 @@ static void virtio_blk_class_init(ObjectClass *klass, void *data)
     dc->props = virtio_blk_properties;
     dc->vmsd = &vmstate_virtio_blk;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+    vdc->get_num_virtqueues = virtio_blk_get_num_virtqueues;
     vdc->realize = virtio_blk_device_realize;
     vdc->unrealize = virtio_blk_device_unrealize;
     vdc->get_config = virtio_blk_update_config;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 41da19d85b..a33d1e93b1 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -28,6 +28,7 @@
 #include "hw/mem/nvdimm.h"
 
 GlobalProperty hw_compat_4_2[] = {
+    { "virtio-blk-device", "auto-num-queues", "off"},
     { "virtio-blk-device", "x-enable-wce-if-config-wce", "off" },
     { "virtio-blk-device", "seg-max-adjust", "off"},
     { "virtio-scsi-device", "auto_num_queues", "off"},
diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
index d9b69a5af3..5131955cec 100644
--- a/hw/virtio/virtio-blk-pci.c
+++ b/hw/virtio/virtio-blk-pci.c
@@ -50,9 +50,10 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
 {
     VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-        vpci_dev->nvectors = dev->vdev.conf.num_queues + 1;
+        vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1;
     }
 
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 1e62f869b2..375d7b920a 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -37,6 +37,7 @@ struct VirtIOBlkConf
     char *serial;
     uint32_t request_merging;
     uint16_t num_queues;
+    bool auto_num_queues;
     uint16_t queue_size;
     bool seg_max_adjust;
     uint32_t max_discard_sectors;
-- 
2.24.1

[PATCH 5/5] vhost-user-blk: default num_queues to -smp N

[Stefan Hajnoczi]

Automatically size the number of request virtqueues to match the number
of vCPUs.  This ensures that completion interrupts are handled on the
same vCPU that submitted the request.  No IPI is necessary to complete
an I/O request and performance is improved.

This patch duplicates the .get_num_virtqueues() function because
vhost-user-blk defines its own qdev property fields and does not share
VirtIOBlkConf.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/vhost-user-blk.c          | 15 +++++++++++++++
 hw/core/machine.c                  |  1 +
 hw/virtio/vhost-user-blk-pci.c     |  3 ++-
 include/hw/virtio/vhost-user-blk.h |  1 +
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 98b383f90e..ac61ce9835 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -21,6 +21,7 @@
 #include "qemu/error-report.h"
 #include "qemu/cutils.h"
 #include "qom/object.h"
+#include "hw/boards.h"
 #include "hw/qdev-core.h"
 #include "hw/qdev-properties.h"
 #include "hw/virtio/vhost.h"
@@ -391,6 +392,17 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
     }
 }
 
+static uint32_t vhost_user_blk_get_num_virtqueues(VirtIODevice *vdev)
+{
+    VHostUserBlk *s = VHOST_USER_BLK(vdev);
+
+    if (s->num_queues == 1 && s->auto_num_queues) {
+        return current_machine->smp.cpus;
+    }
+
+    return s->num_queues;
+}
+
 static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -403,6 +415,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    s->num_queues = vhost_user_blk_get_num_virtqueues(vdev);
     if (!s->num_queues || s->num_queues > VIRTIO_QUEUE_MAX) {
         error_setg(errp, "vhost-user-blk: invalid number of IO queues");
         return;
@@ -501,6 +514,7 @@ static const VMStateDescription vmstate_vhost_user_blk = {
 static Property vhost_user_blk_properties[] = {
     DEFINE_PROP_CHR("chardev", VHostUserBlk, chardev),
     DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues, 1),
+    DEFINE_PROP_BOOL("auto-num-queues", VHostUserBlk, auto_num_queues, true),
     DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
     DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
     DEFINE_PROP_END_OF_LIST(),
@@ -514,6 +528,7 @@ static void vhost_user_blk_class_init(ObjectClass *klass, void *data)
     dc->props = vhost_user_blk_properties;
     dc->vmsd = &vmstate_vhost_user_blk;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+    vdc->get_num_virtqueues = vhost_user_blk_get_num_virtqueues;
     vdc->realize = vhost_user_blk_device_realize;
     vdc->unrealize = vhost_user_blk_device_unrealize;
     vdc->get_config = vhost_user_blk_update_config;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index a33d1e93b1..0d4142c3c8 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -35,6 +35,7 @@ GlobalProperty hw_compat_4_2[] = {
     { "virtio-scsi-device", "seg_max_adjust", "off"},
     { "vhost-blk-device", "seg_max_adjust", "off"},
     { "vhost-scsi", "auto_num_queues", "off"},
+    { "vhost-user-blk", "auto-num-queues", "off"},
     { "vhost-user-scsi", "auto_num_queues", "off"},
     { "usb-host", "suppress-remote-wake", "off" },
     { "usb-redir", "suppress-remote-wake", "off" },
diff --git a/hw/virtio/vhost-user-blk-pci.c b/hw/virtio/vhost-user-blk-pci.c
index 1dc834a3ff..328c6a4066 100644
--- a/hw/virtio/vhost-user-blk-pci.c
+++ b/hw/virtio/vhost-user-blk-pci.c
@@ -53,9 +53,10 @@ static void vhost_user_blk_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
 {
     VHostUserBlkPCI *dev = VHOST_USER_BLK_PCI(vpci_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-        vpci_dev->nvectors = dev->vdev.num_queues + 1;
+        vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1;
     }
 
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h
index 108bfadeeb..09d1fdf722 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -31,6 +31,7 @@ typedef struct VHostUserBlk {
     int32_t bootindex;
     struct virtio_blk_config blkcfg;
     uint16_t num_queues;
+    bool auto_num_queues;
     uint32_t queue_size;
     uint32_t config_wce;
     struct vhost_dev dev;
-- 
2.24.1