From mboxrd@z Thu Jan  1 00:00:00 1970
From: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
Subject: Re: [PATCH v4 06/10] KVM: MMU: fast path of handling guest page fault
Date: Wed, 02 May 2012 13:28:39 +0800
Message-ID: <4FA0C607.5010002@linux.vnet.ibm.com>
References: <4F9776D2.7020506@linux.vnet.ibm.com> <4F9777A4.208@linux.vnet.ibm.com> <20120426234535.GA5057@amt.cnet> <4F9A3445.2060305@linux.vnet.ibm.com> <20120427145213.GB28796@amt.cnet> <4F9B89D9.9060307@linux.vnet.ibm.com> <20120501013459.GB10142@amt.cnet>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Cc: Avi Kivity <avi@redhat.com>, LKML <linux-kernel@vger.kernel.org>,
	KVM <kvm@vger.kernel.org>
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <linux-kernel-owner@vger.kernel.org>
In-Reply-To: <20120501013459.GB10142@amt.cnet>
Sender: linux-kernel-owner@vger.kernel.org
List-Id: kvm.vger.kernel.org

On 05/01/2012 09:34 AM, Marcelo Tosatti wrote:


> 
> It is getting better, but not yet, there are still reads of sptep
> scattered all over (as mentioned before, i think a pattern of read spte
> once, work on top of that, atomically write and then deal with results
> _everywhere_ (where mmu lock is held) is more consistent.
> 


But we only need care the path which depends on is_writable_pte(), no?

So, where call is_writable_pte() are spte_has_volatile_bits(),
spte_write_protect() and set_spte().

I have changed these functions:
In spte_has_volatile_bits():
 static bool spte_has_volatile_bits(u64 spte)
 {
+	/*
+	 * Always atomicly update spte if it can be updated
+	 * out of mmu-lock.
+	 */
+	if (spte_can_lockless_update(spte))
+		return true;
+

In spte_write_protect():

+	spte = mmu_spte_update(sptep, spte);
+
+	if (is_writable_pte(spte))
+		*flush |= true;
+
The 'spte' is from atomically read-write (xchg).

in set_spte():
 set_pte:
-	mmu_spte_update(sptep, spte);
+	entry = mmu_spte_update(sptep, spte);
 	/*
 	 * If we overwrite a writable spte with a read-only one we
 	 * should flush remote TLBs. Otherwise rmap_write_protect
The 'entry' is also the latest value.

>         /*
>          * If we overwrite a writable spte with a read-only one we
>          * should flush remote TLBs. Otherwise rmap_write_protect
>          * will find a read-only spte, even though the writable spte
>          * might be cached on a CPU's TLB.
>          */
>         if (is_writable_pte(entry) && !is_writable_pte(*sptep))
>                 kvm_flush_remote_tlbs(vcpu->kvm);
> 
> This is inconsistent with the above obviously.
> 


'entry' is not a problem since it is from atomically read-write as
mentioned above, i need change this code to:

		/*
		 * 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 (!can_unsync && is_writable_pte(entry) /* Use 'entry' instead of '*sptep'. */
			goto set_pte
   ......


         if (is_writable_pte(entry) && !is_writable_pte(spte)) /* Use 'spte' instead of '*sptep'. */
                 kvm_flush_remote_tlbs(vcpu->kvm);