From mboxrd@z Thu Jan  1 00:00:00 1970
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH] kvm: warn if num cpus is greater than num recommended
Date: Thu, 22 Aug 2013 18:38:34 +0200
Message-ID: <52163E8A.1090002@redhat.com>
References: <1377185968-13129-1-git-send-email-drjones@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org, gleb@redhat.com,
	mtosatti@redhat.com, ehabkost@redhat.com
To: Andrew Jones <drjones@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:36881 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1754478Ab3HWKDi (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 23 Aug 2013 06:03:38 -0400
In-Reply-To: <1377185968-13129-1-git-send-email-drjones@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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