[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]