KVM: MMU: fast page fault

If the the present bit of page fault error code is set, it indicates the shadow page is populated on all levels, it means what we do is only modify the access bit which can be done out of mmu-lock

Currently, in order to simplify the code, we only fix the page fault caused by write-protect on the fast path

In order to better review, we hold mmu-lock to update spte in this patch, the concurrent update will be introduced in the later patch

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c         |  113 ++++++++++++++++++++++++++++++++++++++++++--
 arch/x86/kvm/paging_tmpl.h |    3 +
 2 files changed, 112 insertions(+), 4 deletions(-)

arch/x86/kvm/mmu.c +24 -0

index e7d8ffe..96a9d5b 100644
@@ -2683,18 +2683,117 @@ exit:
 	return ret;
 }
 
+static bool page_fault_can_be_fast(struct kvm_vcpu *vcpu, gfn_t gfn,
+				   u32 error_code)
+{
+	/*
+	 * #PF can be fast only if the shadow page table is present and it
+	 * is caused by write-protect, that means we just need change the
+	 * W bit of the spte which can be done out of mmu-lock.
+	 */
+	if (!(error_code & PFERR_PRESENT_MASK) ||
+	      !(error_code & PFERR_WRITE_MASK))
+		return false;
+
+	return true;
+}
+
+static bool
+fast_pf_fix_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+		  u64 *sptep, u64 spte)
+{
+	gfn_t gfn;
+
+	spin_lock(&vcpu->kvm->mmu_lock);
+
+	/* The spte has been changed. */

arch/x86/kvm/mmu.c +75 -4

+
+	return true;
+}
+
+/*
+ * Return value:
+ * - true: let the vcpu to access on the same address again.
+ * - false: let the real page fault path to fix it.
+ */
+static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, gfn_t gfn,
+			    int level, u32 error_code)
+{
+	struct kvm_shadow_walk_iterator iterator;
+	struct kvm_mmu_page *sp;
+	bool ret = false;
+	u64 spte = 0ull;
+
+	if (!page_fault_can_be_fast(vcpu, gfn, error_code))
+		return false;
+
+	walk_shadow_page_lockless_begin(vcpu);
+	for_each_shadow_entry_lockless(vcpu, gva, iterator, spte)
+		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_rmap_spte(spte)) {
+		ret = true;
+		goto exit;
+	}
+
+	if (!is_last_spte(spte, level))
+		goto exit;
+
+	/*
+	 * Check if it is a spurious fault caused by TLB lazily flushed.
+	 *
+	 * Need not check the access of upper level table entries since
+	 * they are always ACC_ALL.
+	 */
+	 if (is_writable_pte(spte)) {
+		ret = true;
+		goto exit;
+	}
+
+	/*
+	 * Currently, to simplify the code, only the spte write-protected
+	 * by dirty-log can be fast fixed.
+	 */
+	if (!spte_wp_by_dirty_log(spte))
+		goto exit;
+
+	sp = page_header(__pa(iterator.sptep));
+
+	ret = fast_pf_fix_spte(vcpu, sp, iterator.sptep, spte);
+
+exit:
+	walk_shadow_page_lockless_end(vcpu);
+
+	return ret;
+}
+
 static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
 			 gva_t gva, pfn_t *pfn, bool write, bool *writable);
 
-static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, int write, gfn_t gfn,
-			 bool prefault)
+static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, u32 error_code,
+			 gfn_t gfn, bool prefault)
 {
 	int r;
 	int level;
 	int force_pt_level;
 	pfn_t pfn;
 	unsigned long mmu_seq;
-	bool map_writable;
+	bool map_writable, write = error_code & PFERR_WRITE_MASK;
 
 	force_pt_level = mapping_level_dirty_bitmap(vcpu, gfn);
 	if (likely(!force_pt_level)) {

@@ -2711,6 +2810,9 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, int write, gfn_t gfn,
 	} else
 		level = PT_PAGE_TABLE_LEVEL;
 
+	if (fast_page_fault(vcpu, v, gfn, level, error_code))
+		return 0;
+
 	mmu_seq = vcpu->kvm->mmu_notifier_seq;
 	smp_rmb();
 

@@ -3099,7 +3201,7 @@ static int nonpaging_page_fault(struct kvm_vcpu *vcpu, gva_t gva,
 	gfn = gva >> PAGE_SHIFT;
 
 	return nonpaging_map(vcpu, gva & PAGE_MASK,
-			     error_code & PFERR_WRITE_MASK, gfn, prefault);
+			     error_code, gfn, prefault);
 }
 
 static int kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu, gva_t gva, gfn_t gfn)

