Introduce XML to allowing adding iothreads to the domain. These can be used by virtio-blk-pci devices in order to assign a specific thread to handle the workload for the device. The iothreads are the official implementation of the virtio-blk Data Plane that's been in tech preview for QEMU. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- docs/formatdomain.html.in | 26 ++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 6 ++++++ src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ 4 files changed, 54 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index de4e4eb..b584a08 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -470,6 +470,32 @@ </dd> </dl> + <h3><a name="elementsIOThreadsAllocation">IOThreads Allocation</a></h3> + <p> + IOThreads are a QEMU feature that will allow supported disk + devices to be configured to use a dedicated event loop thread + to handle block I/O requests. + <span class="since">Since 1.2.8 (QEMU only)</span> + </p> + +<pre> +&lt;domain&gt; + ... + &lt;iothreads&gt;4&lt;/iothreads&gt; + ... +&lt;/domain&gt; +</pre> + + <dl> + <dt><code>iothreads</code></dt> + <dd> + The content of this optional element defines the number + of IOThreads to be assigned to the domain for use by + virtio-blk-pci target storage devices. There should be + only 1 or 2 IOThreads per host CPU and 1 IOThread per + supported device. + </dd> + </dl> <h3><a name="elementsCPUTuning">CPU Tuning</a></h3> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3170db2..5f1c226 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -632,6 +632,12 @@ </optional> <optional> + <element name="iothreads"> + <ref name="unsignedInt"/> + </element> + </optional> + + <optional> <ref name="blkiotune"/> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dd512ca..81a3fdf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11957,6 +11957,15 @@ virDomainDefParseXML(xmlDocPtr xml, } } + /* Optional - iothreads */ + tmp = virXPathString("string(./iothreads[1])", ctxt); + if (tmp && virStrToLong_uip(tmp, NULL, 0, &def->iothreads) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid iothreads count '%s'"), tmp); + goto error; + } + VIR_FREE(tmp); + /* Extract cpu tunables. */ if ((n = virXPathULong("string(./cputune/shares[1])", ctxt, &def->cputune.shares)) < -1) { @@ -14537,6 +14546,14 @@ virDomainDefCheckABIStability(virDomainDefPtr src, goto error; } + if (src->iothreads != dst->iothreads) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain iothreads count %u does not " + "match source %u"), + dst->iothreads, src->iothreads); + goto error; + } + if (STRNEQ(src->os.type, dst->os.type)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain OS type %s does not match source %s"), @@ -17950,6 +17967,9 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, " current='%u'", def->vcpus); virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus); + if (def->iothreads > 0) + virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n", def->iothreads); + if (def->cputune.sharesSpecified || (def->cputune.nvcpupin && !virDomainIsAllVcpupinInherited(def)) || def->cputune.period || def->cputune.quota || diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index aead903..23b117d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1915,6 +1915,8 @@ struct _virDomainDef { int placement_mode; virBitmapPtr cpumask; + unsigned int iothreads; + struct { unsigned long shares; bool sharesSpecified; -- 1.9.3

