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

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

[Stefan Hajnoczi]

v4:
 * Sorry for the long delay. I considered replacing this series with a simpler
   approach. Real hardware ships with a fixed number of queues (e.g. 128). The
   equivalent can be done in QEMU too. That way we don't need to magically size
   num_queues. In the end I decided against this approach because the Linux
   virtio_blk.ko and virtio_scsi.ko guest drivers unconditionally initialized
   all available queues until recently (it was written with
   num_queues=num_vcpus in mind). It doesn't make sense for a 1 CPU guest to
   bring up 128 virtqueues (waste of resources and possibly weird performance
   effects with blk-mq).
 * Honor maximum number of MSI-X vectors and virtqueues [Daniel Berrange]
 * Update commit descriptions to mention maximum MSI-X vector and virtqueue
   caps [Raphael]
v3:
 * Introduce virtio_pci_optimal_num_queues() helper to enforce VIRTIO_QUEUE_MAX
   in one place
 * Use VIRTIO_SCSI_VQ_NUM_FIXED constant in all cases [Cornelia]
 * Update hw/core/machine.c compat properties for QEMU 5.0 [Michael]
v3:
 * Add new performance results that demonstrate the scalability
 * Mention that this is PCI-specific [Cornelia]
v2:
 * Let the virtio-DEVICE-pci device select num-queues because the optimal
   multi-queue configuration may differ between virtio-pci, virtio-mmio, and
   virtio-ccw [Cornelia]

Enabling multi-queue on virtio-pci storage devices improves 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 on virtio-blk and
virtio-scsi PCI devices.  Older machine types continue to default to 1 queue
for live migration compatibility.

Random read performance:
      IOPS
q=1    78k
q=32  104k  +33%

Boot time:
      Duration
q=1        51s
q=32     1m41s  +98%

Guest configuration: 32 vCPUs, 101 virtio-blk-pci disks

Previously measured results on a 4 vCPU guest were also positive but showed a
smaller 1-4% performance improvement.  They are no longer valid because
significant event loop optimizations have been merged.

Stefan Hajnoczi (5):
  virtio-pci: add virtio_pci_optimal_num_queues() helper
  virtio-scsi: introduce a constant for fixed 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/virtio/virtio-pci.h             |  9 +++++++++
 include/hw/virtio/vhost-user-blk.h |  2 ++
 include/hw/virtio/virtio-blk.h     |  2 ++
 include/hw/virtio/virtio-scsi.h    |  5 +++++
 hw/block/vhost-user-blk.c          |  6 +++++-
 hw/block/virtio-blk.c              |  6 +++++-
 hw/core/machine.c                  |  5 +++++
 hw/scsi/vhost-scsi.c               |  3 ++-
 hw/scsi/vhost-user-scsi.c          |  5 +++--
 hw/scsi/virtio-scsi.c              | 13 ++++++++----
 hw/virtio/vhost-scsi-pci.c         |  9 +++++++--
 hw/virtio/vhost-user-blk-pci.c     |  4 ++++
 hw/virtio/vhost-user-scsi-pci.c    |  9 +++++++--
 hw/virtio/virtio-blk-pci.c         |  7 ++++++-
 hw/virtio/virtio-pci.c             | 32 ++++++++++++++++++++++++++++++
 hw/virtio/virtio-scsi-pci.c        |  9 +++++++--
 16 files changed, 110 insertions(+), 16 deletions(-)

-- 
2.26.2

[PATCH v5 1/5] virtio-pci: add virtio_pci_optimal_num_queues() helper

[Stefan Hajnoczi]

Multi-queue devices achieve the best performance when each vCPU has a
dedicated queue. This ensures that virtqueue used notifications are
handled on the same vCPU that submitted virtqueue buffers.  When another
vCPU handles the the notification an IPI will be necessary to wake the
submission vCPU and this incurs a performance overhead.

Provide a helper function that virtio-pci devices will use in later
patches to automatically select the optimal number of queues.

The function handles guests with large numbers of CPUs by limiting the
number of queues to fit within the following constraints:
1. The maximum number of MSI-X vectors.
2. The maximum number of virtqueues.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio-pci.h |  9 +++++++++
 hw/virtio/virtio-pci.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index e2eaaa9182..91096f0291 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -243,4 +243,13 @@ typedef struct VirtioPCIDeviceTypeInfo {
 /* Register virtio-pci type(s).  @t must be static. */
 void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t);
 
+/**
+ * virtio_pci_optimal_num_queues:
+ * @fixed_queues: number of queues that are always present
+ *
+ * Returns: The optimal number of queues for a multi-queue device, excluding
+ * @fixed_queues.
+ */
+unsigned virtio_pci_optimal_num_queues(unsigned fixed_queues);
+
 #endif
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 7bc8c1c056..c48570c03f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -19,6 +19,7 @@
 
 #include "exec/memop.h"
 #include "standard-headers/linux/virtio_pci.h"
+#include "hw/boards.h"
 #include "hw/virtio/virtio.h"
 #include "migration/qemu-file-types.h"
 #include "hw/pci/pci.h"
@@ -2028,6 +2029,37 @@ void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t)
     g_free(base_name);
 }
 