@@ -3179,6 +3281,9 @@ static int tdp_page_fault(struct kvm_vcpu *vcpu, gva_t gpa, u32 error_code,
 	} else
 		level = PT_PAGE_TABLE_LEVEL;
 
+	if (fast_page_fault(vcpu, gpa, gfn, level, error_code))
+		return 0;
+
 	mmu_seq = vcpu->kvm->mmu_notifier_seq;
 	smp_rmb();
 

arch/x86/kvm/paging_tmpl.h +3 -0

index df5a703..80493fb 100644
@@ -617,6 +617,9 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
 		walker.gfn = walker.gfn & ~(KVM_PAGES_PER_HPAGE(level) - 1);
 	}
 
+	if (fast_page_fault(vcpu, addr, walker.gfn, level, error_code))
+		return 0;
+
 	mmu_seq = vcpu->kvm->mmu_notifier_seq;
 	smp_rmb();
 

Signature · 6 lines

1.7.7.6

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 13 lines

arch/x86/kvm/mmu.c +13 -0

+static bool
+fast_pf_fix_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+		  u64 *sptep, u64 spte)
+{
+	gfn_t gfn;
+
+	spin_lock(&vcpu->kvm->mmu_lock);
+
+	/* The spte has been changed. */
+	if (*sptep != spte)
+		goto exit;
+
+	gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);

arch/x86/kvm/mmu.c exit: · line 2719

Quoted 6 lines

arch/x86/kvm/mmu.c +6 -0

+
+	*sptep = spte | PT_WRITABLE_MASK;
+	mark_page_dirty(vcpu->kvm, gfn);
+
+exit:
+	spin_unlock(&vcpu->kvm->mmu_lock);

There was a misunderstanding.

The suggestion is to change codepaths that
today assume that a side effect of holding mmu_lock is that sptes are not updated or read before attempting to introduce any lockless spte updates.

example)

        u64 mask, old_spte = *sptep;

        if (!spte_has_volatile_bits(old_spte) || (new_spte & mask) == mask)
                __update_clear_spte_fast(sptep, new_spte);
        else
                old_spte = __update_clear_spte_slow(sptep, new_spte);

The local old_spte copy might be stale by the
time "spte_has_volatile_bits(old_spte)" reads the writable bit.

example)

VCPU0 VCPU1
set_spte
mmu_spte_update decides fast write
mov newspte to sptep
(non-locked write instruction)
newspte in store buffer

lockless spte read path reads stale value

spin_unlock(mmu_lock)

Depending on the what stale value CPU1 read and what decision it took this can be a problem, say the new bits (and we do not want to verify every single case). The spte write from inside mmu_lock should always be atomic now?

So these details must be exposed to the reader, they are hidden now (note mmu_spte_update is already a maze, its getting too deep).

Quoted 13 lines · Marcelo Tosatti

Quoted 13 lines

arch/x86/kvm/mmu.c +13 -0

+static bool
+fast_pf_fix_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+		  u64 *sptep, u64 spte)
+{
+	gfn_t gfn;
+
+	spin_lock(&vcpu->kvm->mmu_lock);
+
+	/* The spte has been changed. */
+	if (*sptep != spte)
+		goto exit;
+
+	gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);

Quoted 8 lines

arch/x86/kvm/mmu.c exit: · line 2719

Quoted 6 lines

arch/x86/kvm/mmu.c +6 -0

+
+	*sptep = spte | PT_WRITABLE_MASK;
+	mark_page_dirty(vcpu->kvm, gfn);
+
+exit:
+	spin_unlock(&vcpu->kvm->mmu_lock);

There was a misunderstanding.

Sorry, Marcelo! It is still not clear for me now. :(

Quoted 36 lines

The suggestion is to change codepaths that
today assume that a side effect of holding mmu_lock is that sptes are not updated or read before attempting to introduce any lockless spte updates.

example)

        u64 mask, old_spte = *sptep;

        if (!spte_has_volatile_bits(old_spte) || (new_spte & mask) == mask)
                __update_clear_spte_fast(sptep, new_spte);
        else
                old_spte = __update_clear_spte_slow(sptep, new_spte);

