During qemu process startup (qemuProcessStart), the call to qemuProcessDetectIOThreadPIDs will not attempt to fill in the ->thread_id values if the binary doesn't support IOThreads. However, subsequent calls to setup the IOThread cgroups, affinity, and scheduler parameters had no such check, thus could attempt to set thread_id = 0, which would not be a good idea. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_cgroup.c | 3 +++ src/qemu/qemu_process.c | 18 ++++++++++++------ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 570dab5..66318ea 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 (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) + return 0; + if ((period || quota) && !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8aa9efc..67e7cbc 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2514,10 +2514,14 @@ qemuProcessSetEmulatorAffinity(virDomainObjPtr vm) static int qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) { + qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; size_t i; int ret = -1; + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) + return 0; + for (i = 0; i < def->niothreadids; i++) { /* set affinity only for existing iothreads */ if (!def->iothreadids[i]->cpumask) @@ -2574,12 +2578,14 @@ qemuProcessSetSchedulers(virDomainObjPtr vm) return -1; } - for (i = 0; i < vm->def->niothreadids; i++) { - if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id, - vm->def->iothreadids[i]->thread_id, - vm->def->cputune.niothreadsched, - vm->def->cputune.iothreadsched) < 0) - return -1; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + for (i = 0; i < vm->def->niothreadids; i++) { + if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id, + vm->def->iothreadids[i]->thread_id, + vm->def->cputune.niothreadsched, + vm->def->cputune.iothreadsched) < 0) + return -1; + } } return 0; -- 2.1.0

On 10/13/2015 12:08 PM, Peter Krempa wrote: hmm... I guess these changes were more CYA than anything else... Look for any callers to the API's that were doing actions based on niothreadids and make sure we add the caps check. Blind ambition. I'll adjust this patch to be just the if (def->niothreads == 0) return 0; in qemuSetupCgroupForIOThreads John

On Tue, Oct 13, 2015 at 11:47:08 -0400, John Ferlan wrote: Here this function should return that the iothread could not be found as such VM shouldn't allow any iothreads if we don't support them. Peter

