From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47939) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VCpcM-0002u1-Ho for qemu-devel@nongnu.org; Fri, 23 Aug 2013 07:33:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VCpcG-0004ND-EA for qemu-devel@nongnu.org; Fri, 23 Aug 2013 07:33:26 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:45053) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VCpcG-0004Lv-7Y for qemu-devel@nongnu.org; Fri, 23 Aug 2013 07:33:20 -0400 Date: Fri, 23 Aug 2013 07:33:18 -0400 (EDT) From: Andrew Jones Message-ID: <795650865.3095841.1377257598867.JavaMail.root@redhat.com> In-Reply-To: <52163A7F.7020402@suse.de> References: <1377185968-13129-1-git-send-email-drjones@redhat.com> <52163A7F.7020402@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] kvm: warn if num cpus is greater than num recommended List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?utf-8?Q?F=C3=A4rber?= Cc: Eduardo Habkost , gleb@redhat.com, libvir-list@redhat.com, mtosatti@redhat.com, qemu-devel developers , kvm@vger.kernel.org, pbonzini@redhat.com ----- Original Message ----- > Am 22.08.2013 18:12, schrieb Eduardo Habkost: > >=20 > > On 22/08/2013, at 12:39, Andrew Jones wrote: > >=20 > >> 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 > >=20 > > 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. > >=20 > >> --- > >> 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 =3D kvm_check_extension(s, KVM_CAP_MAX_VCPUS); > >> - if (ret) { > >> - return ret; > >> - } > >> ret =3D 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 =3D 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 =3D kvm_max_vcpus(s); > >> + max_vcpus =3D kvm_recommended_vcpus(s); > >> if (smp_cpus > max_vcpus) { > >> - ret =3D -EINVAL; > >> - fprintf(stderr, "Number of SMP cpus requested (%d) exceeds ma= x > >> 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 =3D kvm_max_vcpus(s); > >> + if (smp_cpus > max_vcpus) { > >> + ret =3D -EINVAL; > >> + fprintf(stderr, "Number of SMP cpus requested (%d) exceed= s " > >> + "max cpus supported by KVM (%d)\n", > >> + smp_cpus, max_vcpus); > >> + goto err; > >> + } >=20 > 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? >=20 > >> } > >> > >> s->vmfd =3D kvm_ioctl(s, KVM_CREATE_VM, 0); >=20 > 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? >=20 That's a better question for hot-plug folk. Does smp_cpus map to the curren= t 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 >=20 > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3= =BCrnberg >=20