The local old_spte copy might be stale by the
time "spte_has_volatile_bits(old_spte)" reads the writable bit.

example)

VCPU0 VCPU1
set_spte
mmu_spte_update decides fast write
mov newspte to sptep
(non-locked write instruction)
newspte in store buffer

lockless spte read path reads stale value

spin_unlock(mmu_lock)

Depending on the what stale value CPU1 read and what decision it took this can be a problem, say the new bits (and we do not want to verify every single case). The spte write from inside mmu_lock should always be atomic now?

Quoted 3 lines

So these details must be exposed to the reader, they are hidden now (note mmu_spte_update is already a maze, its getting too deep).

Actually, in this patch, all the spte update is under mmu-lock, and we lockless-ly read spte , but the spte will be verified again after holding mmu-lock.

arch/x86/kvm/mmu.c +13 -0

+	spin_lock(&vcpu->kvm->mmu_lock);
+
+	/* The spte has been changed. */
+	if (*sptep != spte)
+		goto exit;
+
+	gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+
+	*sptep = spte | PT_WRITABLE_MASK;
+	mark_page_dirty(vcpu->kvm, gfn);
+
+exit:
+	spin_unlock(&vcpu->kvm->mmu_lock);

Is not the same as both read/update spte under mmu-lock?

Hmm, this is what you want?

Quoted 3 lines

Actually, in this patch, all the spte update is under mmu-lock, and we lockless-ly read spte , but the spte will be verified again after holding mmu-lock.

Yes but the objective you are aiming for is to read and write sptes without mmu_lock. That is, i am not talking about this patch. Please read carefully the two examples i gave (separated by "example)").

Quoted 14 lines

(quoted diff) +13 -0

+	spin_lock(&vcpu->kvm->mmu_lock);
+
+	/* The spte has been changed. */
+	if (*sptep != spte)
+		goto exit;
+
+	gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+
+	*sptep = spte | PT_WRITABLE_MASK;
+	mark_page_dirty(vcpu->kvm, gfn);
+
+exit:
+	spin_unlock(&vcpu->kvm->mmu_lock);

Quoted 3 lines

Is not the same as both read/update spte under mmu-lock?

Hmm, this is what you want?

The rules for code under mmu_lock should be:

1) Spte updates under mmu lock must always be atomic and with locked instructions.

2) Spte values must be read once, and appropriate action must be taken when writing them back in case their value has changed (remote TLB flush might be required).

The maintenance of:

- gpte writable bit
- protected by dirty log

Bits is tricky. We should think of a way to simplify things and get rid of them (or at least one of them), if possible.

Quoted 8 lines · Marcelo Tosatti

Quoted 3 lines

Actually, in this patch, all the spte update is under mmu-lock, and we lockless-ly read spte , but the spte will be verified again after holding mmu-lock.

Yes but the objective you are aiming for is to read and write sptes without mmu_lock. That is, i am not talking about this patch. Please read carefully the two examples i gave (separated by "example)").

Thanks for your patience, Marcelo!

Quoted 14 lines

Quoted 14 lines

arch/x86/kvm/mmu.c +13 -0

+	spin_lock(&vcpu->kvm->mmu_lock);
+
+	/* The spte has been changed. */
+	if (*sptep != spte)
+		goto exit;
+
+	gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+
+	*sptep = spte | PT_WRITABLE_MASK;
+	mark_page_dirty(vcpu->kvm, gfn);
+
+exit:
+	spin_unlock(&vcpu->kvm->mmu_lock);

Quoted 8 lines

Quoted 3 lines

Is not the same as both read/update spte under mmu-lock?

Hmm, this is what you want?

The rules for code under mmu_lock should be:

1) Spte updates under mmu lock must always be atomic and with locked instructions.

How about treating the spte is 'volatile' if the spte can be updated out of mmu-lock? In this case, the update is always atomic.

The piece of code:

arch/x86/kvm/mmu.c +19 -0

+static bool spte_can_be_writable(u64 spte)
+{
+	u64 mask = SPTE_HOST_WRITEABLE | SPTE_MMU_WRITEABLE;
+
+	return (spte & mask) == mask;
+}
+
+static bool spte_can_lockless_update(u64 spte)
+{
+	return  !is_writable_pte(spte) && spte_can_be_writable(spte);
+}
+
 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;
+

Quoted 5 lines

2) Spte values must be read once, and appropriate action must be taken when writing them back in case their value has changed (remote TLB flush might be required).

