[libvirt] [PATCHv2] conf: rework the cpu check for vm numa settings

[Luyao Huang]

On 04/22/2015 08:55 PM, Peter Krempa wrote:
> On Wed, Apr 22, 2015 at 20:34:55 +0800, Luyao Huang wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1176020
>>
>> We had a check for the vcpu count total number in <numa>
>> before, however this check is not good enough. There are
>> some examples:
>>
>> 1. one of cpu id is out of maxvcpus, can set success(cpu count = 5 < 10):
>>
>> <vcpu placement='static'>10</vcpu>
>> <cell id='0' cpus='0-3,100' memory='512000' unit='KiB'/>
>>
>> 2. use the same cpu in 2 cell, can set success(cpu count = 8 < 10):
>> <vcpu placement='static'>10</vcpu>
>> <cell id='0' cpus='0-3' memory='512000' unit='KiB'/>
>> <cell id='1' cpus='0-3' memory='512000' unit='KiB'/>
>>
>> 3. use the same cpu in 2 cell, cannot set success(cpu count = 11 > 10):
>> <vcpu placement='static'>10</vcpu>
>> <cell id='0' cpus='0-6' memory='512000' unit='KiB'/>
>> <cell id='1' cpus='0-3' memory='512000' unit='KiB'/>
>>
>> Use a new check for numa cpus, check if use a cpu exceeds maxvcpus
>> and if duplicate use one cpu in more than one cell.
>>
>> Signed-off-by: Luyao Huang <lhuang at redhat.com>
>> ---
>>   src/conf/domain_conf.c |  6 +-----
>>   src/conf/numa_conf.c   | 37 ++++++++++++++++++++++++++++++-------
>>   src/conf/numa_conf.h   |  2 +-
>>   3 files changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 479b4c2..a4a2abb 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -14234,12 +14234,8 @@ virDomainDefParseXML(xmlDocPtr xml,
>>       if (virDomainNumaDefCPUParseXML(def->numa, ctxt) < 0)
>>           goto error;
>>   
>> -    if (virDomainNumaGetCPUCountTotal(def->numa) > def->maxvcpus) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -                       _("Number of CPUs in <numa> exceeds the"
>> -                         " <vcpu> count"));
>> +    if (virDomainNumaCheckCPU(def->numa, def->maxvcpus) < 0)
> This check could be placed after all the numa nodes are parsed and thus
> would function correctly when combined with ...

Indeed

>
>>           goto error;
>> -    }
>>   
>>       if (virDomainNumatuneParseXML(def->numa,
>>                                     def->placement_mode ==
>> diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
>> index 7ad3f66..2b18225 100644
>> --- a/src/conf/numa_conf.c
>> +++ b/src/conf/numa_conf.c
>> @@ -805,16 +805,39 @@ virDomainNumaDefCPUFormat(virBufferPtr buf,
>>   }
>>   
>>   
>> -unsigned int
>> -virDomainNumaGetCPUCountTotal(virDomainNumaPtr numa)
>> +int
>> +virDomainNumaCheckCPU(virDomainNumaPtr numa,
>> +                      unsigned short maxvcpus)
>>   {
>> -    size_t i;
>> -    unsigned int ret = 0;
>> +    size_t i,j;
>>   
>> -    for (i = 0; i < numa->nmem_nodes; i++)
>> -        ret += virBitmapCountBits(virDomainNumaGetNodeCpumask(numa, i));
>> +    for (i = 0; i < numa->nmem_nodes; i++) {
>> +        virBitmapPtr nodeset = NULL;
>> +        ssize_t bit = -1;
>> +
>> +        nodeset = virDomainNumaGetNodeCpumask(numa, i);
>> +        for (j = 0; j < i; j++) {
>> +            if (virBitmapOverlaps(virDomainNumaGetNodeCpumask(numa, j),
>> +                                  nodeset)) {
> ... this check.
>
>> +                virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                               _("Cannot binding one vCPU in 2 NUMA cell"
>> +                                 " %zu and %zu"), i, j);
> This error message doesn't look very explanatory. Perhaps "NUMA cells
> %zu and %zu have overlapping vCPU ids".

Good error.

>
>> +                return -1;
>> +            }
>> +        }
>>   
>> -    return ret;
>> +        while ((bit = virBitmapNextSetBit(nodeset, bit)) >= 0) {
>> +            if (bit <= maxvcpus-1)
> Incorrect spacing around the '-' operator.

right, this should be a typo mistake :)

>
>> +                continue;
> This construct more-or-less reimplements virBitmapLastSetBit()

Yes, i should use virBitmapLastSetBit() in this place.

>
>> +
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("vcpu '%zu' in <numa> cell '%zu' exceeds the maxvcpus"),
>> +                           bit, i);
> This check looks awkward. I'd go with the virDomainNumaGetCPUCountTotal
> and add the check for overlapping indexes.

Hmm...okay, i think this function (virDomainNumaGetCPUCountTotal) should 
be renamed because this function's function will be changed after these fix.

Thanks for your quick review and help.

>> +            return -1;
>> +        }
>> +    }
>> +
>> +    return 0;
>>   }
> Peter

Luyao