| Message ID | 1377185968-13129-1-git-send-email-drjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 >
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
Il 22/08/2013 17:39, Andrew Jones ha scritto: > 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> > --- > 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; > + } You print both error messages when smp_cpus is greater than the max cpus supported; is it intentional? Apart from this, the concept looks good. However, please over qemu-kvm.git's uq/master branch, where we already have Marcelo's patch to check max_cpus too against kvm_max_vcpus(s). Thanks, Paolo
----- 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 >
----- Original Message ----- > Il 22/08/2013 17:39, Andrew Jones ha scritto: > > 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> > > --- > > 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; > > + } > > You print both error messages when smp_cpus is greater than the max cpus > supported; is it intentional? > Yup. This way we can inform the user not only that they're greater than hard-max, but also what the soft-max is. This allows the user to choose how much lower to adjust when they correct for the hard-max, possibly going down low enough to avoid blowing the soft-max as well. > Apart from this, the concept looks good. However, please over > qemu-kvm.git's uq/master branch, where we already have Marcelo's patch > to check max_cpus too against kvm_max_vcpus(s). > OK, will respin on uq/master. thanks, drew
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
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);
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> --- kvm-all.c | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-)