Okay, may be i get your idea now. :)

I will fix mmu_spte_update, let it to return the latest old value which will be checked in the caller before it is updated.

Quoted 5 lines

The maintenance of:

- gpte writable bit
- protected by dirty log

Quoted 3 lines

Bits is tricky. We should think of a way to simplify things and get rid of them (or at least one of them), if possible.

Maybe SPTE_MMU_WRITEABLE is sufficient, the second bit will be dropped.

Marcelo, do you satisfied with this patch?

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.

/*
* 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.

Quoted 1 line · Marcelo Tosatti

Quoted 5 lines

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)
{

arch/x86/kvm/mmu.c +7 -0

+	/*
+	 * 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():

arch/x86/kvm/mmu.c +5 -0

+	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:

arch/x86/kvm/mmu.c +1 -1

-	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.

Quoted 11 lines

/*
* 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);

Quoted 14 lines

'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
......

Quoted 2 lines

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

What is of more importance than the ability to verify that this or that particular case are ok at the moment is to write code in such a way that its easy to verify that it is correct.

Thus the suggestion above:

"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."

Quoted 21 lines · Marcelo Tosatti

Quoted 14 lines

'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
......

Quoted 2 lines

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

What is of more importance than the ability to verify that this or that particular case are ok at the moment is to write code in such a way that its easy to verify that it is correct.

Quoted 6 lines

Thus the suggestion above:

"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."

Marcelo, thanks for your time to patiently review/reply my mail.

I am confused with ' _everywhere_ ', it means all of the path read/update spte? why not only verify the path which depends on is_writable_pte()?

For the reason of "its easy to verify that it is correct"? But these paths are safe since it is not care PT_WRITABLE_MASK at all. What these paths care is the Dirty-bit and Accessed-bit are not lost, that is why we always treat the spte as "volatile" if it is can be updated out of mmu-lock.

For the further development? We can add the delta comment for is_writable_pte() to warn the developer use it more carefully.

It is also very hard to verify spte everywhere. :(

Actually, the current code to care PT_WRITABLE_MASK is just for
tlb flush, may be we can fold it into mmu_spte_update.
[
There are tree ways to modify spte, present -> nonpresent, nonpresent -> present, present -> present.

But we only need care present -> present for lockless.
]

/*
* return true means we need flush tlbs caused by changing spte from writeable
* to read-only.
*/
bool mmu_update_spte(u64 *sptep, u64 spte)

{
	u64 last_spte, old_spte = *sptep;
	bool flush = false;

	last_spte = xchg(sptep, spte);

	if ((is_writable_pte(last_spte) ||
	      spte_has_updated_lockless(old_spte, last_spte)) &&
	         !is_writable_pte(spte) )
		flush = true;

	.... track Drity/Accessed bit ...


	return flush		
}

Furthermore, the style of "if (spte-has-changed) goto beginning" is feasible in set_spte since this path is a fast path. (i can speed up mmu_need_write_protect)

It would be better if all these complications introduced by lockless updates can be avoided, say using A/D bits as Avi suggested.

Quoted 3 lines

I am confused with ' _everywhere_ ', it means all of the path read/update spte? why not only verify the path which depends on is_writable_pte()?

I meant any path that updates from present->present.

Quoted 17 lines

For the reason of "its easy to verify that it is correct"? But these paths are safe since it is not care PT_WRITABLE_MASK at all. What these paths care is the Dirty-bit and Accessed-bit are not lost, that is why we always treat the spte as "volatile" if it is can be updated out of mmu-lock.

For the further development? We can add the delta comment for is_writable_pte() to warn the developer use it more carefully.

