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@xxxxxxxxxx>
---
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? :)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