+unsigned virtio_pci_optimal_num_queues(unsigned fixed_queues)
+{
+    /*
+     * 1:1 vq to vCPU mapping is ideal because the same vCPU that submitted
+     * virtqueue buffers can handle their completion. When a different vCPU
+     * handles completion it may need to IPI the vCPU that submitted the
+     * request and this adds overhead.
+     *
+     * Virtqueues consume guest RAM and MSI-X vectors. This is wasteful in
+     * guests with very many vCPUs and a device that is only used by a few
+     * vCPUs. Unfortunately optimizing that case requires manual pinning inside
+     * the guest, so those users might as well manually set the number of
+     * queues. There is no upper limit that can be applied automatically and
+     * doing so arbitrarily would result in a sudden performance drop once the
+     * threshold number of vCPUs is exceeded.
+     */
+    unsigned num_queues = current_machine->smp.cpus;
+
+    /*
+     * The maximum number of MSI-X vectors is PCI_MSIX_FLAGS_QSIZE + 1, but the
+     * config change interrupt and the fixed virtqueues must be taken into
+     * account too.
+     */
+    num_queues = MIN(num_queues, PCI_MSIX_FLAGS_QSIZE - fixed_queues);
+
+    /*
+     * There is a limit to how many virtqueues a device can have.
+     */
+    return MIN(num_queues, VIRTIO_QUEUE_MAX - fixed_queues);
+}
+
 /* virtio-pci-bus */
 
 static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
-- 
2.26.2

[PATCH v5 2/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>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
---
 include/hw/virtio/virtio-scsi.h | 3 +++
 hw/scsi/vhost-user-scsi.c       | 2 +-
 hw/scsi/virtio-scsi.c           | 7 ++++---
 hw/virtio/vhost-scsi-pci.c      | 3 ++-
 hw/virtio/vhost-user-scsi-pci.c | 3 ++-
 hw/virtio/virtio-scsi-pci.c     | 3 ++-
 6 files changed, 14 insertions(+), 7 deletions(-)

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;
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index f2e524438a..a8b821466f 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -114,7 +114,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 b49775269e..eecdd05af5 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/hw/virtio/vhost-scsi-pci.c b/hw/virtio/vhost-scsi-pci.c
index 095af23f3f..06e814d30e 100644
--- a/hw/virtio/vhost-scsi-pci.c
+++ b/hw/virtio/vhost-scsi-pci.c
@@ -50,7 +50,8 @@ static void vhost_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
 
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-        vpci_dev->nvectors = vs->conf.num_queues + 3;
+        vpci_dev->nvectors = vs->conf.num_queues +
+                             VIRTIO_SCSI_VQ_NUM_FIXED + 1;
     }
 
     qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
diff --git a/hw/virtio/vhost-user-scsi-pci.c b/hw/virtio/vhost-user-scsi-pci.c
index 4705cd54e8..ab6dfb71a9 100644
--- a/hw/virtio/vhost-user-scsi-pci.c
+++ b/hw/virtio/vhost-user-scsi-pci.c
@@ -56,7 +56,8 @@ static void vhost_user_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
 
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-        vpci_dev->nvectors = vs->conf.num_queues + 3;
+        vpci_dev->nvectors = vs->conf.num_queues +
+                             VIRTIO_SCSI_VQ_NUM_FIXED + 1;
     }
 
     qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
index c23a134202..3ff9eb7ef6 100644
--- a/hw/virtio/virtio-scsi-pci.c
+++ b/hw/virtio/virtio-scsi-pci.c
@@ -51,7 +51,8 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     char *bus_name;
 
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-        vpci_dev->nvectors = vs->conf.num_queues + 3;
+        vpci_dev->nvectors = vs->conf.num_queues +
+                             VIRTIO_SCSI_VQ_NUM_FIXED + 1;
     }
 
     /*
-- 
2.26.2

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

[Stefan Hajnoczi]

Automatically size the number of virtio-scsi-pci, vhost-scsi-pci, and
vhost-user-scsi-pci request virtqueues to match the number of vCPUs.
Other transports continue to default to 1 request virtqueue.

A 1:1 virtqueue:vCPU mapping 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.  The
maximum number of MSI-X vectors and virtqueues limit are respected.

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

diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 9f293bcb80..c0b8e4dd7e 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -39,6 +39,8 @@
 /* Number of virtqueues that are always present */
 #define VIRTIO_SCSI_VQ_NUM_FIXED    2
 
