Re: [PATCH v2] qemu: Support multiple queue virtio-scsi

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

On 06/04/13 04:27, John Ferlan wrote:

On 04/05/2013 12:21 PM, Osier Yang wrote:

This introduce a new attribute "num_queues" (same with the good name
QEMU uses) for virtio-scsi controller. An example of the XML:

<controller type='scsi' index='0' model='virtio-scsi' num_queues='8'/>

The corresponding QEMU command line:

-device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \
---
  docs/formatdomain.html.in                          | 18 +++++++++------
  docs/schemas/domaincommon.rng                      |  5 +++++
  src/conf/domain_conf.c                             | 13 +++++++++++
  src/conf/domain_conf.h                             |  1 +
  src/qemu/qemu_command.c                            | 11 +++++++++
  .../qemuxml2argv-disk-virtio-scsi-num_queues.args  |  8 +++++++
  .../qemuxml2argv-disk-virtio-scsi-num_queues.xml   | 26 ++++++++++++++++++++++
  tests/qemuxml2argvtest.c                           |  3 +++
  tests/qemuxml2xmltest.c                            |  1 +
  9 files changed, 79 insertions(+), 7 deletions(-)
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.args
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index cf382e8..2973ce2 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2092,13 +2092,17 @@
        controller.  A "scsi" controller has an optional
        attribute <code>model</code>, which is one of "auto", "buslogic",
        "ibmvscsi", "lsilogic", "lsisas1068", "lsisas1078", "virtio-scsi" or
-      "vmpvscsi".  A "usb" controller has an optional attribute
-      <code>model</code>, which is one of "piix3-uhci", "piix4-uhci", "ehci",
-      "ich9-ehci1", "ich9-uhci1", "ich9-uhci2", "ich9-uhci3",
-      "vt82c686b-uhci", "pci-ohci" or "nec-xhci". Additionally,
-      <span class="since">since 0.10.0</span>, if the USB bus needs to be
-      explicitly disabled for the guest, <code>model='none'</code> may be used.
-      The PowerPC64 "spapr-vio" addresses do not have an associated controller.
+      "vmpvscsi".  The attribute <code>num_queues</code>
+      (<span class="since">1.0.5 (QEMU and KVM only)</span>) is to specify

s/is to specify/specifies/


+      the number of queues for the controller, it's recommended to specify

s/, it's/. For best performance, it's/


+      the value matching the number of vCPUs for best performance.  A "usb"

s/the/a

s/ for best performance//


+      controller has an optional attribute <code>model</code>, which is one
+      of "piix3-uhci", "piix4-uhci", "ehci", "ich9-ehci1", "ich9-uhci1",
+      "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", "pci-ohci" or "nec-xhci".
+      Additionally, <span class="since">since 0.10.0</span>, if the USB bus
+      needs to be explicitly disabled for the guest, <code>model='none'</code>
+      may be used.  The PowerPC64 "spapr-vio" addresses do not have an
+      associated controller.
      </p>
<p> 
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 63ba7d1..d486ae8 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1427,6 +1427,11 @@
          </attribute>
        </optional>
        <optional>
