8:37 a.m.
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(a)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