Re: [libvirt] [PATCH v4 4/9] Move iothreadspin information into iothreadids

>>> + >>> + 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.

>>> - 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.

>>> 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 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.