Create a separate local API that will fill in the iothreadid array entries that were not defined by <iothread id='#'> entries in the XML. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 47 +++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 65e0d8e..217179d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2329,6 +2329,35 @@ virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, } +static int +virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) +{ + unsigned int iothread_id = 1; + int retval = -1; + + /* Same value (either 0 or some number), then we have none to fill in or + * the iothreadid array was filled from the XML + */ + if (def->iothreads == def->niothreadids) + return 0; + + while (def->niothreadids != def->iothreads) { + if (!virDomainIOThreadIDFind(def, iothread_id)) { + virDomainIOThreadIDDefPtr iothrid; + + if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) + goto error; + iothrid->autofill = true; + } + iothread_id++; + } + retval = 0; + + error: + return retval; +} + + void virDomainPinDefFree(virDomainPinDefPtr def) { @@ -14979,22 +15008,8 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(nodes); - /* If no iothreadid's or not fully populated, let's finish the job - * here rather than in PostParseCallback - */ - if (def->iothreads && def->iothreads != def->niothreadids) { - unsigned int iothread_id = 1; - while (def->niothreadids != def->iothreads) { - if (!virDomainIOThreadIDFind(def, iothread_id)) { - virDomainIOThreadIDDefPtr iothrid; - - if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) - goto error; - iothrid->autofill = true; - } - iothread_id++; - } - } + if (virDomainIOThreadIDDefArrayInit(def) < 0) + goto error; /* Extract cpu tunables. */ if ((n = virXPathULong("string(./cputune/shares[1])", ctxt, -- 2.1.0

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 in order to first detect whether there are any iothreadid's defined in the XML. If not, then rather than add one at a time searching for the next valid id, just allocate the whole array and populate it as designed starting at iothread_id = 1 up to the number of iothreads defined in the XML Otherwise, we have a situation where "some number" of iothreadid's were defined in the XML and we're filling in the holes of iothread_id's. Thus, instead of determining if the iothread_id was used (ThreadIDFind) for every ID entry that needs to be filled in, let's only call the find while we still have holes and rather than additionally calling ThreadIDAdd (which also calls the ThreadIDFind), let's just directly add the entry. These algorithm changes only "penalize" those with a large iothreads value that also have a large number of iothread id's which aren't completely defined. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 46 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 217179d..6c90653 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2334,6 +2334,8 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) { unsigned int iothread_id = 1; int retval = -1; + size_t i; + virDomainIOThreadIDDefPtr iothrid = 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,15 +2343,49 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) if (def->iothreads == def->niothreadids) return 0; - while (def->niothreadids != def->iothreads) { - if (!virDomainIOThreadIDFind(def, iothread_id)) { - virDomainIOThreadIDDefPtr iothrid; + /* Optimize - there are no <iothread id='#'> in the XML */ + if (def->niothreadids == 0) { + if (VIR_ALLOC_N(def->iothreadids, def->iothreads) < 0) + goto error; + def->niothreadids = def->iothreads; + for (i = 1; i <= def->iothreads; i++) { + if (VIR_ALLOC(iothrid) < 0) + goto error; + def->iothreadids[i - 1] = iothrid; + iothrid->iothread_id = i; + iothrid->autofill = true; + } + } else { + int found = 0; + int orig_nids = def->niothreadids; + + /* <iothread id='#'> entries were found, then let's fill in the + * holes one at a time, e.g. the relatively hard way. Rather than + * using ThreadIDFind and call ThreadIDAdd which also calls + * ThreadIDFind again which could cause lots of needless spinning + * let's just add the entries directly + */ + for (i = 0; + i < def->iothreads && def->niothreadids != def->iothreads; i++) { + /* While we still have defined <thread id='#'>'s compare our + * current thread_id value against the array. + */ + if (found < orig_nids && + virDomainIOThreadIDFind(def, iothread_id)) { + iothread_id++; + found++; + continue; + } - if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) + /* Add a new entry using the current iothread_id */ + if (VIR_ALLOC(iothrid) < 0) goto error; + iothrid->iothread_id = iothread_id++; iothrid->autofill = true; + if (VIR_APPEND_ELEMENT_COPY(def->iothreadids, def->niothreadids, + iothrid) < 0) + goto error; } - iothread_id++; } retval = 0; -- 2.1.0

On 10/13/2015 12:29 PM, Peter Krempa wrote: Yeah - I agree, but since we didn't set a limit... I think to a degree we are stuck. Sometimes I wonder about the reality of tests. Then again, I seem to recall a recent dialog over the number of nics (and thus hwaddrs) that were deemed reasonable when the algorithm was first written... This would work if we required thread_id values to be <= def->iothreads, but we don't, yet... I thought of using a bitmap, but it would no cover the following case : <iothreads>4</iothreads> <iothreadids> <iothread id='100'/> <iothreadids> This would/should create thread ids 100, 1, 2, 3 We never placed a limit on the thread_id value other than it cannot be zero. Although we do indicate the default algorithm is 1 to the number of iothreads. Would a 'real world' user do something like this - perhaps not. Much less make "<iothreads>1000000</iothreads>" (that'd be one long command line - say nothing of the resources required in order to really start it. I'd be all for restricting iothread id values to be <= def->iothreads - that'd solve the unnecessarily complex algorithm. My other thought was to restrict the number of iothreads to be the number of disk devices or even 2 * ndisks. It's possible to assign multiple disks to 1 iothread, but impossible to assign 1 disk to multiple threads. Well 80 cols max is the desired number. Although in thinking about it, I don't think we could ever hit the i == def->iothreads - if we did then there was another error. Removing it moves us back down to < 80 all on one for line. Suffice to say those VIR_APPEND_* are not necessarily self documenting. I assume though you are indicating using VIR_RESIZE_N. However, that may not work in 'all cases'. This algorithm handles the case where there's 5 iothreads and let's say id "2" and "5" are defined. There's no way to "know" what the id's are without looking and I gave up sorting them. Again, assuming iothread_id can be > def->iothreads. John