On Tue, Aug 26, 2014 at 06:15:46PM -0400, John Ferlan wrote: I forgot two things in the review. One of the things is that you use '0' here (as I suggested, sorry for that), which would not that big of a deal, it would even be awesome, but unfortunately we use '10' everywhere in the XML parsing code :( Even when parsing the iothread="" in 3/4 you are using '10', so this should be changed to '10'. The second thing is that there might be a qemuxml2xmltest case for the qemuxml2argv-iothreads.xml (which can be added in this patch), but that's just a bike-shedding. The ACK still stands, of course. Martin

On Tue, Aug 26, 2014 at 06:15:48PM -0400, John Ferlan wrote: I'm rather fan of "assigns" then "will assign", but both make sense here. ACK either way. Martin

For virtio-blk-pci disks with the disk iothread attribute that are running the correct emulator, add the "iothread=iothread#" to the -device command line in order to enable iothreads for the disk. This code will check both the "start" and "hotplug" paths for the capability, whether the iothreadsmap has been defined, and whether there's an available iothread to be used for the disk. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_command.c | 51 ++++++++++++++++++++++ src/qemu/qemu_hotplug.c | 6 +++ .../qemuxml2argv-iothreads-disk.args | 17 ++++++++ .../qemuxml2argv-iothreads-disk.xml | 40 +++++++++++++++++ tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmltest.c | 1 + 6 files changed, 117 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ba0b977..763bb74 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3673,6 +3673,49 @@ qemuBuildDriveStr(virConnectPtr conn, return NULL; } + +static bool +qemuCheckIothreads(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virDomainDiskDefPtr disk) +{ + bool inuse; + + /* Have capability */ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not supported for this QEMU")); + return false; + } + + /* Right "type" of disk" */ + if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO && + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not available for this disk")); + return false; + } + + /* Value larger than iothreads available? */ + if (disk->iothread > def->iothreads) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Disk iothread '%u' invalid only %u IOThreads"), + disk->iothread, def->iothreads); + return false; + } + + /* Is the requested entry available */ + if (virBitmapGetBit(def->iothreadmap, disk->iothread, &inuse) < 0 || + inuse) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("IOThread '%u' is already in use"), disk->iothread); + return false; + } + + return true; +} + + char * qemuBuildDriveDevStr(virDomainDefPtr def, virDomainDiskDefPtr disk, @@ -3697,6 +3740,9 @@ qemuBuildDriveDevStr(virDomainDefPtr def, } } + if (disk->iothread && !qemuCheckIothreads(def, qemuCaps, disk)) + goto error; + switch (disk->bus) { case VIR_DOMAIN_DISK_BUS_IDE: if (disk->info.addr.drive.target != 0) { @@ -3874,6 +3920,8 @@ qemuBuildDriveDevStr(virDomainDefPtr def, virBufferAddLit(&opt, "virtio-blk-device"); } else { virBufferAddLit(&opt, "virtio-blk-pci"); + if (disk->iothread) + virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); } qemuBuildIoEventFdStr(&opt, disk->ioeventfd, qemuCaps); if (disk->event_idx && @@ -8288,6 +8336,9 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; virCommandAddArg(cmd, optstr); VIR_FREE(optstr); + if (disk->iothread) + ignore_value(virBitmapSetBit(def->iothreadmap, + disk->iothread)); } } } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a364c52..71f47b2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -379,6 +379,9 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, virFreeError(orig_err); } } + if (ret == 0 && disk->iothread) + ignore_value(virBitmapSetBit(vm->def->iothreadmap, + disk->iothread)); } } else if (!disk->info.type || disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { @@ -2539,6 +2542,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, } } + if (disk->iothread) + ignore_value(virBitmapClearBit(vm->def->iothreadmap, disk->iothread)); + qemuDomainReleaseDeviceAddress(vm, &disk->info, src); if (virSecurityManagerRestoreDiskLabel(driver->securityManager, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.args b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.args new file mode 100644 index 0000000..4d271ed --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.args @@ -0,0 +1,17 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M \ +pc -m 214 -smp 2 \ +-object iothread,id=iothread1 \ +-object iothread,id=iothread2 \ +-nographic -nodefaults -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-drive file=/var/lib/libvirt/images/iothrtest1.img,if=none,\ +id=drive-virtio-disk1 \ +-device virtio-blk-pci,iothread=iothread1,bus=pci.0,addr=0x4,\ +drive=drive-virtio-disk1,id=virtio-disk1 \ +-drive file=/var/lib/libvirt/images/iothrtest2.img,if=none,\ +id=drive-virtio-disk2 \ +-device virtio-blk-pci,iothread=iothread2,bus=pci.0,addr=0x3,\ +drive=drive-virtio-disk2,id=virtio-disk2 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.xml b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.xml new file mode 100644 index 0000000..72122fb --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.xml @@ -0,0 +1,40 @@ +<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'>2</vcpu> + <iothreads>2</iothreads> + <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'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw' iothread='1'/> + <source file='/var/lib/libvirt/images/iothrtest1.img'/> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw' iothread='2'/> + <source file='/var/lib/libvirt/images/iothrtest2.img'/> + <target dev='vdc' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 64a4d3d..3feb2fe 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1186,6 +1186,8 @@ mymain(void) DO_TEST("smp", QEMU_CAPS_SMP_TOPOLOGY); DO_TEST("iothreads", QEMU_CAPS_OBJECT_IOTHREAD); + DO_TEST("iothreads-disk", QEMU_CAPS_OBJECT_IOTHREAD, QEMU_CAPS_DEVICE, + QEMU_CAPS_DRIVE); DO_TEST("cpu-topology1", QEMU_CAPS_SMP_TOPOLOGY); DO_TEST("cpu-topology2", QEMU_CAPS_SMP_TOPOLOGY); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 796977b..b4ab671 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -302,6 +302,7 @@ mymain(void) DO_TEST("smp"); DO_TEST("iothreads"); + DO_TEST("iothreads-disk"); DO_TEST("lease"); DO_TEST("event_idx"); DO_TEST("vhost_queues"); -- 1.9.3

On 08/27/2014 03:00 AM, Martin Kletzander wrote: OK - I left it at "virtio pci", but can change it to "virtio-pci" or "virtio-blk-pci" based on the answer to the following question... Is this safe enough to be pushed with the freeze now in effect. I know the review started before and all that, but I figured I'd ask before pushing in any case! Tks, John