[libvirt] [PATCH v2 0/4] Fix a couple of iothread bugs
v1: http://www.redhat.com/archives/libvir-list/2015-October/msg00417.html Differences since v1 - many. - Reworked patches 1 & 2 to become patches 1 - 3 in the new series - Patch 1 is new, but the thinking behind patch 3 caused me to generate this patch which essentially changes comparisons with "->iothreads" to use "->niothreadids" instead except for one place where we're building the "iothreadids[]" array based on the difference between iothreads and niothreadids. - Patch 2 is essentially what former patch 1 became, adding a single check for niothreads == 0 rather than checking the capabilities bit. - The former patch 2 was dropped. In it's place is the new Patch 3 which moves the check for the capabilities bit to when the IOThread objects are added to the domain at startup time. Making that change had a couple of side effects and the whole sordid history of how we got into this situation was left in the commit message. Due to moving the check one test started failing, so that was fixed and while I was at it - I added a test which would define iothreads, some iothreadids, and some iothreadpin to show that without the capability the failure will be at start time. - The former patch 3 was already pushed since it was ACK'd - The former patch 4 logic was adjusted (more or less) to what was requested during the review with a couple of tweaks based on the realities I found during testing. NB: I spent some cycles considering how to handle the case where a domain with iothreads/niothreadids > 0 was already running when libvirtd was restarted and how to handle that. In the end I decided upon trying to do the somewhat nice thing and remove all traces and if some domain had iothreadid's defined, then leave a VIR_INFO message indicating the removal. Without this change, then qemuDomainPinIOThread for the live code will need some kind of adjustment in order to handle the case where the capability doesn't exist (some sort of thread_id == 0 check right after the IOThreadIDFind succeeds). John Ferlan (4): qemu: Use 'niothreadids' instead of 'iothreads' qemu: Check for niothreads == 0 in qemuSetupCgroupForIOThreads qemu: Fix qemu startup check for QEMU_CAPS_OBJECT_IOTHREAD conf: Optimize the iothreadid initialization src/conf/domain_audit.c | 4 +- src/conf/domain_conf.c | 43 +++++++++++++++++----- src/qemu/qemu_cgroup.c | 3 ++ src/qemu/qemu_command.c | 19 ++++------ src/qemu/qemu_driver.c | 16 ++++---- src/qemu/qemu_process.c | 32 ++++++++++++++-- .../qemuxml2argv-cputune-numatune.args | 1 + .../qemuxml2argv-iothreads-nocap.xml | 37 +++++++++++++++++++ tests/qemuxml2argvtest.c | 2 + 9 files changed, 122 insertions(+), 35 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-nocap.xml -- 2.1.0
Although theoretically both should be the same value, the niothreadids should be used in favor of iothreads when performing comparisons. This leaves the iothreads as a purely numeric value to be saved in the config file. The one exception to the rule is virDomainIOThreadIDDefArrayInit where the iothreadids are being generated from the iothreads count since iothreadids were added after initial iothreads support. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_audit.c | 4 ++-- src/conf/domain_conf.c | 4 ++-- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_driver.c | 16 ++++++++-------- src/qemu/qemu_process.c | 6 +++--- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index caebdba..b842495 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -886,8 +886,8 @@ virDomainAuditStart(virDomainObjPtr vm, const char *reason, bool success) virDomainAuditMemory(vm, 0, virDomainDefGetMemoryActual(vm->def), "start", true); virDomainAuditVcpu(vm, 0, vm->def->vcpus, "start", true); - if (vm->def->iothreads) - virDomainAuditIOThread(vm, 0, vm->def->iothreads, "start", true); + if (vm->def->niothreadids) + virDomainAuditIOThread(vm, 0, vm->def->niothreadids, "start", true); virDomainAuditLifecycle(vm, "start", reason, success); } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 217179d..70b2afc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15220,7 +15220,7 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } if (n) { - if (n > def->iothreads) { + if (n > def->niothreadids) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("too many iothreadsched nodes in cputune")); goto error; @@ -21729,7 +21729,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, " current='%u'", def->vcpus); virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus); - if (def->iothreads > 0) { + if (def->niothreadids > 0) { virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n", def->iothreads); /* Only print out iothreadids if we read at least one */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f727d0b..f99e363 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9462,7 +9462,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, smp); VIR_FREE(smp); - if (def->iothreads > 0 && + if (def->niothreadids > 0 && virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { /* Create iothread objects using the defined iothreadids list * and the defined id and name from the list. These may be used diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8cd5ee3..a2cc002 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5671,13 +5671,13 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, size_t i; int ret = -1; - if (targetDef->iothreads == 0) + if (targetDef->niothreadids == 0) return 0; if ((hostcpus = nodeGetCPUCount(NULL)) < 0) goto cleanup; - if (VIR_ALLOC_N(info_ret, targetDef->iothreads) < 0) + if (VIR_ALLOC_N(info_ret, targetDef->niothreadids) < 0) goto cleanup; for (i = 0; i < targetDef->niothreadids; i++) { @@ -5707,11 +5707,11 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, *info = info_ret; info_ret = NULL; - ret = targetDef->iothreads; + ret = targetDef->niothreadids; cleanup: if (info_ret) { - for (i = 0; i < targetDef->iothreads; i++) + for (i = 0; i < targetDef->niothreadids; i++) virDomainIOThreadInfoFree(info_ret[i]); VIR_FREE(info_ret); } @@ -5910,8 +5910,8 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver, size_t idx; int rc = -1; int ret = -1; - unsigned int orig_niothreads = vm->def->iothreads; - unsigned int exp_niothreads = vm->def->iothreads; + unsigned int orig_niothreads = vm->def->niothreadids; + unsigned int exp_niothreads = vm->def->niothreadids; int new_niothreads = 0; qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; virCgroupPtr cgroup_iothread = NULL; @@ -6039,8 +6039,8 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver, char *alias = NULL; int rc = -1; int ret = -1; - unsigned int orig_niothreads = vm->def->iothreads; - unsigned int exp_niothreads = vm->def->iothreads; + unsigned int orig_niothreads = vm->def->niothreadids; + unsigned int exp_niothreads = vm->def->niothreadids; int new_niothreads = 0; qemuMonitorIOThreadInfoPtr *new_iothreads = NULL; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8aa9efc..e31885b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2308,11 +2308,11 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, if (niothreads < 0) goto cleanup; - if (niothreads != vm->def->iothreads) { + if (niothreads != vm->def->niothreadids) { virReportError(VIR_ERR_INTERNAL_ERROR, _("got wrong number of IOThread pids from QEMU monitor. " - "got %d, wanted %d"), - niothreads, vm->def->iothreads); + "got %d, wanted %zu"), + niothreads, vm->def->niothreadids); goto cleanup; } -- 2.1.0
On Thu, Oct 15, 2015 at 16:43:52 -0400, John Ferlan wrote:
Although theoretically both should be the same value, the niothreadids should be used in favor of iothreads when performing comparisons. This leaves the iothreads as a purely numeric value to be saved in the config file. The one exception to the rule is virDomainIOThreadIDDefArrayInit where the iothreadids are being generated from the iothreads count since iothreadids were added after initial iothreads support.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_audit.c | 4 ++-- src/conf/domain_conf.c | 4 ++-- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_driver.c | 16 ++++++++-------- src/qemu/qemu_process.c | 6 +++--- 5 files changed, 16 insertions(+), 16 deletions(-)
ACK, Peter`
If there are no IOThreads defined, no sense making other checks Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_cgroup.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 570dab5..a8e0b8c 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1163,6 +1163,9 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) char *mem_mask = NULL; virDomainNumatuneMemMode mem_mode; + if (def->niothreadids == 0) + return 0; + if ((period || quota) && !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", -- 2.1.0
On Thu, Oct 15, 2015 at 16:43:53 -0400, John Ferlan wrote:
If there are no IOThreads defined, no sense making other checks
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_cgroup.c | 3 +++ 1 file changed, 3 insertions(+)
ACK, Peter
https://bugzilla.redhat.com/show_bug.cgi?id=1249981 When qemuDomainPinIOThread was added in commit id 'fb562614', a check for the IOThread capability was not needed since a check for iothreadpids covered the condition where the support for IOThreads was not present. The iothreadpids array was only created if qemuProcessDetectIOThreadPIDs was able to query the monitor for IOThreads. It would only do that if the QEMU_CAPS_OBJECT_IOTHREAD capability was set. However, when iothreadids were added in commit id '8d4614a5' and the check for iothreadpids was replaced by a search through the iothreadids[] array for the matching iothread_id that left open the possibility that an iothreadids[] array was defined, but the entries essentially pointed to elements with only the 'iothread_id' defined leaving the 'thread_id' value of 0 and eventually the cpumap entry of NULL. This was because, the original IOThreads commit id '72edaae7' only checked if IOThreads were defined and if the emulator had the IOThreads capability, then IOThread objects were added at startup. The "capability failure" check was only done when a disk was assigned to an IOThread in qemuCheckIOThreads. This was because the initial implementation had no way to dynamically add IOThreads, but it was possible to dynamically add a disk to the domain. So the decision was if the domain supported it, then add the IOThread objects. Then if a disk with an IOThread defined was added, it could check the capability and fail to add if not there. This just meant the 'iothreads' value was essentially ignored. Eventually commit id 'a27ed6e7' allowed for the dynamic addition and deletion of IOThread objects. So it was no longer necessary to generate IOThread objects to dynamically attach a disk to. However, the startup and disk check code was not modified to reflect this. This patch will move the capability failure check to when IOThread objects are being added to the command line. Thus a domain that has IOThreads defined will not be started if the emulator doesn't support the capability. This means when qemuCheckIOThreads is called to add a disk, it's no longer necessary to check the capability. Instead the code can use the IOThreadFind call to indicate that the IOThread doesn't exist. Finally because it could be possible to have a domain running with the iothreadids[] defined prior to this change if libvirtd is restarted each having mostly empty elements, qemuProcessDetectIOThreadPIDs will check if there are niothreadids when the QEMU_CAPS_OBJECT_IOTHREAD capability check fails and remove the elements and array if it exists. With these changes in place, it turns out the cputune-numatune test was failing because the right bit wasn't set in the test. So used the opportunity to fix that and create a test that would expect to fail with some sort of iothreads defined and used, but not having the correct capability. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 19 +++++------ src/qemu/qemu_process.c | 26 ++++++++++++++- .../qemuxml2argv-cputune-numatune.args | 1 + .../qemuxml2argv-iothreads-nocap.xml | 37 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 5 files changed, 73 insertions(+), 12 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-nocap.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f99e363..ae04a69 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4145,16 +4145,8 @@ qemuBuildDriveStr(virConnectPtr conn, static bool qemuCheckIOThreads(virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, virDomainDiskDefPtr disk) { - /* 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 && @@ -4208,7 +4200,7 @@ qemuBuildDriveDevStr(virDomainDefPtr def, if (!qemuCheckCCWS390AddressSupport(def, disk->info, qemuCaps, disk->dst)) goto error; - if (disk->iothread && !qemuCheckIOThreads(def, qemuCaps, disk)) + if (disk->iothread && !qemuCheckIOThreads(def, disk)) goto error; switch (disk->bus) { @@ -9462,8 +9454,13 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, smp); VIR_FREE(smp); - if (def->niothreadids > 0 && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + if (def->niothreadids && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not supported for this QEMU")); + goto error; + } + if (def->niothreadids) { /* Create iothread objects using the defined iothreadids list * and the defined id and name from the list. These may be used * by a disk definition which will associate to an iothread by diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e31885b..07e41f0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2296,8 +2296,32 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, int ret = -1; size_t i; - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + /* The following check is because at one time a domain could + * define iothreadids and start the domain - only failing the + * capability check when attempting to add a disk. Because the + * iothreads and [n]iothreadids were left untouched other code + * assumed it could use the ->thread_id value to make thread_id + * based adjustments (e.g. pinning, scheduling) which while + * succeeding would execute on the calling thread. + */ + if (vm->def->niothreadids) { + for (i = 0; i < vm->def->niothreadids; i++) { + /* Check if the domain had defined any iothreadid elements + * and supply a VIR_INFO indicating that it's being removed. + */ + if (!vm->def->iothreadids[i]->autofill) + VIR_INFO("IOThreads not supported, remove iothread id '%u'", + vm->def->iothreadids[i]->iothread_id); + virDomainIOThreadIDDefFree(vm->def->iothreadids[i]); + } + /* Remove any trace */ + VIR_FREE(vm->def->iothreadids); + vm->def->niothreadids = 0; + vm->def->iothreads = 0; + } return 0; + } /* Get the list of IOThreads from qemu */ if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.args b/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.args index 481f72f..affe648 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.args @@ -1,5 +1,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/bin/qemu-system-x86_64 -S -M pc-q35-2.3 -m 128 \ -smp 2,maxcpus=6,sockets=6,cores=1,threads=1 \ +-object iothread,id=iothread1 -object iothread,id=iothread2 \ -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi \ -boot c -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-iothreads-nocap.xml b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-nocap.xml new file mode 100644 index 0000000..61871e7 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-nocap.xml @@ -0,0 +1,37 @@ +<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>4</iothreads> + <iothreadids> + <iothread id='5'/> + <iothread id='6'/> + </iothreadids> + <cputune> + <iothreadpin iothread='5' cpuset='2'/> + <iothreadpin iothread='6' cpuset='1'/> + </cputune> + <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> + <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 80cb55e..53580e3 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1285,6 +1285,7 @@ mymain(void) DO_TEST("iothreads", QEMU_CAPS_OBJECT_IOTHREAD); DO_TEST("iothreads-ids", QEMU_CAPS_OBJECT_IOTHREAD); DO_TEST("iothreads-ids-partial", QEMU_CAPS_OBJECT_IOTHREAD); + DO_TEST_FAILURE("iothreads-nocap", NONE); DO_TEST("iothreads-disk", QEMU_CAPS_OBJECT_IOTHREAD, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE); DO_TEST("iothreads-disk-virtio-ccw", QEMU_CAPS_OBJECT_IOTHREAD, QEMU_CAPS_DEVICE, @@ -1338,6 +1339,7 @@ mymain(void) DO_TEST_PARSE_ERROR("cputune-vcpusched-overlap", QEMU_CAPS_NAME); DO_TEST("cputune-numatune", QEMU_CAPS_SMP_TOPOLOGY, QEMU_CAPS_KVM, + QEMU_CAPS_OBJECT_IOTHREAD, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE); -- 2.1.0
On Thu, Oct 15, 2015 at 16:43:54 -0400, John Ferlan wrote:
...
--- src/qemu/qemu_command.c | 19 +++++------ src/qemu/qemu_process.c | 26 ++++++++++++++- .../qemuxml2argv-cputune-numatune.args | 1 + .../qemuxml2argv-iothreads-nocap.xml | 37 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 5 files changed, 73 insertions(+), 12 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-nocap.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f99e363..ae04a69 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c
[...]
@@ -9462,8 +9454,13 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, smp); VIR_FREE(smp);
- if (def->niothreadids > 0 && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + if (def->niothreadids && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) {
This case can be folded ...
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not supported for this QEMU")); + goto error; + } + if (def->niothreadids) {
... here so that you don't have to test def->niothreadids twice in 6 lines of code where it can't change.
/* Create iothread objects using the defined iothreadids list * and the defined id and name from the list. These may be used * by a disk definition which will associate to an iothread by
ACK with the above stuff optimized. Peter
https://bugzilla.redhat.com/show_bug.cgi?id=1264008 The existing algorithm assumed that someone was making small, incremental changes; however, it is possible to change iothreads from 0 (or relatively small number) to some really large number and the algorithm would possibly spin its wheels doing unnecessary searches. So, optimize the algorithm using a bitmap to find available iothread_id's starting at 1 that aren't already defined by a "<thread id='#'>" and filling in the iothreadids array with those iothread_id values. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 70b2afc..3e15dcc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2332,8 +2332,11 @@ virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, static int virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) { - unsigned int iothread_id = 1; int retval = -1; + size_t i; + ssize_t nxt = -1; + virDomainIOThreadIDDefPtr iothrid = NULL; + virBitmapPtr thrmap = NULL; /* Same value (either 0 or some number), then we have none to fill in or * the iothreadid array was filled from the XML @@ -2341,19 +2344,39 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) if (def->iothreads == def->niothreadids) return 0; - while (def->niothreadids != def->iothreads) { - if (!virDomainIOThreadIDFind(def, iothread_id)) { - virDomainIOThreadIDDefPtr iothrid; + /* iothread's are numbered starting at 1, account for that */ + thrmap = virBitmapNew(def->iothreads + 1); + virBitmapSetAll(thrmap); - if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) - goto error; - iothrid->autofill = true; + /* Clear 0 since we don't use it, then mark those which are + * already provided by the user */ + ignore_value(virBitmapClearBit(thrmap, 0)); + for (i = 0; i < def->niothreadids; i++) + ignore_value(virBitmapClearBit(thrmap, + def->iothreadids[i]->iothread_id)); + + /* resize array */ + if (VIR_REALLOC_N(def->iothreadids, def->iothreads) < 0) + goto error; + + /* Populate iothreadids[] using the set bit number from thrmap */ + while (def->niothreadids < def->iothreads) { + if ((nxt = virBitmapNextSetBit(thrmap, nxt)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to populate iothreadids")); + goto error; } - iothread_id++; + if (VIR_ALLOC(iothrid) < 0) + goto error; + iothrid->iothread_id = nxt; + iothrid->autofill = true; + def->iothreadids[def->niothreadids++] = iothrid; } + retval = 0; error: + virBitmapFree(thrmap); return retval; } -- 2.1.0
On Thu, Oct 15, 2015 at 16:43:55 -0400, John Ferlan wrote:
[...]
--- src/conf/domain_conf.c | 39 +++++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 70b2afc..3e15dcc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -2341,19 +2344,39 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) if (def->iothreads == def->niothreadids) return 0;
- while (def->niothreadids != def->iothreads) { - if (!virDomainIOThreadIDFind(def, iothread_id)) { - virDomainIOThreadIDDefPtr iothrid; + /* iothread's are numbered starting at 1, account for that */ + thrmap = virBitmapNew(def->iothreads + 1);
You want to check whether the allocation succeeded ...
+ virBitmapSetAll(thrmap);
... or this will crash.
- if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) - goto error; - iothrid->autofill = true;
ACK, Peter
On 10/15/2015 04:43 PM, John Ferlan wrote: ...
John Ferlan (4): qemu: Use 'niothreadids' instead of 'iothreads' qemu: Check for niothreads == 0 in qemuSetupCgroupForIOThreads qemu: Fix qemu startup check for QEMU_CAPS_OBJECT_IOTHREAD conf: Optimize the iothreadid initialization
src/conf/domain_audit.c | 4 +- src/conf/domain_conf.c | 43 +++++++++++++++++----- src/qemu/qemu_cgroup.c | 3 ++ src/qemu/qemu_command.c | 19 ++++------ src/qemu/qemu_driver.c | 16 ++++---- src/qemu/qemu_process.c | 32 ++++++++++++++-- .../qemuxml2argv-cputune-numatune.args | 1 + .../qemuxml2argv-iothreads-nocap.xml | 37 +++++++++++++++++++ tests/qemuxml2argvtest.c | 2 + 9 files changed, 122 insertions(+), 35 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-nocap.xml
Made requested adjustments and pushed - Thanks for the speedy reviews John
participants (2)
-
John Ferlan -
Peter Krempa