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

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?

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.