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

[Andreas Färber]

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