[libvirt] [Qemu-devel] [PATCH] kvm: warn if num cpus is greater than num recommended

[Andrew Jones]

----- Original Message -----
> Am 22.08.2013 18:12, schrieb Eduardo Habkost:
> > 
> > On 22/08/2013, at 12:39, Andrew Jones <drjones at 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 at 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
>