[libvirt] [PATCH] qemu: Remove maximum cpu limit when setting processor count using the API
When setting processor count for a domain using the API libvirt enforced a maximum processor count, while it isn't enforced when taking the XML path. This patch removes the check to match the API. --- src/qemu/qemu_driver.c | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3f62414..833bf7f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3737,8 +3737,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; virDomainDefPtr persistentDef; - const char * type; - int max; int ret = -1; bool maximum; virQEMUDriverConfigPtr cfg = NULL; @@ -3778,27 +3776,11 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, goto endjob; } - if (!(type = virDomainVirtTypeToString(vm->def->virtType))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown virt type in domain definition '%d'"), - vm->def->virtType); - goto endjob; - } - - if ((max = qemuGetMaxVCPUs(NULL, type)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("could not determine max vcpus for the domain")); - goto endjob; - } - - if (!maximum && vm->def->maxvcpus < max) { - max = vm->def->maxvcpus; - } - - if (nvcpus > max) { + if (!maximum && nvcpus > vm->def->maxvcpus) { virReportError(VIR_ERR_INVALID_ARG, _("requested vcpus is greater than max allowable" - " vcpus for the domain: %d > %d"), nvcpus, max); + " vcpus for the domain: %d > %d"), + nvcpus, vm->def->maxvcpus); goto endjob; } -- 1.8.1.5
On Fri, Apr 05, 2013 at 12:11:31AM +0200, Peter Krempa wrote:
When setting processor count for a domain using the API libvirt enforced a maximum processor count, while it isn't enforced when taking the XML path.
This patch removes the check to match the API.
Do you mean s/API/XML/ here ? I'm not sure whether removing this check is a good idea. Should we not instead make the guest startup code also validate max CPU count when generating the CLI args ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 04/05/13 10:37, Daniel P. Berrange wrote:
On Fri, Apr 05, 2013 at 12:11:31AM +0200, Peter Krempa wrote:
When setting processor count for a domain using the API libvirt enforced a maximum processor count, while it isn't enforced when taking the XML path.
This patch removes the check to match the API.
Do you mean s/API/XML/ here ?
Indeed.
I'm not sure whether removing this check is a good idea. Should we not instead make the guest startup code also validate max CPU count when generating the CLI args ?
Well, I don't think so. Adding the check to the CLI generator would introduce more problems than it would solve: 1) chicken and egg problem: we can't use the new QMP query-cpu-max command as we need a running qemu for this and in order to start a qemu you already need to provide the desired number of cpus: $ qemu-system-x86_64 -smp 256 -M pc -S --monitor stdio Unsupported number of maxcpus $ qemu-system-x86_64 -smp 255 -M pc -S --monitor stdio QEMU 1.4.50 monitor - type 'help' for more information (qemu) info cpu_max Maximum number of CPUs is 255 (qemu) quit $ 2) qemuGetMaxVCPUs() is obsolete and doesn't report apropriate numbers. For non-kvm guests this would limit us to 16 cpus per guest and for kvm guests it depends if the kernel supports the KVM_CAP_MAX_VCPUS ioctl (introduced in linux-3.2) and reports a correct value or we return the recommended value of processors (KVM_CAP_NR_VCPUS ioctl) that is hardcoded to 160. If neither of the ioctls is supported, 4 will be returned as the maximum count (according to kernel docs). qemuGetMaxVCPUs(virConnectPtr conn ATTRIBUTE_UNUSED, const char *type) { if (!type) return 16; if (STRCASEEQ(type, "qemu")) return 16; if (STRCASEEQ(type, "kvm")) return kvmGetMaxVCPUs(); if (STRCASEEQ(type, "kqemu")) return 1; ... I don't see a way how we could reliably determine the maximum for a guest before we start it and it doesn't make sense to start it to find out. I think we should just remove the check and let qemu handle the limit. Peter
On Fri, Apr 05, 2013 at 11:13:17AM +0200, Peter Krempa wrote:
On 04/05/13 10:37, Daniel P. Berrange wrote:
On Fri, Apr 05, 2013 at 12:11:31AM +0200, Peter Krempa wrote:
When setting processor count for a domain using the API libvirt enforced a maximum processor count, while it isn't enforced when taking the XML path.
This patch removes the check to match the API.
Do you mean s/API/XML/ here ?
Indeed.
I'm not sure whether removing this check is a good idea. Should we not instead make the guest startup code also validate max CPU count when generating the CLI args ?
Well, I don't think so. Adding the check to the CLI generator would introduce more problems than it would solve:
1) chicken and egg problem: we can't use the new QMP query-cpu-max command as we need a running qemu for this and in order to start a qemu you already need to provide the desired number of cpus:
$ qemu-system-x86_64 -smp 256 -M pc -S --monitor stdio Unsupported number of maxcpus $ qemu-system-x86_64 -smp 255 -M pc -S --monitor stdio QEMU 1.4.50 monitor - type 'help' for more information (qemu) info cpu_max Maximum number of CPUs is 255 (qemu) quit $
2) qemuGetMaxVCPUs() is obsolete and doesn't report apropriate numbers. For non-kvm guests this would limit us to 16 cpus per guest and for kvm guests it depends if the kernel supports the KVM_CAP_MAX_VCPUS ioctl (introduced in linux-3.2) and reports a correct value or we return the recommended value of processors (KVM_CAP_NR_VCPUS ioctl) that is hardcoded to 160. If neither of the ioctls is supported, 4 will be returned as the maximum count (according to kernel docs).
qemuGetMaxVCPUs(virConnectPtr conn ATTRIBUTE_UNUSED, const char *type) { if (!type) return 16;
if (STRCASEEQ(type, "qemu")) return 16;
if (STRCASEEQ(type, "kvm")) return kvmGetMaxVCPUs();
if (STRCASEEQ(type, "kqemu")) return 1;
...
I don't see a way how we could reliably determine the maximum for a guest before we start it and it doesn't make sense to start it to find out. I think we should just remove the check and let qemu handle the limit.
Ok, that makes sense now. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 04/05/13 11:21, Daniel P. Berrange wrote:
On Fri, Apr 05, 2013 at 11:13:17AM +0200, Peter Krempa wrote:
On 04/05/13 10:37, Daniel P. Berrange wrote:
On Fri, Apr 05, 2013 at 12:11:31AM +0200, Peter Krempa wrote:
When setting processor count for a domain using the API libvirt enforced a maximum processor count, while it isn't enforced when taking the XML path.
This patch removes the check to match the API.
Do you mean s/API/XML/ here ?
Indeed.
I've fixed the commit message and ...
...
I don't see a way how we could reliably determine the maximum for a guest before we start it and it doesn't make sense to start it to find out. I think we should just remove the check and let qemu handle the limit.
Ok, that makes sense now.
... pushed the patch now. If we decide that we still want to check the user supplied values we will need to do better than the code this patch removes. Thanks. Peter
participants (2)
-
Daniel P. Berrange -
Peter Krempa