...
>>>> +
>>>> + 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list