Re: [PATCH v3 3/8] kvm: x86: mmu: Fast Page Fault path retries

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

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 <junaids@google.com>
> ---
>  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?  :)