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

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



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

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

> +                return -1;
> +            }
> +        }
>  
> -    return ret;
> +        while ((bit = virBitmapNextSetBit(nodeset, bit)) >= 0) {
> +            if (bit <= maxvcpus-1)

Incorrect spacing around the '-' operator.

> +                continue;

This construct more-or-less reimplements virBitmapLastSetBit()

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

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

Peter

Attachment: signature.asc
Description: Digital signature


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