From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: KVM: MMU: optimize set_spte for page sync
Date: Mon, 24 Nov 2008 13:04:23 +0100
Message-ID: <20081124120423.GB4379@dmt.cnet>
References: <20081121184927.GA20607@dmt.cnet> <4929322D.7050503@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm-devel <kvm@vger.kernel.org>
To: Avi Kivity <avi@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:37023 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751170AbYKXPG2 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 24 Nov 2008 10:06:28 -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 mAOF6Suq009821
	for <kvm@vger.kernel.org>; Mon, 24 Nov 2008 10:06:28 -0500
Content-Disposition: inline
In-Reply-To: <4929322D.7050503@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.