[libvirt] [PATCH v5 04/10] Move iothreadspin information into iothreadids

[John Ferlan]

On 04/27/2015 10:20 AM, Peter Krempa wrote:
> On Fri, Apr 24, 2015 at 12:05:56 -0400, John Ferlan wrote:
>> Remove the iothreadspin array from cputune and replace with a cpumask
>> to be stored in the iothreadids list.
>>
>> Adjust the test output because our printing goes in order of the iothreadids
>> list now.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  docs/formatdomain.html.in                          |  10 +-
>>  src/conf/domain_conf.c                             | 112 ++++++++++-----------
>>  src/conf/domain_conf.h                             |   3 +-
>>  src/qemu/qemu_cgroup.c                             |  16 +--
>>  src/qemu/qemu_driver.c                             |  75 ++++----------
>>  src/qemu/qemu_process.c                            |  10 +-
>>  .../qemuxml2xmlout-cputune-iothreads.xml           |   2 +-
>>  7 files changed, 86 insertions(+), 142 deletions(-)
>>
> 
> ...
> 
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index ddb0d83..129908d 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
> 
> ...
> 
>> @@ -20862,16 +20859,19 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>>          VIR_FREE(cpumask);
>>      }
>>  
>> -    for (i = 0; i < def->cputune.niothreadspin; i++) {
>> +    for (i = 0; i < def->niothreadids; i++) {
>>          char *cpumask;
>> -        /* Ignore the iothreadpin which inherit from "cpuset of "<vcpu>." */
>> -        if (virBitmapEqual(def->cpumask, def->cputune.iothreadspin[i]->cpumask))
>> +
>> +        /* 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;
> 
> I still think that comparing the cpu map with the default cpumap is
> wrong. It should not be copied there in the first place. Additionally if
> the user specifies the same CPU map as the default one, we should not
> remove it.
> 

Removed the "virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask)"
check.

John

>>  
>>          virBufferAsprintf(buf, "<iothreadpin iothread='%u' ",
>> -                          def->cputune.iothreadspin[i]->id);
>> +                          def->iothreadids[i]->iothread_id);
>>  
>> -        if (!(cpumask = virBitmapFormat(def->cputune.iothreadspin[i]->cpumask)))
>> +        if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask)))
>>              goto error;
>>  
>>          virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask);
> 
> ...
> 
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 330ffdf..1fac0b8 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
> 
> ...
> 
>> @@ -6145,31 +6114,23 @@ 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;
>> -        }
>> +
>> +        virBitmapFree(iothrid->cpumask);
>> +        iothrid->cpumask = cpumask;
> 
> Much nicer!
> 
>>  
>>          ret = virDomainSaveConfig(cfg->configDir, persistentDef);
>>          goto endjob;
> 
> The code is much cleaner now. The cpu threads/pinning implementation is
> horrible in this aspect.
> 
> ACK with the bitmap comparison removed.
> 
> Peter
>