From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: KVM: MMU: optimize set_spte for page sync Date: Mon, 24 Nov 2008 14:23:06 +0100 Message-ID: <20081124132306.GA13532@dmt.cnet> References: <20081121184927.GA20607@dmt.cnet> <4929322D.7050503@redhat.com> <20081124120423.GB4379@dmt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm-devel To: Avi Kivity Return-path: Received: from mx2.redhat.com ([66.187.237.31]:58952 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752642AbYKXQZR (ORCPT ); Mon, 24 Nov 2008 11:25:17 -0500 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id mAOGPHt3026484 for ; Mon, 24 Nov 2008 11:25:17 -0500 Content-Disposition: inline In-Reply-To: <20081124120423.GB4379@dmt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Nov 24, 2008 at 01:04:23PM +0100, Marcelo Tosatti wrote: > On Sun, Nov 23, 2008 at 12:36:29PM +0200, Avi Kivity wrote: > > Marcelo Tosatti wrote: > > > >> The cost of hash table and memslot lookups are quite significant if the > >> workload is pagetable write intensive resulting in increased mmu_lock > >> contention. > >> > >> @@ -1593,7 +1593,16 @@ static int set_spte(struct kvm_vcpu *vcp > >> spte |= PT_WRITABLE_MASK; > >> - if (mmu_need_write_protect(vcpu, gfn, can_unsync)) { > >> + /* > >> + * Optimization: for pte sync, if spte was writable the hash > >> + * lookup is unnecessary (and expensive). Write protection > >> + * is responsibility of mmu_get_page / kvm_sync_page. > >> + * Same reasoning can be applied to dirty page accounting. > >> + */ > >> + if (sync_page && is_writeble_pte(*shadow_pte)) > >> + goto set_pte; > >> > > > > What if *shadow_pte points at a different page? Is that possible? > > To a different gfn? Then sync_page will have nuked the spte: > > if (gpte_to_gfn(gpte) != gfn || !is_present_pte(gpte) || > !(gpte & PT_ACCESSED_MASK)) { > u64 nonpresent; > .. > set_shadow_pte(&sp->spt[i], nonpresent); > } > > Otherwise: > > /* > * Using the cached information from sp->gfns is safe because: > * - The spte has a reference to the struct page, so the pfn for a given > * gfn can't change unless all sptes pointing to it are nuked first. *shadow_pte can point to a different page if the guest updates pagetable, there is a fault before resync, the fault updates the spte with new gfn (and pfn) via mmu_set_spte. In which case the gfn cache is updated since: } else if (pfn != spte_to_pfn(*shadow_pte)) { printk("hfn old %lx new %lx\n", spte_to_pfn(*shadow_pte), pfn); rmap_remove(vcpu->kvm, shadow_pte);