It is also very hard to verify spte everywhere. :(

Actually, the current code to care PT_WRITABLE_MASK is just for
tlb flush, may be we can fold it into mmu_spte_update.
[
There are tree ways to modify spte, present -> nonpresent, nonpresent -> present, present -> present.

Quoted 2 lines

But we only need care present -> present for lockless.
]

Also need to take memory ordering into account, which was not an issue before. So it is not only TLB flush.

Quoted 23 lines

/*
* return true means we need flush tlbs caused by changing spte from writeable
* to read-only.
*/
bool mmu_update_spte(u64 *sptep, u64 spte)

{
	u64 last_spte, old_spte = *sptep;
	bool flush = false;

	last_spte = xchg(sptep, spte);

	if ((is_writable_pte(last_spte) ||
	      spte_has_updated_lockless(old_spte, last_spte)) &&
	         !is_writable_pte(spte) )
		flush = true;

	.... track Drity/Accessed bit ...


	return flush		
}

Quoted 2 lines

Furthermore, the style of "if (spte-has-changed) goto beginning" is feasible in set_spte since this path is a fast path. (i can speed up mmu_need_write_protect)

What you mean exactly?

It would be better if all these complications introduced by lockless updates can be avoided, say using A/D bits as Avi suggested.

Yes but the objective you are aiming for is to read and write sptes without mmu_lock. That is, i am not talking about this patch. Please read carefully the two examples i gave (separated by "example)").

The rules for code under mmu_lock should be:

1) Spte updates under mmu lock must always be atomic and with locked instructions.
2) Spte values must be read once, and appropriate action must be taken when writing them back in case their value has changed (remote TLB flush might be required).

Quoted 3 lines · On Fri, 27 Apr 2012 11:52:13 -0300 Marcelo Tosatti <[email protected]> wrote:

Yes but the objective you are aiming for is to read and write sptes without mmu_lock. That is, i am not talking about this patch. Please read carefully the two examples i gave (separated by "example)").

The real objective is not still clear.

The ~10% improvement reported before was on macro benchmarks during live migration. At least, that optimization was the initial objective.

But at some point, the objective suddenly changed to "lock-less" without understanding what introduced the original improvement.

Was the problem really mmu_lock contention?

If the path being introduced by this patch is really fast, isn't it possible to achieve the same improvement still using mmu_lock?

Note: During live migration, the fact that the guest gets faulted is itself a limitation. We could easily see noticeable slowdown of a program even if it runs only between two GET_DIRTY_LOGs.

Quoted 7 lines

The rules for code under mmu_lock should be:

1) Spte updates under mmu lock must always be atomic and with locked instructions.
2) Spte values must be read once, and appropriate action must be taken when writing them back in case their value has changed (remote TLB flush might be required).

Although I am not certain about what will be really needed in the final form, if this kind of maybe-needed-overhead is going to be added little by little, I worry about possible regression.

Thanks,
Takuya

Quoted 3 lines · On Fri, 27 Apr 2012 11:52:13 -0300 Marcelo Tosatti <[email protected]> wrote:

Yes but the objective you are aiming for is to read and write sptes without mmu_lock. That is, i am not talking about this patch. Please read carefully the two examples i gave (separated by "example)").

The real objective is not still clear.

The ~10% improvement reported before was on macro benchmarks during live migration. At least, that optimization was the initial objective.

But at some point, the objective suddenly changed to "lock-less" without understanding what introduced the original improvement.

Was the problem really mmu_lock contention?

If the path being introduced by this patch is really fast, isn't it possible to achieve the same improvement still using mmu_lock?

Note: During live migration, the fact that the guest gets faulted is itself a limitation. We could easily see noticeable slowdown of a program even if it runs only between two GET_DIRTY_LOGs.

Quoted 8 lines

The rules for code under mmu_lock should be:

1) Spte updates under mmu lock must always be atomic and with locked instructions.
2) Spte values must be read once, and appropriate action must be taken when writing them back in case their value has changed (remote TLB flush might be required).

Although I am not certain about what will be really needed in the final form, if this kind of maybe-needed-overhead is going to be added little by little, I worry about possible regression.

Quoted 15 lines · Takuya Yoshikawa

Quoted 3 lines · On Fri, 27 Apr 2012 11:52:13 -0300 Marcelo Tosatti <[email protected]> wrote:

Yes but the objective you are aiming for is to read and write sptes without mmu_lock. That is, i am not talking about this patch. Please read carefully the two examples i gave (separated by "example)").

The real objective is not still clear.

The ~10% improvement reported before was on macro benchmarks during live migration. At least, that optimization was the initial objective.

But at some point, the objective suddenly changed to "lock-less" without understanding what introduced the original improvement.

Quoted 2 lines

Was the problem really mmu_lock contention?

Takuya, i am so tired to argue the advantage of lockless write-protect and lockless O(1) dirty-log again and again.

Quoted 8 lines

If the path being introduced by this patch is really fast, isn't it possible to achieve the same improvement still using mmu_lock?

Note: During live migration, the fact that the guest gets faulted is itself a limitation. We could easily see noticeable slowdown of a program even if it runs only between two GET_DIRTY_LOGs.