+#define VIRTIO_SCSI_AUTO_NUM_QUEUES UINT32_MAX
+
 typedef struct virtio_scsi_cmd_req VirtIOSCSICmdReq;
 typedef struct virtio_scsi_cmd_resp VirtIOSCSICmdResp;
 typedef struct virtio_scsi_ctrl_tmf_req VirtIOSCSICtrlTMFReq;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 211b4e077a..3df534405b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -29,7 +29,10 @@
 #include "migration/vmstate.h"
 
 GlobalProperty hw_compat_5_0[] = {
+    { "vhost-scsi", "num_queues", "1"},
+    { "vhost-user-scsi", "num_queues", "1"},
     { "virtio-balloon-device", "page-poison", "false" },
+    { "virtio-scsi-device", "num_queues", "1"},
     { "vmport", "x-read-set-eax", "off" },
     { "vmport", "x-signal-unsupported-cmd", "off" },
     { "vmport", "x-report-vmx-type", "off" },
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index c1b012aea4..5e3bc319ab 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -272,7 +272,8 @@ static Property vhost_scsi_properties[] = {
     DEFINE_PROP_STRING("vhostfd", VirtIOSCSICommon, conf.vhostfd),
     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_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues,
+                       VIRTIO_SCSI_AUTO_NUM_QUEUES),
     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 a8b821466f..7c0631656c 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -162,7 +162,8 @@ static void vhost_user_scsi_unrealize(DeviceState *dev)
 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_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues,
+                       VIRTIO_SCSI_AUTO_NUM_QUEUES),
     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 eecdd05af5..3a71ea7097 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -891,6 +891,9 @@ void virtio_scsi_common_realize(DeviceState *dev,
     virtio_init(vdev, "virtio-scsi", VIRTIO_ID_SCSI,
                 sizeof(VirtIOSCSIConfig));
 
+    if (s->conf.num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
+        s->conf.num_queues = 1;
+    }
     if (s->conf.num_queues == 0 ||
             s->conf.num_queues > VIRTIO_QUEUE_MAX - VIRTIO_SCSI_VQ_NUM_FIXED) {
         error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
@@ -964,7 +967,8 @@ static void virtio_scsi_device_unrealize(DeviceState *dev)
 }
 
 static Property virtio_scsi_properties[] = {
-    DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1),
+    DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues,
+                       VIRTIO_SCSI_AUTO_NUM_QUEUES),
     DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI,
                                          parent_obj.conf.virtqueue_size, 256),
     DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI,
diff --git a/hw/virtio/vhost-scsi-pci.c b/hw/virtio/vhost-scsi-pci.c
index 06e814d30e..a6bb0dc60d 100644
--- a/hw/virtio/vhost-scsi-pci.c
+++ b/hw/virtio/vhost-scsi-pci.c
@@ -47,11 +47,15 @@ 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);
+    VirtIOSCSIConf *conf = &dev->vdev.parent_obj.parent_obj.conf;
+
+    if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
+        conf->num_queues =
+            virtio_pci_optimal_num_queues(VIRTIO_SCSI_VQ_NUM_FIXED);
+    }
 
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-        vpci_dev->nvectors = vs->conf.num_queues +
-                             VIRTIO_SCSI_VQ_NUM_FIXED + 1;
+        vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 1;
     }
 
     qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
diff --git a/hw/virtio/vhost-user-scsi-pci.c b/hw/virtio/vhost-user-scsi-pci.c
index ab6dfb71a9..25e97ca54e 100644
--- a/hw/virtio/vhost-user-scsi-pci.c
+++ b/hw/virtio/vhost-user-scsi-pci.c
@@ -53,11 +53,15 @@ 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);
+    VirtIOSCSIConf *conf = &dev->vdev.parent_obj.parent_obj.conf;
+
+    if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
+        conf->num_queues =
+            virtio_pci_optimal_num_queues(VIRTIO_SCSI_VQ_NUM_FIXED);
+    }
 
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-        vpci_dev->nvectors = vs->conf.num_queues +
-                             VIRTIO_SCSI_VQ_NUM_FIXED + 1;
+        vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 1;
     }
 
     qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
index 3ff9eb7ef6..fa4b3bfb50 100644
--- a/hw/virtio/virtio-scsi-pci.c
+++ b/hw/virtio/virtio-scsi-pci.c
@@ -46,13 +46,17 @@ 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);
     DeviceState *proxy = DEVICE(vpci_dev);
+    VirtIOSCSIConf *conf = &dev->vdev.parent_obj.conf;
     char *bus_name;
 
