From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@qumranet.com>
Subject: Re: [RFC][PATCH] reduce KVM stack usage
Date: Sat, 19 Jul 2008 10:32:27 +0300
Message-ID: <4881988B.207@qumranet.com>
References: <1216308735.9161.3.camel@nimitz>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
	format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: kvm-devel <kvm@vger.kernel.org>,
	Anthony Liguori <aliguori@us.ibm.com>,
	Roland Dreier <rdreier@cisco.com>
To: Dave Hansen <dave@linux.vnet.ibm.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from il.qumranet.com ([212.179.150.194]:52601 "EHLO il.qumranet.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751505AbYGSHc3 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sat, 19 Jul 2008 03:32:29 -0400
In-Reply-To: <1216308735.9161.3.camel@nimitz>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Dave Hansen wrote:
> KVM uses a lot of kernel stack on x86, especially with gcc 3.x.  It
> likes to overflow it and make my poor machine go boom.  This patch ta=
kes
> the worst stack users and makes them use kmalloc().  It also saves ~3=
0
> bytes in kvm_arch_vm_ioctl() by using a union.
>
> I haven't tested this at all yet.  Just posting to get some comments.
>
> Avi, you said that one of these was a hot path.  Which one is that?  =
=EF=BB=BFI
> kinda doubt you'll be able to see any impact anyway.
>
>  =20

The pv_mmu_ops thing.

> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 7e7c396..f4e62f2 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2154,18 +2154,24 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsi=
gned long bytes,
>  		  gpa_t addr, unsigned long *ret)
>  {
>  	int r;
> -	struct kvm_pv_mmu_op_buffer buffer;
> +	struct kvm_pv_mmu_op_buffer *buffer =3D
> +		kmalloc(GFP_KERNEL, sizeof(struct kvm_pv_mmu_op_buffer));
> =20
> -	buffer.ptr =3D buffer.buf;
> -	buffer.len =3D min_t(unsigned long, bytes, sizeof buffer.buf);
> -	buffer.processed =3D 0;
> +	if (!buffer) {
> +		*ret =3D 0;
> +		return -ENOMEM;
> +	}
> +
> +	buffer->ptr =3D buffer->buf;
> +	buffer->len =3D min_t(unsigned long, bytes, sizeof buffer->buf);
> +	buffer->processed =3D 0;
> =20
> -	r =3D kvm_read_guest(vcpu->kvm, addr, buffer.buf, buffer.len);
> +	r =3D kvm_read_guest(vcpu->kvm, addr, buffer->buf, buffer->len);
>  	if (r)
>  		goto out;
> =20
> -	while (buffer.len) {
> -		r =3D kvm_pv_mmu_op_one(vcpu, &buffer);
> +	while (buffer->len) {
> +		r =3D kvm_pv_mmu_op_one(vcpu, buffer);
>  		if (r < 0)
>  			goto out;
>  		if (r =3D=3D 0)
> @@ -2174,7 +2180,8 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsign=
ed long bytes,
> =20
>  	r =3D 1;
>  out:
> -	*ret =3D buffer.processed;
> +	*ret =3D buffer->processed;
> +	kfree(buffer);
>  =20

Please change this to store the buffer in vcpu->arch.something, and fre=
e=20
it on vcpu destruction.  If this path is called at all, it is called=20
very often, so it's fine to "waste" those 512 bytes.

Might even allocate it as a field in vcpu->arch, since it's only 512 by=
tes.

> =20
> +static int kvm_arch_vm_irqchip_ioctl(struct kvm *kvm, void *argp,
> +				     unsigned int ioctl)
> +{
> +
> +int x86_kvm_vm_ioctl_set_memory_region(struct kvm *kvm, void *argp)
> +{
>  =20

Please keep the code inlined in kvm_arch_vcpu_ioctl(), as this is a=20
-stable candidate and I want to keep the patch obvious.  A later patch=20
can break it up.

Please separate the vm ioctl, vcpu ioctl, and pv_mmu_op thing into=20
separate patches, for bisect goodness.

--=20
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.