+        <attribute name="num_queues">
+          <ref name="unsignedInt"/>
+        </attribute>
+      </optional>
+      <optional>
          <ref name="usbmaster"/>
        </optional>
        <optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5ad3080..f50a964 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5034,6 +5034,7 @@ virDomainControllerDefParseXML(xmlNodePtr node,
      char *type = NULL;
      char *idx = NULL;
      char *model = NULL;
+    char *num_queues = NULL;
if (VIR_ALLOC(def) < 0) { 
          virReportOOMError();
@@ -5069,6 +5070,14 @@ virDomainControllerDefParseXML(xmlNodePtr node,
          def->model = -1;
      }
+ if ((num_queues = virXMLPropString(node, "num_queues"))) { 
+        if (virStrToLong_ui(num_queues, NULL, 10, &def->num_queues) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("Malformed 'num_queues' value '%s'"), num_queues);
+            goto error;
+        }
+    }
+
      if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
          goto error;
@@ -5146,6 +5155,7 @@ cleanup: 
      VIR_FREE(type);
      VIR_FREE(idx);
      VIR_FREE(model);
+    VIR_FREE(num_queues);
return def; @@ -13195,6 +13205,9 @@ virDomainControllerDefFormat(virBufferPtr buf, 
          virBufferEscapeString(buf, " model='%s'", model);
      }
+ if (def->num_queues) 
+        virBufferAsprintf(buf, " num_queues='%u'", def->num_queues);
+
      switch (def->type) {
      case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
          if (def->opts.vioserial.ports != -1) {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 700f03f..75e3f15 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -728,6 +728,7 @@ struct _virDomainControllerDef {
      int type;
      int idx;
      int model; /* -1 == undef */
+    unsigned int num_queues;
      union {
          virDomainVirtioSerialOpts vioserial;
      } opts;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 493e5f8..53caba5 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3533,6 +3533,14 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
      virBuffer buf = VIR_BUFFER_INITIALIZER;
      int model;
+ if (def->num_queues && 
+        !(def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
+          def->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("'num_queues' is only supported by virtio-scsi controller"));
+        return NULL;
+    }
+
      switch (def->type) {
      case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
          model = def->model;
@@ -3616,6 +3624,9 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
          goto error;
      }
+ if (def->num_queues) 
+        virBufferAsprintf(&buf, ",num_queues=%u", def->num_queues);
+
      if (qemuBuildDeviceAddressStr(&buf, &def->info, qemuCaps) < 0)
          goto error;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.args 
new file mode 100644
index 0000000..8d46799
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.args
@@ -0,0 +1,8 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
+/usr/bin/qemu -S -M pc -m 214 -smp 8 -nographic -nodefconfig -nodefaults \
+-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \
+-device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \
+-device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml
new file mode 100644
index 0000000..dfa9cf1
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml
@@ -0,0 +1,26 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219136</memory>
+  <currentMemory unit='KiB'>219136</currentMemory>
+  <vcpu placement='static'>8</vcpu>
+  <os>
+    <type arch='i686' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu</emulator>
+    <disk type='block' device='disk'>
+      <source dev='/dev/HostVG/QEMUGuest1'/>
+      <target dev='sdb' bus='scsi'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <controller type='usb' index='0'/>
+    <controller type='scsi' index='0' model='virtio-scsi' num_queues='8'/>
+    <memballoon model='virtio'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 099fb36..d6575e7 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -553,6 +553,9 @@ mymain(void)
      DO_TEST("disk-scsi-virtio-scsi",
              QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG,
              QEMU_CAPS_VIRTIO_SCSI);
+    DO_TEST("disk-virtio-scsi-num_queues",
+            QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG,
+            QEMU_CAPS_VIRTIO_SCSI);
      DO_TEST("disk-scsi-megasas",
              QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG,
              QEMU_CAPS_SCSI_MEGASAS);
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index aa058bd..10a6ea2 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -179,6 +179,7 @@ mymain(void)
      DO_TEST("disk-scsi-device");
      DO_TEST("disk-scsi-vscsi");
      DO_TEST("disk-scsi-virtio-scsi");
+    DO_TEST("disk-virtio-scsi-num_queues");
      DO_TEST("disk-scsi-megasas");
      DO_TEST_FULL("disk-mirror", false, WHEN_ACTIVE);
      DO_TEST_FULL("disk-mirror", true, WHEN_INACTIVE);


ACK - mechanically it seems fine.

Does there need to be any capability checks done that the QEMU supports
the option?  Or does QEMU happily ignore the option if passed and it
doesn't support it?


It showed up in same qemu version with virtio-scsi-pci, which we
already supported.



What is the implication of someone supplying 100 to num_queues with a
domain with a vCPU count of 1 or 2?  I know, if they want to shoot
themselves in the foot, well go ahead.  However, if this is something
that could be prevented or perhaps would be rejected by QEMU, then one
would think some sort of range validation would exist. The only reason I
mention it is the trigger that went off in my head when it was stated
"For performance reasons" this value should match the vCPU count.


See paolo's feedback on v1. Currently qemu uses 1 queue for any number
not matching the number of vCPU. I don't think we should prevent it,
as it's very possible qemu will do some improvements.

Thanks for the reviewing, I pushed this.

Osier

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]