From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [patch 02/13] KVM: MMU: switch to get_user_pages_fast
Date: Sun, 7 Sep 2008 17:44:15 -0300
Message-ID: <20080907204415.GA14479@dmt.cnet>
References: <20080906184822.560099087@localhost.localdomain> <20080906192430.598327536@localhost.localdomain> <48C394B5.20706@qumranet.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org, Andrea Arcangeli <andrea@qumranet.com>
To: Avi Kivity <avi@qumranet.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([66.187.233.31]:38301 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1755560AbYIGUpV (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 7 Sep 2008 16:45:21 -0400
Content-Disposition: inline
In-Reply-To: <48C394B5.20706@qumranet.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Sun, Sep 07, 2008 at 11:45:41AM +0300, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> Avoid mmap_sem / pt lock acquision if the pagetables are present. The
>> improvement for hugepage backed guests is more significant, since pte
>> walk + page grab for such mappings is serialized by mm->page_table_lock.
>>
>> CC: Andrea Arcangeli <andrea@qumranet.com>
>>
>>   
>
> I'd like to apply this.  Andrea, can you verify that mmu notifiers  
> interaction is okay?
>
>>   static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault 
>> *vmf)
>>  {
>> +	struct page *page[1];
>> +	unsigned long addr;
>> +	int npages;
>> +	gfn_t gfn = vmf->pgoff;
>>  	struct kvm *kvm = vma->vm_file->private_data;
>> -	struct page *page;
>>  -	if (!kvm_is_visible_gfn(kvm, vmf->pgoff))
>> +	addr = gfn_to_hva(kvm, gfn);
>> +	if (kvm_is_error_hva(addr))
>>  		return VM_FAULT_SIGBUS;
>> -	page = gfn_to_page(kvm, vmf->pgoff);
>> -	if (is_error_page(page)) {
>> -		kvm_release_page_clean(page);
>> +
>> +	npages = get_user_pages(current, current->mm, addr, 1, 1, 0, page,
>> +				NULL);
>> +	if (unlikely(npages != 1))
>>  		return VM_FAULT_SIGBUS;
>> -	}
>> -	vmf->page = page;
>> +
>> +	vmf->page = page[0];
>>  	return 0;
>>  }
>>    
>
> Why this change?

Because get_user_pages_fast grabs mmap_sem if necessary, but ->vm_fault
already holds it.

Deadlock:

CPU0                        CPU1    

down_read(mmap_sem)
kvm_vm_fault
                            down_write(mmap_sem)
gfn_to_page
get_user_pages_fast
down_read(mmap_sem)