[libvirt] [PATCH] qemu: Support multiple queue virtio-scsi
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 | 9 +++++--- 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, 74 insertions(+), 3 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..262f1ac 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2092,10 +2092,13 @@ 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 + "vmpvscsi". The attribute <code>num_queues</code> + (<span class="since">1.0.5 (QEMU and KVM only)</span>) is to specify + the number of queues for the controller, it should match the number + of vCPUs. 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, + "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. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8d7e6db..089be6c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1407,6 +1407,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 f3fca7f..e9d6b8b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4825,6 +4825,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, char *type = NULL; char *idx = NULL; char *model = NULL; + char *num_queues = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -4860,6 +4861,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; @@ -4937,6 +4946,7 @@ cleanup: VIR_FREE(type); VIR_FREE(idx); VIR_FREE(model); + VIR_FREE(num_queues); return def; @@ -13037,6 +13047,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 edddf25..8e79961 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 a0c278f..bc7beff 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3530,6 +3530,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; @@ -3613,6 +3621,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 38787ac..055d935 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -550,6 +550,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 ba9aa96..e5056f1 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); -- 1.8.1.4
On 04/01/2013 10:04 AM, 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'/>
Our XML is not consistent on whether we prefer '-' or '_' in element and attribute names, so I guess your naming is as good as any.
The corresponding QEMU command line:
-device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \ --- docs/formatdomain.html.in | 9 +++++--- 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, 74 insertions(+), 3 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..262f1ac 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2092,10 +2092,13 @@ 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 + "vmpvscsi". The attribute <code>num_queues</code> + (<span class="since">1.0.5 (QEMU and KVM only)</span>) is to specify + the number of queues for the controller, it should match the number + of vCPUs. A "usb" controller has an optional attribute
What happens if it doesn't match the number of vcpus? And if it should match, then why must we specify it in the XML, instead of supplying the qemu argument automatically? Did you instead mean to say that if the attribute is not present, then the number of queues defaults to the number of vcpus?
<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, + "ich9-ehci1", "ich9-uhci1", "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", + "pci-ohci" or "nec-xhci". Additionally,
Any reason you reformatted these lines? The rest of the patch looks okay, but I want to make sure we have the design right before committing to it in this form. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/04/13 00:21, Eric Blake wrote:
On 04/01/2013 10:04 AM, 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'/> Our XML is not consistent on whether we prefer '-' or '_' in element and attribute names, so I guess your naming is as good as any.
The corresponding QEMU command line:
-device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \ --- docs/formatdomain.html.in | 9 +++++--- 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, 74 insertions(+), 3 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..262f1ac 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2092,10 +2092,13 @@ 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 + "vmpvscsi". The attribute <code>num_queues</code> + (<span class="since">1.0.5 (QEMU and KVM only)</span>) is to specify + the number of queues for the controller, it should match the number + of vCPUs. A "usb" controller has an optional attribute What happens if it doesn't match the number of vcpus? And if it should match, then why must we specify it in the XML, instead of supplying the qemu argument automatically? Did you instead mean to say that if the attribute is not present, then the number of queues defaults to the number of vcpus?
No, multiple queue is not enabled by default. For "what happens if it doesn't match the number of vcpus", honestly, I'm not sure about it, my understanding is it doesn't have to match. But matching will have the best performance, and with this thought I think an attribute allows the user to specify the number makes sense instead of a boolean attribute. @paolo, can you confirm or deny this?
<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, + "ich9-ehci1", "ich9-uhci1", "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", + "pci-ohci" or "nec-xhci". Additionally,
Any reason you reformatted these lines?
I tended to make it more beautiful, but apprently it's not true. :-)
The rest of the patch looks okay, but I want to make sure we have the design right before committing to it in this form.
On 04/01/2013 10:31 AM, Osier Yang wrote:
No, multiple queue is not enabled by default. For "what happens if it doesn't match the number of vcpus", honestly, I'm not sure about it, my understanding is it doesn't have to match. But matching will have the best performance, and with this thought I think an attribute allows the user to specify the number makes sense instead of a boolean attribute. @paolo, can you confirm or deny this?
Then maybe all we need is just a recommendation that matching the number of vcpus tends to give best performance, but not stating that it is mandatory to match.
<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, + "ich9-ehci1", "ich9-uhci1", "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", + "pci-ohci" or "nec-xhci". Additionally,
Any reason you reformatted these lines?
I tended to make it more beautiful, but apprently it's not true. :-)
It doesn't affect the html output; so all it affects is the source file legibility. But it is unrelated to the patch at hand, and since it makes no semantic difference, I was merely questioning why you did it. It's not wrong to leave it in, but it also doesn't add any value so leaving it unchanged makes for a smaller diff. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
----- Messaggio originale -----
Da: "Eric Blake" <eblake@redhat.com> A: "Osier Yang" <jyang@redhat.com> Cc: libvir-list@redhat.com, pbonzini@redhat.com Inviato: Lunedì, 1 aprile 2013 18:43:11 Oggetto: Re: [libvirt] [PATCH] qemu: Support multiple queue virtio-scsi
On 04/01/2013 10:31 AM, Osier Yang wrote:
No, multiple queue is not enabled by default. For "what happens if it doesn't match the number of vcpus", honestly, I'm not sure about it, my understanding is it doesn't have to match. But matching will have the best performance, and with this thought I think an attribute allows the user to specify the number makes sense instead of a boolean attribute. @paolo, can you confirm or deny this?
Then maybe all we need is just a recommendation that matching the number of vcpus tends to give best performance, but not stating that it is mandatory to match.
I think in the current patches anything that doesn't exactly matches the number of VCPUs will fallback to single-queue. But it doesn't have to be the case, so a numeric attribute is preferrable. Paolo
participants (3)
-
Eric Blake -
Osier Yang -
Paolo Bonzini