...
+ + if (!(iothrid = virDomainIOThreadIDFind(def, iothreadid))) { + if (!(iothrid = virDomainIOThreadIDAdd(def, iothreadid)))
Why are you adding a iothread definition here? This code is executed after the iothread info should already be parsed so adding a thread here doesn't make sense. The section here should ressemble the check [1] that you've removed.
Because you required that the iothreadpin information to be included in the iothreadid; however, if someone hasn't defined <iothreadids>'s, then there won't be any until the PostParse is called - thus we have to add one here so that we can store the pin information for an iothread
Okay, I didn't see that because I've remembered a different version of patch 1 where you allocated the full list upfront.
By the way, the approach in this series now has the following loophole:
1) Specify <iothreads> > nelemes(<iothreadids>) 2) Specify iothreadpin for thread 9999.
Now one of the threads is added with id 9999 due to the code above.
Thus I think that the PostParse callback code should probably be removed and the missing threads should be numbered right after <iohtreadids> is parsed so that this patch doesn't allow that.
I've moved the code in patch 1 ...
- virBufferAsprintf(buf, "<iothreadpin iothread='%u' ", - def->cputune.iothreadspin[i]->id); + /* Ignore iothreadids with no cpumask or a cpumask that + * inherits from "cpuset of "<vcpu>." */ + if (!def->iothreadids[i]->cpumask || + virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask)) + continue;
... here. Also why are you explicitly rejecting cpumask that is equal to the default one? If a user explicitly specifies it that way then the info would be lost. Additionally, def->cpumask here is used on the iothread automatically without filling it to the structure if it's used so there's no need to do that.
Prior review - it's already there - see commit id '938fb12f'.... I'm fairly certain I was specifically asked to do that...
It doesn't make much sense though. The default pinning is used from def->cpumap if the specific is not present. If the specific pinning is removed it should be freed.
OK, but I contend this needs to be a separate patch after this series so that all 3 are fixed at once. ...
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index cdf0aaf..5282449 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1149,7 +1149,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) virCgroupPtr cgroup_iothread = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; - size_t i, j; + size_t i; unsigned long long period = vm->def->cputune.period; long long quota = vm->def->cputune.quota; char *mem_mask = NULL; @@ -1214,18 +1214,11 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) /* default cpu masks */ if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) cpumask = priv->autoCpuset; + else if (def->iothreadids[i]->cpumask) + cpumask = def->iothreadids[i]->cpumask;
This has to be the first case. Even if VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO is specified, it cannot override what the user configured pinning.
OK, but see qemuSetupCgroupForVcpu and qemuSetupCgroupForEmulator... Vcpu has one way of doing it and Emulator slightly different, but both check AUTO first.
VCPU checks AUTO first, then sets to default. Then it checks the vcpupin and resets to whatever is there.
Emulator checks AUTO first, then it checks if emulatorpin is set and uses that, and finally falls back to default if defined.
Um, I made a mistake when I refactored qemuSetupCgroupForEmulator. qemuSetupCgroupForVcpu is the right approach.
I will follow ForVcpu and someone else can fix ForEmulator
I followed emulatorpin
If I'm wrong, then emulator and vcpu are wrong, but I think fixing that is a separate patch.
yes.
else cpumask = def->cpumask;
- /* specific cpu mask */ - for (j = 0; j < def->cputune.niothreadspin; j++) { - if (def->cputune.iothreadspin[j]->id == - def->iothreadids[i]->iothread_id) { - cpumask = def->cputune.iothreadspin[j]->cpumask; - break; - }
Finally, this can be removed!
- } - if (cpumask && qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f95cc7..ee13d08 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5894,19 +5894,14 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, goto cleanup;
for (i = 0; i < targetDef->niothreadids; i++) { - virDomainPinDefPtr pininfo; - if (VIR_ALLOC(info_ret[i]) < 0) goto cleanup;
/* IOThread ID's are taken from the iothreadids list */ info_ret[i]->iothread_id = targetDef->iothreadids[i]->iothread_id;
- /* Initialize the cpumap */ - pininfo = virDomainPinFind(targetDef->cputune.iothreadspin, - targetDef->cputune.niothreadspin, - targetDef->iothreadids[i]->iothread_id); - if (!pininfo) { + cpumask = targetDef->iothreadids[i]->cpumask; + if (!cpumask) { if (targetDef->cpumask) { cpumask = targetDef->cpumask; } else { @@ -5915,8 +5910,6 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, virBitmapSetAll(bitmap); cpumask = bitmap; } - } else { - cpumask = pininfo->cpumask; } if (virBitmapToData(cpumask, &info_ret[i]->cpumap, &info_ret[i]->cpumaplen) < 0) @@ -5999,8 +5992,6 @@ qemuDomainPinIOThread(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; virBitmapPtr pcpumap = NULL; qemuDomainObjPrivatePtr priv; - virDomainPinDefPtr *newIOThreadsPin = NULL; - size_t newIOThreadsPinNum = 0; virCgroupPtr cgroup_iothread = NULL; virObjectEventPtr event = NULL; char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; @@ -6050,33 +6041,21 @@ qemuDomainPinIOThread(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) {
virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask;
if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value %d not found"), iothread_id); + _("iothreadid %d not found"), iothread_id);
I'd compromise between those two.
"iothread %d not foud"
OK
goto endjob; }
- if (vm->def->cputune.iothreadspin) { - newIOThreadsPin = - virDomainPinDefCopy(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - if (!newIOThreadsPin) - goto endjob; - - newIOThreadsPinNum = vm->def->cputune.niothreadspin; - } else { - if (VIR_ALLOC(newIOThreadsPin) < 0) - goto endjob; - newIOThreadsPinNum = 0; - } - - if (virDomainPinAdd(&newIOThreadsPin, &newIOThreadsPinNum, - cpumap, maplen, iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update iothreadspin")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + vm->def->cputune.niothreadspin++;
This should go. If you add a function that checks whether pinning s specified it will not be needed.
Yep
+ virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask;
/* Configure the corresponding cpuset cgroup before set affinity. */ if (virCgroupHasController(priv->cgroup, @@ -6099,14 +6078,6 @@ qemuDomainPinIOThread(virDomainPtr dom, } }
- if (vm->def->cputune.iothreadspin) - virDomainPinDefArrayFree(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); - - vm->def->cputune.iothreadspin = newIOThreadsPin; - vm->def->cputune.niothreadspin = newIOThreadsPinNum; - newIOThreadsPin = NULL; - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob;
@@ -6124,31 +6095,25 @@ qemuDomainPinIOThread(virDomainPtr dom, }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + virDomainIOThreadIDDefPtr iothrid; + virBitmapPtr cpumask; + /* Coverity didn't realize that targetDef must be set if we got here. */ sa_assert(persistentDef);
- if (iothread_id > persistentDef->iothreads) { + if (!(iothrid = virDomainIOThreadIDFind(persistentDef, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, - _("iothread value out of range %d > %d"), - iothread_id, persistentDef->iothreads); + _("iothreadid %d not found"), iothread_id); goto endjob; }
- if (!persistentDef->cputune.iothreadspin) { - if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) - goto endjob; - persistentDef->cputune.niothreadspin = 0; - } - if (virDomainPinAdd(&persistentDef->cputune.iothreadspin, - &persistentDef->cputune.niothreadspin, - cpumap, - maplen, - iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add iothreadspin xml " - "of a persistent domain")); + if (!(cpumask = virBitmapNewData(cpumap, maplen))) goto endjob; - } + + if (!iothrid->cpumask) + persistentDef->cputune.niothreadspin++;
Again, this should not be necessary.
yep
+ virBitmapFree(iothrid->cpumask); + iothrid->cpumask = cpumask;
ret = virDomainSaveConfig(cfg->configDir, persistentDef); goto endjob;
The code looks much better now, but this patch has still some nits.
Peter
All the following changes to be squashed in:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a191b02..9876557 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2115,6 +2115,19 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin)
Adding patches in plaintex in thunderbird or other client that breaks lines makes them unusable so I don't usually bother checking it.
OK - I'll just resend a v5 with changes tks - John