+    if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
+        conf->num_queues =
+            virtio_pci_optimal_num_queues(VIRTIO_SCSI_VQ_NUM_FIXED);
+    }
+
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-        vpci_dev->nvectors = vs->conf.num_queues +
-                             VIRTIO_SCSI_VQ_NUM_FIXED + 1;
+        vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 1;
     }
 
     /*
-- 
2.26.2

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

[Stefan Hajnoczi]

Automatically size the number of virtio-blk-pci request virtqueues to
match the number of vCPUs.  Other transports continue to default to 1
request virtqueue.

A 1:1 virtqueue:vCPU mapping 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.  The
maximum number of MSI-X vectors and virtqueues limit are respected.

Performance improves from 78k to 104k IOPS on a 32 vCPU guest with 101
virtio-blk-pci devices (ioengine=libaio, iodepth=1, bs=4k, rw=randread
with NVMe storage).

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
---
 include/hw/virtio/virtio-blk.h | 2 ++
 hw/block/virtio-blk.c          | 6 +++++-
 hw/core/machine.c              | 1 +
 hw/virtio/virtio-blk-pci.c     | 7 ++++++-
 4 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index b1334c3904..7539c2b848 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -30,6 +30,8 @@ struct virtio_blk_inhdr
     unsigned char status;
 };
 
+#define VIRTIO_BLK_AUTO_NUM_QUEUES UINT16_MAX
+
 struct VirtIOBlkConf
 {
     BlockConf conf;
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 413783693c..2204ba149e 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1147,6 +1147,9 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "Device needs media, but drive is empty");
         return;
     }
+    if (conf->num_queues == VIRTIO_BLK_AUTO_NUM_QUEUES) {
+        conf->num_queues = 1;
+    }
     if (!conf->num_queues) {
         error_setg(errp, "num-queues property must be larger than 0");
         return;
@@ -1281,7 +1284,8 @@ static Property virtio_blk_properties[] = {
 #endif
     DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0,
                     true),
-    DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1),
+    DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues,
+                       VIRTIO_BLK_AUTO_NUM_QUEUES),
     DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 256),
     DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true),
     DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 3df534405b..845f6476cb 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -32,6 +32,7 @@ GlobalProperty hw_compat_5_0[] = {
     { "vhost-scsi", "num_queues", "1"},
     { "vhost-user-scsi", "num_queues", "1"},
     { "virtio-balloon-device", "page-poison", "false" },
+    { "virtio-blk-device", "num-queues", "1"},
     { "virtio-scsi-device", "num_queues", "1"},
     { "vmport", "x-read-set-eax", "off" },
     { "vmport", "x-signal-unsupported-cmd", "off" },
diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
index 849cc7dfd8..37c6e0aeb4 100644
--- a/hw/virtio/virtio-blk-pci.c
+++ b/hw/virtio/virtio-blk-pci.c
@@ -50,9 +50,14 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
 {
     VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
+    VirtIOBlkConf *conf = &dev->vdev.conf;
+
+    if (conf->num_queues == VIRTIO_BLK_AUTO_NUM_QUEUES) {
+        conf->num_queues = virtio_pci_optimal_num_queues(0);
+    }
 
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-        vpci_dev->nvectors = dev->vdev.conf.num_queues + 1;
+        vpci_dev->nvectors = conf->num_queues + 1;
     }
 
     qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
-- 
2.26.2

[PATCH v5 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.  The maximum number of MSI-X
vectors and virtqueues limit are respected.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 include/hw/virtio/vhost-user-blk.h | 2 ++
 hw/block/vhost-user-blk.c          | 6 +++++-
 hw/core/machine.c                  | 1 +
 hw/virtio/vhost-user-blk-pci.c     | 4 ++++
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h
index 34ad6f0c0e..292d17147c 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -25,6 +25,8 @@
 #define VHOST_USER_BLK(obj) \
         OBJECT_CHECK(VHostUserBlk, (obj), TYPE_VHOST_USER_BLK)
 
+#define VHOST_USER_BLK_AUTO_NUM_QUEUES UINT16_MAX
+
 typedef struct VHostUserBlk {
     VirtIODevice parent_obj;
     CharBackend chardev;
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index a00b854736..39aec42dae 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -420,6 +420,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (s->num_queues == VHOST_USER_BLK_AUTO_NUM_QUEUES) {
+        s->num_queues = 1;
+    }
     if (!s->num_queues || s->num_queues > VIRTIO_QUEUE_MAX) {
         error_setg(errp, "vhost-user-blk: invalid number of IO queues");
         return;
@@ -531,7 +534,8 @@ 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_UINT16("num-queues", VHostUserBlk, num_queues,
+                       VHOST_USER_BLK_AUTO_NUM_QUEUES),
     DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
     DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 845f6476cb..31bfaacdb5 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -30,6 +30,7 @@
 
 GlobalProperty hw_compat_5_0[] = {
     { "vhost-scsi", "num_queues", "1"},
+    { "vhost-user-blk", "num-queues", "1"},
     { "vhost-user-scsi", "num_queues", "1"},
     { "virtio-balloon-device", "page-poison", "false" },
     { "virtio-blk-device", "num-queues", "1"},
diff --git a/hw/virtio/vhost-user-blk-pci.c b/hw/virtio/vhost-user-blk-pci.c
index 4f5d5cbf44..a62a71e067 100644
--- a/hw/virtio/vhost-user-blk-pci.c
+++ b/hw/virtio/vhost-user-blk-pci.c
@@ -54,6 +54,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);
 
+    if (dev->vdev.num_queues == VHOST_USER_BLK_AUTO_NUM_QUEUES) {
+        dev->vdev.num_queues = virtio_pci_optimal_num_queues(0);
+    }
+
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
         vpci_dev->nvectors = dev->vdev.num_queues + 1;
     }
-- 
2.26.2

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

[Cornelia Huck]

On Mon,  6 Jul 2020 14:56:48 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

Maybe mention 'pci' in the subject as well?

> Automatically size the number of virtio-scsi-pci, vhost-scsi-pci, and
> vhost-user-scsi-pci request virtqueues to match the number of vCPUs.
> Other transports continue to default to 1 request virtqueue.
> 
> A 1:1 virtqueue:vCPU mapping 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.  The
> maximum number of MSI-X vectors and virtqueues limit are respected.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/hw/virtio/virtio-scsi.h |  2 ++
>  hw/core/machine.c               |  3 +++
>  hw/scsi/vhost-scsi.c            |  3 ++-
>  hw/scsi/vhost-user-scsi.c       |  3 ++-
>  hw/scsi/virtio-scsi.c           |  6 +++++-
>  hw/virtio/vhost-scsi-pci.c      | 10 +++++++---
>  hw/virtio/vhost-user-scsi-pci.c | 10 +++++++---
>  hw/virtio/virtio-scsi-pci.c     | 10 +++++++---
>  8 files changed, 35 insertions(+), 12 deletions(-)

(...)

> diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
> index 3ff9eb7ef6..fa4b3bfb50 100644
> --- a/hw/virtio/virtio-scsi-pci.c
> +++ b/hw/virtio/virtio-scsi-pci.c
> @@ -46,13 +46,17 @@ 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);
>      DeviceState *proxy = DEVICE(vpci_dev);
> +    VirtIOSCSIConf *conf = &dev->vdev.parent_obj.conf;
>      char *bus_name;
>  
> +    if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
> +        conf->num_queues =
> +            virtio_pci_optimal_num_queues(VIRTIO_SCSI_VQ_NUM_FIXED);
> +    }
> +
>      if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> -        vpci_dev->nvectors = vs->conf.num_queues +
> -                             VIRTIO_SCSI_VQ_NUM_FIXED + 1;
> +        vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 1;
>      }
>  
>      /*

One corner case where the setup may end up being a bit odd is a
situation where nvectors was specified, but num_queues was not, and the
device suddenly ends up with more queues than vectors. But I don't see
a reason why you would want to specify nvectors but not num_queues in a
real word scenario, so I think we can ignore that corner case.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v5 1/5] virtio-pci: add virtio_pci_optimal_num_queues() helper

[Cornelia Huck]

On Mon,  6 Jul 2020 14:56:46 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Multi-queue devices achieve the best performance when each vCPU has a
> dedicated queue. This ensures that virtqueue used notifications are
> handled on the same vCPU that submitted virtqueue buffers.  When another
> vCPU handles the the notification an IPI will be necessary to wake the
> submission vCPU and this incurs a performance overhead.
> 
> Provide a helper function that virtio-pci devices will use in later
> patches to automatically select the optimal number of queues.
> 
> The function handles guests with large numbers of CPUs by limiting the
> number of queues to fit within the following constraints:
> 1. The maximum number of MSI-X vectors.
> 2. The maximum number of virtqueues.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio-pci.h |  9 +++++++++
>  hw/virtio/virtio-pci.c | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)

I guess this should honour all relevant limits now.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

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

[Michael S. Tsirkin]

On Mon, Jul 06, 2020 at 02:56:45PM +0100, Stefan Hajnoczi wrote:
> v4:
>  * Sorry for the long delay. I considered replacing this series with a simpler
>    approach. Real hardware ships with a fixed number of queues (e.g. 128). The
>    equivalent can be done in QEMU too. That way we don't need to magically size
>    num_queues. In the end I decided against this approach because the Linux
>    virtio_blk.ko and virtio_scsi.ko guest drivers unconditionally initialized
>    all available queues until recently (it was written with
>    num_queues=num_vcpus in mind). It doesn't make sense for a 1 CPU guest to
>    bring up 128 virtqueues (waste of resources and possibly weird performance
>    effects with blk-mq).
>  * Honor maximum number of MSI-X vectors and virtqueues [Daniel Berrange]
>  * Update commit descriptions to mention maximum MSI-X vector and virtqueue
>    caps [Raphael]
> v3:
>  * Introduce virtio_pci_optimal_num_queues() helper to enforce VIRTIO_QUEUE_MAX
>    in one place
>  * Use VIRTIO_SCSI_VQ_NUM_FIXED constant in all cases [Cornelia]
>  * Update hw/core/machine.c compat properties for QEMU 5.0 [Michael]
> v3:
>  * Add new performance results that demonstrate the scalability
>  * Mention that this is PCI-specific [Cornelia]
> v2:
>  * Let the virtio-DEVICE-pci device select num-queues because the optimal
>    multi-queue configuration may differ between virtio-pci, virtio-mmio, and
>    virtio-ccw [Cornelia]
> 
> Enabling multi-queue on virtio-pci storage devices improves 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 on virtio-blk and
> virtio-scsi PCI devices.  Older machine types continue to default to 1 queue
> for live migration compatibility.
> 
> Random read performance:
>       IOPS
> q=1    78k
> q=32  104k  +33%
> 
> Boot time:
>       Duration
> q=1        51s
> q=32     1m41s  +98%
> 
> Guest configuration: 32 vCPUs, 101 virtio-blk-pci disks
> 
> Previously measured results on a 4 vCPU guest were also positive but showed a
> smaller 1-4% performance improvement.  They are no longer valid because
> significant event loop optimizations have been merged.

I'm guessing this should be deferred to the next release as
it (narrowly) missed the freeze window. Does this make sense to you?

> Stefan Hajnoczi (5):
>   virtio-pci: add virtio_pci_optimal_num_queues() helper
>   virtio-scsi: introduce a constant for fixed 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/virtio/virtio-pci.h             |  9 +++++++++
>  include/hw/virtio/vhost-user-blk.h |  2 ++
>  include/hw/virtio/virtio-blk.h     |  2 ++
>  include/hw/virtio/virtio-scsi.h    |  5 +++++
>  hw/block/vhost-user-blk.c          |  6 +++++-
>  hw/block/virtio-blk.c              |  6 +++++-
>  hw/core/machine.c                  |  5 +++++
>  hw/scsi/vhost-scsi.c               |  3 ++-
>  hw/scsi/vhost-user-scsi.c          |  5 +++--
>  hw/scsi/virtio-scsi.c              | 13 ++++++++----
>  hw/virtio/vhost-scsi-pci.c         |  9 +++++++--
>  hw/virtio/vhost-user-blk-pci.c     |  4 ++++
>  hw/virtio/vhost-user-scsi-pci.c    |  9 +++++++--
>  hw/virtio/virtio-blk-pci.c         |  7 ++++++-
>  hw/virtio/virtio-pci.c             | 32 ++++++++++++++++++++++++++++++
>  hw/virtio/virtio-scsi-pci.c        |  9 +++++++--
>  16 files changed, 110 insertions(+), 16 deletions(-)
> 
> -- 
> 2.26.2
> 

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

[Stefan Hajnoczi]

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

On Wed, Jul 08, 2020 at 06:59:41AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jul 06, 2020 at 02:56:45PM +0100, Stefan Hajnoczi wrote:
> > v4:
> >  * Sorry for the long delay. I considered replacing this series with a simpler
> >    approach. Real hardware ships with a fixed number of queues (e.g. 128). The
> >    equivalent can be done in QEMU too. That way we don't need to magically size
> >    num_queues. In the end I decided against this approach because the Linux
> >    virtio_blk.ko and virtio_scsi.ko guest drivers unconditionally initialized
> >    all available queues until recently (it was written with
> >    num_queues=num_vcpus in mind). It doesn't make sense for a 1 CPU guest to
> >    bring up 128 virtqueues (waste of resources and possibly weird performance
> >    effects with blk-mq).
> >  * Honor maximum number of MSI-X vectors and virtqueues [Daniel Berrange]
> >  * Update commit descriptions to mention maximum MSI-X vector and virtqueue
> >    caps [Raphael]
> > v3:
> >  * Introduce virtio_pci_optimal_num_queues() helper to enforce VIRTIO_QUEUE_MAX
> >    in one place
> >  * Use VIRTIO_SCSI_VQ_NUM_FIXED constant in all cases [Cornelia]
> >  * Update hw/core/machine.c compat properties for QEMU 5.0 [Michael]
> > v3:
> >  * Add new performance results that demonstrate the scalability
> >  * Mention that this is PCI-specific [Cornelia]
> > v2:
> >  * Let the virtio-DEVICE-pci device select num-queues because the optimal
> >    multi-queue configuration may differ between virtio-pci, virtio-mmio, and
> >    virtio-ccw [Cornelia]
> > 
> > Enabling multi-queue on virtio-pci storage devices improves 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 on virtio-blk and
> > virtio-scsi PCI devices.  Older machine types continue to default to 1 queue
> > for live migration compatibility.
> > 
> > Random read performance:
> >       IOPS
> > q=1    78k
> > q=32  104k  +33%
> > 
> > Boot time:
> >       Duration
> > q=1        51s
> > q=32     1m41s  +98%
> > 
> > Guest configuration: 32 vCPUs, 101 virtio-blk-pci disks
> > 
> > Previously measured results on a 4 vCPU guest were also positive but showed a
> > smaller 1-4% performance improvement.  They are no longer valid because
> > significant event loop optimizations have been merged.
> 
> I'm guessing this should be deferred to the next release as
> it (narrowly) missed the freeze window. Does this make sense to you?

Yes, that is fine. Thanks!

Stefan

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

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

[Stefan Hajnoczi]

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

On Tue, Jul 07, 2020 at 05:44:53PM +0200, Cornelia Huck wrote:
> On Mon,  6 Jul 2020 14:56:48 +0100
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> Maybe mention 'pci' in the subject as well?

I think this patch does too many things. I'll split up the generic and
PCI parts so that the commit message is more accurate.

> > Automatically size the number of virtio-scsi-pci, vhost-scsi-pci, and
> > vhost-user-scsi-pci request virtqueues to match the number of vCPUs.
> > Other transports continue to default to 1 request virtqueue.
> > 
> > A 1:1 virtqueue:vCPU mapping 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.  The
> > maximum number of MSI-X vectors and virtqueues limit are respected.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  include/hw/virtio/virtio-scsi.h |  2 ++
> >  hw/core/machine.c               |  3 +++
> >  hw/scsi/vhost-scsi.c            |  3 ++-
> >  hw/scsi/vhost-user-scsi.c       |  3 ++-
> >  hw/scsi/virtio-scsi.c           |  6 +++++-
> >  hw/virtio/vhost-scsi-pci.c      | 10 +++++++---
> >  hw/virtio/vhost-user-scsi-pci.c | 10 +++++++---
> >  hw/virtio/virtio-scsi-pci.c     | 10 +++++++---
> >  8 files changed, 35 insertions(+), 12 deletions(-)
> 
> (...)
> 
> > diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
> > index 3ff9eb7ef6..fa4b3bfb50 100644
> > --- a/hw/virtio/virtio-scsi-pci.c
> > +++ b/hw/virtio/virtio-scsi-pci.c
> > @@ -46,13 +46,17 @@ 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);
> >      DeviceState *proxy = DEVICE(vpci_dev);
> > +    VirtIOSCSIConf *conf = &dev->vdev.parent_obj.conf;
> >      char *bus_name;
> >  
> > +    if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
> > +        conf->num_queues =
> > +            virtio_pci_optimal_num_queues(VIRTIO_SCSI_VQ_NUM_FIXED);
> > +    }
> > +
> >      if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> > -        vpci_dev->nvectors = vs->conf.num_queues +
> > -                             VIRTIO_SCSI_VQ_NUM_FIXED + 1;
> > +        vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 1;
> >      }
> >  
> >      /*
> 
> One corner case where the setup may end up being a bit odd is a
> situation where nvectors was specified, but num_queues was not, and the
> device suddenly ends up with more queues than vectors. But I don't see
> a reason why you would want to specify nvectors but not num_queues in a
> real word scenario, so I think we can ignore that corner case.

I agree, I've ignored that case. Other options include printing a
warning or even an error when num_queues disagrees with nvectors.

> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

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

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

[Stefan Hajnoczi]

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

On Tue, Jul 07, 2020 at 05:44:53PM +0200, Cornelia Huck wrote:
> On Mon,  6 Jul 2020 14:56:48 +0100
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> Maybe mention 'pci' in the subject as well?

Actually splitting up the patch is hard due to the nvectors dependency
on num_queues. I will leave it as a single patch and rewrite the commit
message.

Stefan

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

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

[Cornelia Huck]

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

On Wed, 8 Jul 2020 14:05:26 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Tue, Jul 07, 2020 at 05:44:53PM +0200, Cornelia Huck wrote:
> > On Mon,  6 Jul 2020 14:56:48 +0100
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:

> > > diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
> > > index 3ff9eb7ef6..fa4b3bfb50 100644
> > > --- a/hw/virtio/virtio-scsi-pci.c
> > > +++ b/hw/virtio/virtio-scsi-pci.c
> > > @@ -46,13 +46,17 @@ 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);
> > >      DeviceState *proxy = DEVICE(vpci_dev);
> > > +    VirtIOSCSIConf *conf = &dev->vdev.parent_obj.conf;
> > >      char *bus_name;
> > >  
> > > +    if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
> > > +        conf->num_queues =
> > > +            virtio_pci_optimal_num_queues(VIRTIO_SCSI_VQ_NUM_FIXED);
> > > +    }
> > > +
> > >      if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> > > -        vpci_dev->nvectors = vs->conf.num_queues +
> > > -                             VIRTIO_SCSI_VQ_NUM_FIXED + 1;
> > > +        vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 1;
> > >      }
> > >  
> > >      /*  
> > 
> > One corner case where the setup may end up being a bit odd is a
> > situation where nvectors was specified, but num_queues was not, and the
> > device suddenly ends up with more queues than vectors. But I don't see
> > a reason why you would want to specify nvectors but not num_queues in a
> > real word scenario, so I think we can ignore that corner case.  
> 
> I agree, I've ignored that case. Other options include printing a
> warning or even an error when num_queues disagrees with nvectors.

I think an error would be too harsh, but a warning sounds useful.

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

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

[Raphael Norwitz]

On Mon, Jul 6, 2020 at 7:00 AM 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.  The maximum number of MSI-X
> vectors and virtqueues limit are respected.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  include/hw/virtio/vhost-user-blk.h | 2 ++
>  hw/block/vhost-user-blk.c          | 6 +++++-
>  hw/core/machine.c                  | 1 +
>  hw/virtio/vhost-user-blk-pci.c     | 4 ++++
>  4 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h
> index 34ad6f0c0e..292d17147c 100644
> --- a/include/hw/virtio/vhost-user-blk.h
> +++ b/include/hw/virtio/vhost-user-blk.h
> @@ -25,6 +25,8 @@
>  #define VHOST_USER_BLK(obj) \
>          OBJECT_CHECK(VHostUserBlk, (obj), TYPE_VHOST_USER_BLK)
>
> +#define VHOST_USER_BLK_AUTO_NUM_QUEUES UINT16_MAX
> +
>  typedef struct VHostUserBlk {
>      VirtIODevice parent_obj;
>      CharBackend chardev;
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index a00b854736..39aec42dae 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -420,6 +420,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>
> +    if (s->num_queues == VHOST_USER_BLK_AUTO_NUM_QUEUES) {
> +        s->num_queues = 1;
> +    }

What is this check for? Is it just a backstop to ensure that
num_queues is set to 1 if vhost-user-blk-pci doesn't update it?

>      if (!s->num_queues || s->num_queues > VIRTIO_QUEUE_MAX) {
>          error_setg(errp, "vhost-user-blk: invalid number of IO queues");
>          return;
> @@ -531,7 +534,8 @@ 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_UINT16("num-queues", VHostUserBlk, num_queues,
> +                       VHOST_USER_BLK_AUTO_NUM_QUEUES),
>      DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
>      DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 845f6476cb..31bfaacdb5 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -30,6 +30,7 @@
>
>  GlobalProperty hw_compat_5_0[] = {
>      { "vhost-scsi", "num_queues", "1"},
> +    { "vhost-user-blk", "num-queues", "1"},
>      { "vhost-user-scsi", "num_queues", "1"},
>      { "virtio-balloon-device", "page-poison", "false" },
>      { "virtio-blk-device", "num-queues", "1"},
> diff --git a/hw/virtio/vhost-user-blk-pci.c b/hw/virtio/vhost-user-blk-pci.c
> index 4f5d5cbf44..a62a71e067 100644
> --- a/hw/virtio/vhost-user-blk-pci.c
> +++ b/hw/virtio/vhost-user-blk-pci.c
> @@ -54,6 +54,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);
>
> +    if (dev->vdev.num_queues == VHOST_USER_BLK_AUTO_NUM_QUEUES) {
> +        dev->vdev.num_queues = virtio_pci_optimal_num_queues(0);
> +    }
> +
>      if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
>          vpci_dev->nvectors = dev->vdev.num_queues + 1;
>      }
> --
> 2.26.2
>

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

[Stefan Hajnoczi]

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

On Thu, Jul 09, 2020 at 11:02:24AM -0700, Raphael Norwitz wrote:
> On Mon, Jul 6, 2020 at 7:00 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index a00b854736..39aec42dae 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -420,6 +420,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> >          return;
> >      }
> >
> > +    if (s->num_queues == VHOST_USER_BLK_AUTO_NUM_QUEUES) {
> > +        s->num_queues = 1;
> > +    }
> 
> What is this check for? Is it just a backstop to ensure that
> num_queues is set to 1 if vhost-user-blk-pci doesn't update it?

For the non-PCI VIRTIO transports that do not handle num_queues ==
VHOST_USER_BLK_AUTO_NUM_QUEUES themselves.

Stefan

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

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

[Raphael Norwitz]

On Fri, Jul 10, 2020 at 5:53 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Thu, Jul 09, 2020 at 11:02:24AM -0700, Raphael Norwitz wrote:
> > On Mon, Jul 6, 2020 at 7:00 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > > index a00b854736..39aec42dae 100644
> > > --- a/hw/block/vhost-user-blk.c
> > > +++ b/hw/block/vhost-user-blk.c
> > > @@ -420,6 +420,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> > >          return;
> > >      }
> > >
> > > +    if (s->num_queues == VHOST_USER_BLK_AUTO_NUM_QUEUES) {
> > > +        s->num_queues = 1;
> > > +    }
> >
> > What is this check for? Is it just a backstop to ensure that
> > num_queues is set to 1 if vhost-user-blk-pci doesn't update it?
>
> For the non-PCI VIRTIO transports that do not handle num_queues ==
> VHOST_USER_BLK_AUTO_NUM_QUEUES themselves.
>

Got it. Looks good then.

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> Stefan

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

[Stefan Hajnoczi]

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

On Wed, Jul 08, 2020 at 06:50:12PM +0200, Cornelia Huck wrote:
> On Wed, 8 Jul 2020 14:05:26 +0100
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > On Tue, Jul 07, 2020 at 05:44:53PM +0200, Cornelia Huck wrote:
> > > On Mon,  6 Jul 2020 14:56:48 +0100
> > > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > > > diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
> > > > index 3ff9eb7ef6..fa4b3bfb50 100644
> > > > --- a/hw/virtio/virtio-scsi-pci.c
> > > > +++ b/hw/virtio/virtio-scsi-pci.c
> > > > @@ -46,13 +46,17 @@ 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);
> > > >      DeviceState *proxy = DEVICE(vpci_dev);
> > > > +    VirtIOSCSIConf *conf = &dev->vdev.parent_obj.conf;
> > > >      char *bus_name;
> > > >  
> > > > +    if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
> > > > +        conf->num_queues =
> > > > +            virtio_pci_optimal_num_queues(VIRTIO_SCSI_VQ_NUM_FIXED);
> > > > +    }
> > > > +
> > > >      if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> > > > -        vpci_dev->nvectors = vs->conf.num_queues +
> > > > -                             VIRTIO_SCSI_VQ_NUM_FIXED + 1;
> > > > +        vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 1;
> > > >      }
> > > >  
> > > >      /*  
> > > 
> > > One corner case where the setup may end up being a bit odd is a
> > > situation where nvectors was specified, but num_queues was not, and the
> > > device suddenly ends up with more queues than vectors. But I don't see
> > > a reason why you would want to specify nvectors but not num_queues in a
> > > real word scenario, so I think we can ignore that corner case.  
> > 
> > I agree, I've ignored that case. Other options include printing a
> > warning or even an error when num_queues disagrees with nvectors.
> 
> I think an error would be too harsh, but a warning sounds useful.

I'll send this as a separate patch because at least virtio-serial is
also affected.

Stefan

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