Re: [RFC][PATCH] reduce KVM stack usage

[Avi Kivity <avi@qumranet.com>]

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 takes
> the worst stack users and makes them use kmalloc().  It also saves ~30
> 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?  I
> kinda doubt you'll be able to see any impact anyway.
>
>   

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, unsigned long bytes,
>  		  gpa_t addr, unsigned long *ret)
>  {
>  	int r;
> -	struct kvm_pv_mmu_op_buffer buffer;
> +	struct kvm_pv_mmu_op_buffer *buffer =
> +		kmalloc(GFP_KERNEL, sizeof(struct kvm_pv_mmu_op_buffer));
>  
> -	buffer.ptr = buffer.buf;
> -	buffer.len = min_t(unsigned long, bytes, sizeof buffer.buf);
> -	buffer.processed = 0;
> +	if (!buffer) {
> +		*ret = 0;
> +		return -ENOMEM;
> +	}
> +
> +	buffer->ptr = buffer->buf;
> +	buffer->len = min_t(unsigned long, bytes, sizeof buffer->buf);
> +	buffer->processed = 0;
>  
> -	r = kvm_read_guest(vcpu->kvm, addr, buffer.buf, buffer.len);
> +	r = kvm_read_guest(vcpu->kvm, addr, buffer->buf, buffer->len);
>  	if (r)
>  		goto out;
>  
> -	while (buffer.len) {
> -		r = kvm_pv_mmu_op_one(vcpu, &buffer);
> +	while (buffer->len) {
> +		r = kvm_pv_mmu_op_one(vcpu, buffer);
>  		if (r < 0)
>  			goto out;
>  		if (r == 0)
> @@ -2174,7 +2180,8 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
>  
>  	r = 1;
>  out:
> -	*ret = buffer.processed;
> +	*ret = buffer->processed;
> +	kfree(buffer);
>   

Please change this to store the buffer in vcpu->arch.something, and free 
it on vcpu destruction.  If this path is called at all, it is called 
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 bytes.

>  
> +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)
> +{
>   

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

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

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