From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [PATCH v3 3/8] kvm: x86: mmu: Fast Page Fault path retries Date: Thu, 15 Dec 2016 15:20:19 +0800 Message-ID: <1948ea57-76c4-dfe1-bdd2-09dbf265747f@linux.intel.com> References: <1481071577-40250-1-git-send-email-junaids@google.com> <1481071577-40250-4-git-send-email-junaids@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: andreslc@google.com, pfeiner@google.com, pbonzini@redhat.com To: Junaid Shahid , kvm@vger.kernel.org Return-path: Received: from mga07.intel.com ([134.134.136.100]:8346 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752675AbcLOH25 (ORCPT ); Thu, 15 Dec 2016 02:28:57 -0500 In-Reply-To: <1481071577-40250-4-git-send-email-junaids@google.com> Sender: kvm-owner@vger.kernel.org List-ID: On 12/07/2016 08:46 AM, Junaid Shahid wrote: > This change adds retries into the Fast Page Fault path. Without the > retries, the code still works, but if a retry does end up being needed, > then it will result in a second page fault for the same memory access, > which will cause much more overhead compared to just retrying within the > original fault. > > This would be especially useful with the upcoming fast access tracking > change, as that would make it more likely for retries to be needed > (e.g. due to read and write faults happening on different CPUs at > the same time). > > Signed-off-by: Junaid Shahid > --- > arch/x86/kvm/mmu.c | 124 +++++++++++++++++++++++++++++++---------------------- > 1 file changed, 73 insertions(+), 51 deletions(-) > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index 4d33275..bcf1b95 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -2881,6 +2881,10 @@ static bool page_fault_can_be_fast(u32 error_code) > return true; > } > > +/* > + * Returns true if the SPTE was fixed successfully. Otherwise, > + * someone else modified the SPTE from its original value. > + */ > static bool > fast_pf_fix_direct_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, > u64 *sptep, u64 spte) > @@ -2907,8 +2911,10 @@ fast_pf_fix_direct_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, > * > * Compare with set_spte where instead shadow_dirty_mask is set. > */ > - if (cmpxchg64(sptep, spte, spte | PT_WRITABLE_MASK) == spte) > - kvm_vcpu_mark_page_dirty(vcpu, gfn); > + if (cmpxchg64(sptep, spte, spte | PT_WRITABLE_MASK) != spte) > + return false; > + > + kvm_vcpu_mark_page_dirty(vcpu, gfn); > > return true; > } > @@ -2923,8 +2929,9 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level, > { > struct kvm_shadow_walk_iterator iterator; > struct kvm_mmu_page *sp; > - bool ret = false; > + bool fault_handled = false; > u64 spte = 0ull; > + uint retry_count = 0; > > if (!VALID_PAGE(vcpu->arch.mmu.root_hpa)) > return false; > @@ -2937,62 +2944,77 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level, > if (!is_shadow_present_pte(spte) || iterator.level < level) > break; > > - /* > - * If the mapping has been changed, let the vcpu fault on the > - * same address again. > - */ > - if (!is_shadow_present_pte(spte)) { > - ret = true; > - goto exit; > - } > + do { > + /* > + * If the mapping has been changed, let the vcpu fault on the > + * same address again. > + */ > + if (!is_shadow_present_pte(spte)) { > + fault_handled = true; > + break; > + } Why not include lockless_walk into the loop, retry 4 times for a invalid sp is expensive. I am curious that did you see this retry is really helpful? :)