From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [RFC PATCH 2/2] kvm, x86: use ro page and don't copy shared page Date: Wed, 09 Jun 2010 12:28:11 +0300 Message-ID: <4C0F5EAB.3030401@redhat.com> References: <4C0F4883.3010100@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm@vger.kernel.org To: Lai Jiangshan Return-path: Received: from mx1.redhat.com ([209.132.183.28]:15969 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755773Ab0FIJ2Q (ORCPT ); Wed, 9 Jun 2010 05:28:16 -0400 In-Reply-To: <4C0F4883.3010100@cn.fujitsu.com> Sender: kvm-owner@vger.kernel.org List-ID: On 06/09/2010 10:53 AM, Lai Jiangshan wrote: > When page fault, we always call get_user_pages(write=1). > > Actually, we don't need to do this when it is not write fault. > get_user_pages(write=1) will cause shared page(ksm) copied. > If this page is not modified in future, this copying and the copied page > are just wasted. Ksm may scan and merge them and may cause thrash. > > This patch is not for inclusion, because I know nothing about mmio > and this patch includes a "workaround" which ensures mmio pfns > are always writable in tdp_page_fault(). > The guest can't even boot up without this workaround. > mmio pfns are used for device assignment. These are host pfns that don't have a struct page, instead they belong to a device BAR. I don't understand why you see a failure since they aren't even present on guests without assigned devices. > @@ -2357,7 +2359,12 @@ static int tdp_page_fault(struct kvm_vcpu *vcpu, gva_t gpa, > > mmu_seq = vcpu->kvm->mmu_notifier_seq; > smp_rmb(); > - pfn = kvm_get_pfn_for_gfn(vcpu->kvm, gfn, 1); > + pfn = kvm_get_pfn_for_gfn(vcpu->kvm, gfn, error_code& PFERR_WRITE_MASK); > This is a pessimization, since now we may need two faults per page, one to page it in read-only and another to establish write access. May not be so bad for tdp, but will surely reduce performance on shadow. The way I think it should be improved, is to extend get_user_pages_fast() to also return the pte. So now, if we get a page for read, but it happens to have a writeable/dirty pte, we can still allow write access in the spte. > + if (!(error_code& PFERR_WRITE_MASK)&& kvm_is_mmio_pfn(pfn)) { > + kvm_release_pfn_clean(pfn); > + /* I don't know why we have to ensure mmio pfns are always writable. */ > + pfn = kvm_get_pfn_for_gfn(vcpu->kvm, gfn, 1); > + } > Wierd. For what gfn/pfns does this trigger? In general this is a great optimization. -- error compiling committee.c: too many arguments to function