Obviously no.

It depends on what the guest is doing, from my autotest test, it very easily to see that, the huge improvement is on bench-migration not pure-migration.

Quoted 9 lines

Quoted 8 lines

The rules for code under mmu_lock should be:

1) Spte updates under mmu lock must always be atomic and with locked instructions.
2) Spte values must be read once, and appropriate action must be taken when writing them back in case their value has changed (remote TLB flush might be required).

Quoted 3 lines

Although I am not certain about what will be really needed in the final form, if this kind of maybe-needed-overhead is going to be added little by little, I worry about possible regression.

Well, will you suggest Linus to reject all patches and stop all discussion for the "possible regression" reason?

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 21 lines · Xiao Guangrong

Quoted 17 lines · Takuya Yoshikawa

Quoted 3 lines · On Fri, 27 Apr 2012 11:52:13 -0300 Marcelo Tosatti <[email protected]> wrote:

Yes but the objective you are aiming for is to read and write sptes without mmu_lock. That is, i am not talking about this patch. Please read carefully the two examples i gave (separated by "example)").

The real objective is not still clear.

The ~10% improvement reported before was on macro benchmarks during live migration. At least, that optimization was the initial objective.

But at some point, the objective suddenly changed to "lock-less" without understanding what introduced the original improvement.

Was the problem really mmu_lock contention?

Quoted 2 lines

Takuya, i am so tired to argue the advantage of lockless write-protect and lockless O(1) dirty-log again and again.

His point is valid: there is a lack of understanding on the details of the improvement.

From the test result, lockless gains little improvement for unix-migration,

Did you see the pahole output on struct kvm? Apparently mmu_lock is sharing a cacheline with read-intensive memslots pointer. It would be interesting to see what are the effects of cacheline aligning mmu_lock.

Actually, the improvement of lockless is that it can let vcpu to be parallel as possible.

From the test result, lockless gains little improvement for unix-migration, in this case, the vcpus are almost idle (at least not busy).

The large improvement is from dbench-migration, in this case, all vcpus are busy accessing memory which is write-protected by dirty-log. If you enable page-fault/fast-page-fault tracepoints, you can see huge number of page fault from different vcpu during the migration.

Quoted 1 line

Was the problem really mmu_lock contention?

Takuya, i am so tired to argue the advantage of lockless write-protect and lockless O(1) dirty-log again and again.

Quoted 3 lines

Although I am not certain about what will be really needed in the final form, if this kind of maybe-needed-overhead is going to be added little by little, I worry about possible regression.

Well, will you suggest Linus to reject all patches and stop all discussion for the "possible regression" reason?

Quoted 4 lines · On Wed, 02 May 2012 13:39:51 +0800 Xiao Guangrong…

Quoted 1 line

Was the problem really mmu_lock contention?

Takuya, i am so tired to argue the advantage of lockless write-protect and lockless O(1) dirty-log again and again.

You are missing my point. Please do not take my comments as an objection to your whole work: whey do you feel so?

I thought that your new fast-page-fault path was fast and optimized the guest during dirty logging.

So in this v4, you might get a similar result even before dropping mmu_lock, without 07/10?, if the problem Marcelo explained was not there.

Of course there is a problem of mmu_lock contention. What I am suggesting is to split that problem and do measurement separately so that part of your work can be merged soon.

Your guest size and workload was small to make get_dirty hold mmu_lock long time. If you want to appeal the real value of lock-less, you need to do another measurment.

But this is your work and it's up to you. Although I was thinking to help your measurement, I cannot do that knowing the fact that you would not welcome my help.

Quoted 7 lines

Quoted 4 lines

Although I am not certain about what will be really needed in the final form, if this kind of maybe-needed-overhead is going to be added little by little, I worry about possible regression.

Well, will you suggest Linus to reject all patches and stop all discussion for the "possible regression" reason?

My concern was for Marcelo's examples, not your current implementation. If you can show explicitely what will be needed in the final form, I do not have any concern.

Sorry for disturbing.

Takuya, i am sorry, please forgive my rudeness! Since my English is so poor that it is easy for me to misunderstand the mail.　:(

Quoted 3 lines

But this is your work and it's up to you. Although I was thinking to help your measurement, I cannot do that knowing the fact that you would not welcome my help.

Of course, any measurement is appreciative!

Quoted 1 line

Sorry for disturbing.

Sorry again.