Re: [libvirt] [PATCH] kvm: warn if num cpus is greater than num recommended
On 22/08/2013, at 12:39, Andrew Jones <drjones@redhat.com> wrote:
The comment in kvm_max_vcpus() states that it's using the recommended procedure from the kernel API documentation to get the max number of vcpus that kvm supports. It is, but by always returning the maximum number supported. The maximum number should only be used for development purposes. qemu should check KVM_CAP_NR_VCPUS for the recommended number of vcpus. This patch adds a warning if a user specifies a number of cpus between the recommended and max.
Signed-off-by: Andrew Jones <drjones@redhat.com>
CCing libvir-list. It is probably interesting for libvirt to expose or warn about the recommended VCPU limit somehow, and in this case a simple warning on stderr won't be enough.
--- kvm-all.c | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/kvm-all.c b/kvm-all.c index 716860f617455..9092e13ae60ea 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -1313,24 +1313,24 @@ static int kvm_irqchip_create(KVMState *s) return 0; }
-static int kvm_max_vcpus(KVMState *s) +/* Find number of supported CPUs using the recommended + * procedure from the kernel API documentation to cope with + * older kernels that may be missing capabilities. + */ +static int kvm_recommended_vcpus(KVMState *s) { int ret;
- /* Find number of supported CPUs using the recommended - * procedure from the kernel API documentation to cope with - * older kernels that may be missing capabilities. - */ - ret = kvm_check_extension(s, KVM_CAP_MAX_VCPUS); - if (ret) { - return ret; - } ret = kvm_check_extension(s, KVM_CAP_NR_VCPUS); - if (ret) { - return ret; - } + return (ret) ? ret : 4; +}
- return 4; +static int kvm_max_vcpus(KVMState *s) +{ + int ret; + + ret = kvm_check_extension(s, KVM_CAP_MAX_VCPUS); + return (ret) ? ret : kvm_recommended_vcpus(s); }
int kvm_init(void) @@ -1383,12 +1383,21 @@ int kvm_init(void) goto err; }
- max_vcpus = kvm_max_vcpus(s); + max_vcpus = kvm_recommended_vcpus(s); if (smp_cpus > max_vcpus) { - ret = -EINVAL; - fprintf(stderr, "Number of SMP cpus requested (%d) exceeds max cpus " - "supported by KVM (%d)\n", smp_cpus, max_vcpus); - goto err; + fprintf(stderr, + "Warning: Number of SMP cpus requested (%d) exceeds " + "recommended cpus supported by KVM (%d)\n", + smp_cpus, max_vcpus); + + max_vcpus = kvm_max_vcpus(s); + if (smp_cpus > max_vcpus) { + ret = -EINVAL; + fprintf(stderr, "Number of SMP cpus requested (%d) exceeds " + "max cpus supported by KVM (%d)\n", + smp_cpus, max_vcpus); + goto err; + } }
s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0); -- 1.8.1.4
-- Eduardo
Am 22.08.2013 18:12, schrieb Eduardo Habkost:
On 22/08/2013, at 12:39, Andrew Jones <drjones@redhat.com> wrote:
The comment in kvm_max_vcpus() states that it's using the recommended procedure from the kernel API documentation to get the max number of vcpus that kvm supports. It is, but by always returning the maximum number supported. The maximum number should only be used for development purposes. qemu should check KVM_CAP_NR_VCPUS for the recommended number of vcpus. This patch adds a warning if a user specifies a number of cpus between the recommended and max.
Signed-off-by: Andrew Jones <drjones@redhat.com>
CCing libvir-list. It is probably interesting for libvirt to expose or warn about the recommended VCPU limit somehow, and in this case a simple warning on stderr won't be enough.
--- kvm-all.c | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/kvm-all.c b/kvm-all.c index 716860f617455..9092e13ae60ea 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -1313,24 +1313,24 @@ static int kvm_irqchip_create(KVMState *s) return 0; }
-static int kvm_max_vcpus(KVMState *s) +/* Find number of supported CPUs using the recommended + * procedure from the kernel API documentation to cope with + * older kernels that may be missing capabilities. + */ +static int kvm_recommended_vcpus(KVMState *s) { int ret;
- /* Find number of supported CPUs using the recommended - * procedure from the kernel API documentation to cope with - * older kernels that may be missing capabilities. - */ - ret = kvm_check_extension(s, KVM_CAP_MAX_VCPUS); - if (ret) { - return ret; - } ret = kvm_check_extension(s, KVM_CAP_NR_VCPUS); - if (ret) { - return ret; - } + return (ret) ? ret : 4; +}
- return 4; +static int kvm_max_vcpus(KVMState *s) +{ + int ret; + + ret = kvm_check_extension(s, KVM_CAP_MAX_VCPUS); + return (ret) ? ret : kvm_recommended_vcpus(s); }
int kvm_init(void) @@ -1383,12 +1383,21 @@ int kvm_init(void) goto err; }
- max_vcpus = kvm_max_vcpus(s); + max_vcpus = kvm_recommended_vcpus(s); if (smp_cpus > max_vcpus) { - ret = -EINVAL; - fprintf(stderr, "Number of SMP cpus requested (%d) exceeds max cpus " - "supported by KVM (%d)\n", smp_cpus, max_vcpus); - goto err; + fprintf(stderr, + "Warning: Number of SMP cpus requested (%d) exceeds " + "recommended cpus supported by KVM (%d)\n", + smp_cpus, max_vcpus); + + max_vcpus = kvm_max_vcpus(s); + if (smp_cpus > max_vcpus) { + ret = -EINVAL; + fprintf(stderr, "Number of SMP cpus requested (%d) exceeds " + "max cpus supported by KVM (%d)\n", + smp_cpus, max_vcpus); + goto err; + }
Should at least the fatal one use the new error_report()?
}
s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
I notice that only checks in kvm_init() based on smp_cpus are touched herein. Should we add similar checks to CPU hot-add code and thus possibly move that into some per-vCPU code path? Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
----- Original Message -----
Am 22.08.2013 18:12, schrieb Eduardo Habkost:
On 22/08/2013, at 12:39, Andrew Jones <drjones@redhat.com> wrote:
The comment in kvm_max_vcpus() states that it's using the recommended procedure from the kernel API documentation to get the max number of vcpus that kvm supports. It is, but by always returning the maximum number supported. The maximum number should only be used for development purposes. qemu should check KVM_CAP_NR_VCPUS for the recommended number of vcpus. This patch adds a warning if a user specifies a number of cpus between the recommended and max.
Signed-off-by: Andrew Jones <drjones@redhat.com>
CCing libvir-list. It is probably interesting for libvirt to expose or warn about the recommended VCPU limit somehow, and in this case a simple warning on stderr won't be enough.
--- kvm-all.c | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/kvm-all.c b/kvm-all.c index 716860f617455..9092e13ae60ea 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -1313,24 +1313,24 @@ static int kvm_irqchip_create(KVMState *s) return 0; }
-static int kvm_max_vcpus(KVMState *s) +/* Find number of supported CPUs using the recommended + * procedure from the kernel API documentation to cope with + * older kernels that may be missing capabilities. + */ +static int kvm_recommended_vcpus(KVMState *s) { int ret;
- /* Find number of supported CPUs using the recommended - * procedure from the kernel API documentation to cope with - * older kernels that may be missing capabilities. - */ - ret = kvm_check_extension(s, KVM_CAP_MAX_VCPUS); - if (ret) { - return ret; - } ret = kvm_check_extension(s, KVM_CAP_NR_VCPUS); - if (ret) { - return ret; - } + return (ret) ? ret : 4; +}
- return 4; +static int kvm_max_vcpus(KVMState *s) +{ + int ret; + + ret = kvm_check_extension(s, KVM_CAP_MAX_VCPUS); + return (ret) ? ret : kvm_recommended_vcpus(s); }
int kvm_init(void) @@ -1383,12 +1383,21 @@ int kvm_init(void) goto err; }
- max_vcpus = kvm_max_vcpus(s); + max_vcpus = kvm_recommended_vcpus(s); if (smp_cpus > max_vcpus) { - ret = -EINVAL; - fprintf(stderr, "Number of SMP cpus requested (%d) exceeds max cpus " - "supported by KVM (%d)\n", smp_cpus, max_vcpus); - goto err; + fprintf(stderr, + "Warning: Number of SMP cpus requested (%d) exceeds " + "recommended cpus supported by KVM (%d)\n", + smp_cpus, max_vcpus); + + max_vcpus = kvm_max_vcpus(s); + if (smp_cpus > max_vcpus) { + ret = -EINVAL; + fprintf(stderr, "Number of SMP cpus requested (%d) exceeds " + "max cpus supported by KVM (%d)\n", + smp_cpus, max_vcpus); + goto err; + }
Should at least the fatal one use the new error_report()?
So far kvm-all.c doesn't use error_report(). I'm inclined to leave it that way for now, for the scope of this patch anyway. Maybe we should convert all of kvm-all.c at some point though?
}
s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
I notice that only checks in kvm_init() based on smp_cpus are touched herein. Should we add similar checks to CPU hot-add code and thus possibly move that into some per-vCPU code path?
That's a better question for hot-plug folk. Does smp_cpus map to the current number of cpus, or to the number of possible cpus? If it maps to the number of possible cpus, then this is the right place. If the former, then I guess it'll take more thought. I'ved added Igor (still on vacation) to this reply, but regardless I vote we worry about hot-plug limit checking in different patch. thanks, drew
Regards, Andreas
-- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Il 23/08/2013 13:33, Andrew Jones ha scritto:
Does smp_cpus map to the current number of cpus, or to the number of possible cpus? If it maps to the number of possible cpus, then this is the right place. If the former, then I guess it'll take more thought. I'ved added Igor (still on vacation) to this reply, but regardless I vote we worry about hot-plug limit checking in different patch.
smp_cpus is the initial number, max_cpus is the number of possible cpus. Paolo
----- Original Message -----
Il 23/08/2013 13:33, Andrew Jones ha scritto:
Does smp_cpus map to the current number of cpus, or to the number of possible cpus? If it maps to the number of possible cpus, then this is the right place. If the former, then I guess it'll take more thought. I'ved added Igor (still on vacation) to this reply, but regardless I vote we worry about hot-plug limit checking in different patch.
smp_cpus is the initial number, max_cpus is the number of possible cpus.
Yeah, I noticed that this issue is at least partially addressed already with my v2, which incorporates Marcelo's check against the number of hotpluggable cpus (max_cpus). drew
participants (4)
-
Andreas Färber -
Andrew Jones -
Eduardo Habkost -
Paolo Bonzini