[libvirt] [PATCH] qemu: match vcpu topology to maximum vcpu numbers.
For qemu, if the -smp N or the value of maxcpus is given, it define the number of vcpu of guest whenever the vcpu topology is defined or not. But if the -smp N and maxcpus are missing, the topology can compute and define vcpus for guest automatically by math: vcpu number = sockets*cores*threads For libvirt, as <vcpu> is always mandatory, so we can ask topology to match maximum vcpu numbers. --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a9656af..ffdc6da 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11815,10 +11815,10 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; if (def->cpu->sockets && - def->maxvcpus > + def->maxvcpus != def->cpu->sockets * def->cpu->cores * def->cpu->threads) { virReportError(VIR_ERR_XML_DETAIL, "%s", - _("Maximum CPUs greater than topology limit")); + _("Topology limit does not match maximum CPUs")); goto error; } -- 1.8.1.4
On 05/28/2013 03:59 AM, Guannan Ren wrote:
For qemu, if the -smp N or the value of maxcpus is given, it define the number of vcpu of guest whenever the vcpu topology is defined or not. But if the -smp N and maxcpus are missing, the topology can compute and define vcpus for guest automatically by math:
vcpu number = sockets*cores*threads
For libvirt, as <vcpu> is always mandatory, so we can ask topology to match maximum vcpu numbers. --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a9656af..ffdc6da 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11815,10 +11815,10 @@ virDomainDefParseXML(xmlDocPtr xml, goto error;
if (def->cpu->sockets && - def->maxvcpus > + def->maxvcpus != def->cpu->sockets * def->cpu->cores * def->cpu->threads) { virReportError(VIR_ERR_XML_DETAIL, "%s", - _("Maximum CPUs greater than topology limit")); + _("Topology limit does not match maximum CPUs"));
Is this going to reject XML that was previously accepted? Is there a bugzilla showing what happens if this patch is not incorporated? I'm worried about introducing an unintentional regression if we include this in 1.0.6 without more justification. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/29/2013 04:20 AM, Eric Blake wrote:
On 05/28/2013 03:59 AM, Guannan Ren wrote:
For qemu, if the -smp N or the value of maxcpus is given, it define the number of vcpu of guest whenever the vcpu topology is defined or not. But if the -smp N and maxcpus are missing, the topology can compute and define vcpus for guest automatically by math:
vcpu number = sockets*cores*threads
For libvirt, as <vcpu> is always mandatory, so we can ask topology to match maximum vcpu numbers. --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a9656af..ffdc6da 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11815,10 +11815,10 @@ virDomainDefParseXML(xmlDocPtr xml, goto error;
if (def->cpu->sockets && - def->maxvcpus > + def->maxvcpus != def->cpu->sockets * def->cpu->cores * def->cpu->threads) { virReportError(VIR_ERR_XML_DETAIL, "%s", - _("Maximum CPUs greater than topology limit")); + _("Topology limit does not match maximum CPUs")); Is this going to reject XML that was previously accepted? Is there a bugzilla showing what happens if this patch is not incorporated? I'm worried about introducing an unintentional regression if we include this in 1.0.6 without more justification.
Yes, it make the cpu topology limit setting more strict. Alternatively, we can add more comments for <topology> to make it clear that <vcpu> decide the number of vcpus, the sockets*cores*threads had better be equal to vcpu numbers. The bz:https://bugzilla.redhat.com/show_bug.cgi?id=880017 Description of problem: libvirt should check if vcpu topology is right. If the wrong vcpu topology is given in xml , the wrong arguments also be passed to qemu-kvm. vcpu number = sockets*cores*threads Steps to Reproduce: 1.# virsh start vm Domain vm started 2.# virsh dumpxml vm <domain type='kvm' id='104'> ....... <vcpu placement='static'>4</vcpu> ...... <cpu> <topology sockets='1' cores='4' threads='2'/> </cpu> 3.# ps -ef|grep qemu-kvm qemu 21296 1 14 16:41 ? 00:00:17 /usr/libexec/qemu-kvm -name vm -S -M rhel6.4.0 -enable-kvm -m 1024 -smp 4,sockets=1,cores=4,threads=2 ...... Actual results: Wrong vcpu topology can be given in xml and passed to qemu-kvm Expected results: libvirt should check if vcpu topology is right.
On 05/28/2013 07:32 PM, Guannan Ren wrote:
if (def->cpu->sockets && - def->maxvcpus > + def->maxvcpus != def->cpu->sockets * def->cpu->cores * def->cpu->threads) { virReportError(VIR_ERR_XML_DETAIL, "%s", - _("Maximum CPUs greater than topology limit")); + _("Topology limit does not match maximum CPUs"));
Is this going to reject XML that was previously accepted? Is there a bugzilla showing what happens if this patch is not incorporated? I'm worried about introducing an unintentional regression if we include this in 1.0.6 without more justification.
Yes, it make the cpu topology limit setting more strict.
I'm okay with making things more strict, if it makes sense. But isn't it feasible to support a topology that has more slots than the maximum? That is, since a bare metal machine can have two sockets but only populate one, why can't a virtual machine have 2 sockets but set a max vcpu of 1 so that only one socket can be populated? Does qemu itself enforce that there be a match? That's why I'm worried about enforcing that the user can't have a mismatch between maxvcpus vs. topology, if they specify both. I'm also worried that this check might fire if the user specifies only topology or only maxvcpus; I want to make sure that we only do the sanity checking if both numbers are provided.
Alternatively, we can add more comments for <topology> to make it clear that <vcpu> decide the number of vcpus, the sockets*cores*threads had better be equal to vcpu numbers.
If there is a problem with a mismatch, then we should document that a match is required, and fail on mismatch. Merely documenting that a match is required without enforcing it is too weak; and enforcing it without documentation is a disservice. I haven't ruled out enforcing a match, but want to make sure that we aren't artificially crippling something that might prove useful in reality, when compared with what bare-metal setups can provide.
The bz:https://bugzilla.redhat.com/show_bug.cgi?id=880017
Description of problem: libvirt should check if vcpu topology is right. If the wrong vcpu topology is given in xml , the wrong arguments also be passed to qemu-kvm.
vcpu number = sockets*cores*threads
Steps to Reproduce: 1.# virsh start vm Domain vm started
2.# virsh dumpxml vm <domain type='kvm' id='104'> ....... <vcpu placement='static'>4</vcpu> ...... <cpu> <topology sockets='1' cores='4' threads='2'/> </cpu>
3.# ps -ef|grep qemu-kvm qemu 21296 1 14 16:41 ? 00:00:17 /usr/libexec/qemu-kvm -name vm -S -M rhel6.4.0 -enable-kvm -m 1024 -smp 4,sockets=1,cores=4,threads=2 ......
Actual results: Wrong vcpu topology can be given in xml and passed to qemu-kvm
Expected results: libvirt should check if vcpu topology is right.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- docs/formatdomain.html.in | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 755d084..27c1580 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -941,7 +941,9 @@ virtual CPU provided to the guest. Three non-zero values have to be given for <code>sockets</code>, <code>cores</code>, and <code>threads</code>: total number of CPU sockets, number of cores per - socket, and number of threads per core, respectively.</dd> + socket, and number of threads per core, respectively. The maximum number + of virtual CPUs is defined by <code>vcpu</code> element, the product of + the three valuses should be equal to the maximum vcpus.</dd> <dt><code>feature</code></dt> <dd>The <code>cpu</code> element can contain zero or more -- 1.8.1.4
On 05/28/2013 08:53 PM, Guannan Ren wrote:
--- docs/formatdomain.html.in | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 755d084..27c1580 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -941,7 +941,9 @@ virtual CPU provided to the guest. Three non-zero values have to be given for <code>sockets</code>, <code>cores</code>, and <code>threads</code>: total number of CPU sockets, number of cores per - socket, and number of threads per core, respectively.</dd> + socket, and number of threads per core, respectively. The maximum number + of virtual CPUs is defined by <code>vcpu</code> element, the product of + the three valuses should be equal to the maximum vcpus.</dd>
s/valuses/values/ Again, if we are going to document that they must be equal, then the code should match; otherwise, if we are going to allow a difference between the two, then the wording should be a bit nicer ("It is recommended that if both maxvcpu and topology are provided, then the two numbers should match"). Or maybe a one-sided restriction makes sense, where maxvcpus can be less than or equal, but not greater than, an explicit topology. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/29/2013 11:14 AM, Eric Blake wrote:
On 05/28/2013 08:53 PM, Guannan Ren wrote:
--- docs/formatdomain.html.in | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
There are little document about how to set values of sockets, cores, threads properly. After going through qemu code, I summarize the package/cores/threads/ algorithm which can be verified by 'cpuid' tool. The following only explains the case of all the four number are given in qemu commandline. smp = n (argument to -smp) nr_threads = smp / (cores * sockets) nr_cores = cores -smp 4, sockets=1,cores=4,threads=2 leads to: 1(physical cpu) *4(cores) *1(thread) One physical cpu contains four cores log processor which each contains one thread logical processor. 0 means only one physical cpu package pkg_id = cpu_index/nr_threads/nr_cores 0 = 0 / 1 / 4 0 = 1 / 1 / 4 0 = 2 / 1 / 4 0 = 3 / 1 / 4 3 means there are four multi-cores logical processor core_id = cpu_index/nr_threads % nr_cores 0 = 0 / 1 % 4 1 = 1 / 1 % 4 2 = 2 / 1 % 4 3 = 3 / 1 % 4 0 means only one thread logical processor smt_id = cpu_index % nr_threads 0 = 0 % 1 0 = 1 % 1 0 = 2 % 1 0 = 3 % 1 And the result could be verified by 'cpuid' tool running in fedora guest. 1st vcpu: (APIC synth): PKG_ID=0 CORE_ID=0 SMT_ID=0 2nd vcpu: (APIC synth): PKG_ID=0 CORE_ID=1 SMT_ID=0 3rd vcpu: (APIC synth): PKG_ID=0 CORE_ID=2 SMT_ID=0 4th vcpu: (APIC synth): PKG_ID=0 CORE_ID=3 SMT_ID=0 I tried it on several cpu topology: -smp 16, sockets=2,cores=2,threads=1 leads to 2 * 2 * 4 There are two physical cpu package. Each of them contains two cores logical processor and each of the cores contains four hyper-threading logical processor. The result could be verified by cpuid tool. If the calculation is true, I think my patch is self-acked. Guannan
participants (2)
-
Eric Blake -
Guannan Ren