Discussion:
[PATCH 1/2] KVM: mmu_notifier: Flush TLBs before releasing mmu_lock
(too old to reply)
Takuya Yoshikawa
2012-02-10 06:28:31 UTC
Permalink
Other threads may process the same page in that small window and skip
TLB flush and then return before these functions do flush.

Signed-off-by: Takuya Yoshikawa <***@oss.ntt.co.jp>
---
virt/kvm/kvm_main.c | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 470e305..2b4bc77 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -289,15 +289,15 @@ static void kvm_mmu_notifier_invalidate_page(struct mmu_notifier *mn,
*/
idx = srcu_read_lock(&kvm->srcu);
spin_lock(&kvm->mmu_lock);
+
kvm->mmu_notifier_seq++;
need_tlb_flush = kvm_unmap_hva(kvm, address) | kvm->tlbs_dirty;
- spin_unlock(&kvm->mmu_lock);
- srcu_read_unlock(&kvm->srcu, idx);
-
/* we've to flush the tlb before the pages can be freed */
if (need_tlb_flush)
kvm_flush_remote_tlbs(kvm);

+ spin_unlock(&kvm->mmu_lock);
+ srcu_read_unlock(&kvm->srcu, idx);
}

static void kvm_mmu_notifier_change_pte(struct mmu_notifier *mn,
@@ -335,12 +335,12 @@ static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
for (; start < end; start += PAGE_SIZE)
need_tlb_flush |= kvm_unmap_hva(kvm, start);
need_tlb_flush |= kvm->tlbs_dirty;
- spin_unlock(&kvm->mmu_lock);
- srcu_read_unlock(&kvm->srcu, idx);
-
/* we've to flush the tlb before the pages can be freed */
if (need_tlb_flush)
kvm_flush_remote_tlbs(kvm);
+
+ spin_unlock(&kvm->mmu_lock);
+ srcu_read_unlock(&kvm->srcu, idx);
}

static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,
@@ -378,13 +378,14 @@ static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn,

idx = srcu_read_lock(&kvm->srcu);
spin_lock(&kvm->mmu_lock);
- young = kvm_age_hva(kvm, address);
- spin_unlock(&kvm->mmu_lock);
- srcu_read_unlock(&kvm->srcu, idx);

+ young = kvm_age_hva(kvm, address);
if (young)
kvm_flush_remote_tlbs(kvm);

+ spin_unlock(&kvm->mmu_lock);
+ srcu_read_unlock(&kvm->srcu, idx);
+
return young;
}
--
1.7.5.4

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Takuya Yoshikawa
2012-02-10 06:29:50 UTC
Permalink
This function's TLB flush was moved sometimes in the past:

1.
commit 4539b35881ae9664b0e2953438dd83f5ee02c0b4
KVM: Fix missing smp tlb flush in invlpg

inserted it in the critical section.

2.
commit 505aef8f30a95f7e4abf2c07e54ded1521587ba0
KVM: MMU: cleanup FNAME(invlpg)

moved it inside the loop: right after mmu_page_zap_pte().

3.
commit f57f2ef58f6703e6df70ed52a198920cb3e8edba
KVM: MMU: fast prefetch spte on invlpg path

inserted update_pte() after it.

In addition, the caller, kvm_mmu_invlpg(), still does flush.

This patch simplifies the logic and removes this redundancy.

Signed-off-by: Takuya Yoshikawa <***@oss.ntt.co.jp>
---
arch/x86/kvm/mmu.c | 1 -
arch/x86/kvm/paging_tmpl.h | 6 ++++--
2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index ae76cc3..ea27ee3 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3769,7 +3769,6 @@ EXPORT_SYMBOL_GPL(kvm_mmu_page_fault);
void kvm_mmu_invlpg(struct kvm_vcpu *vcpu, gva_t gva)
{
vcpu->arch.mmu.invlpg(vcpu, gva);
- kvm_mmu_flush_tlb(vcpu);
++vcpu->stat.invlpg;
}
EXPORT_SYMBOL_GPL(kvm_mmu_invlpg);
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 1561028..69d06f5 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -682,6 +682,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
mmu_topup_memory_caches(vcpu);

spin_lock(&vcpu->kvm->mmu_lock);
+
for_each_shadow_entry(vcpu, gva, iterator) {
level = iterator.level;
sptep = iterator.sptep;
@@ -697,8 +698,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
pte_gpa = FNAME(get_level1_sp_gpa)(sp);
pte_gpa += (sptep - sp->spt) * sizeof(pt_element_t);

- if (mmu_page_zap_pte(vcpu->kvm, sp, sptep))
- kvm_flush_remote_tlbs(vcpu->kvm);
+ mmu_page_zap_pte(vcpu->kvm, sp, sptep);

if (!rmap_can_add(vcpu))
break;
@@ -713,6 +713,8 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
if (!is_shadow_present_pte(*sptep) || !sp->unsync_children)
break;
}
+
+ kvm_flush_remote_tlbs(vcpu->kvm);
spin_unlock(&vcpu->kvm->mmu_lock);
}
--
1.7.5.4

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Xiao Guangrong
2012-02-10 06:55:58 UTC
Permalink
Post by Takuya Yoshikawa
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 1561028..69d06f5 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -682,6 +682,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
mmu_topup_memory_caches(vcpu);
spin_lock(&vcpu->kvm->mmu_lock);
+
for_each_shadow_entry(vcpu, gva, iterator) {
level = iterator.level;
sptep = iterator.sptep;
@@ -697,8 +698,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
pte_gpa = FNAME(get_level1_sp_gpa)(sp);
pte_gpa += (sptep - sp->spt) * sizeof(pt_element_t);
- if (mmu_page_zap_pte(vcpu->kvm, sp, sptep))
- kvm_flush_remote_tlbs(vcpu->kvm);
+ mmu_page_zap_pte(vcpu->kvm, sp, sptep);
if (!rmap_can_add(vcpu))
break;
@@ -713,6 +713,8 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
if (!is_shadow_present_pte(*sptep) || !sp->unsync_children)
break;
}
+
+ kvm_flush_remote_tlbs(vcpu->kvm);
spin_unlock(&vcpu->kvm->mmu_lock);
It is obvious wrong, i do not think all tlbs always need be flushed...

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Takuya Yoshikawa
2012-02-10 07:21:53 UTC
Permalink
Post by Xiao Guangrong
Post by Takuya Yoshikawa
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 1561028..69d06f5 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -682,6 +682,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
mmu_topup_memory_caches(vcpu);
spin_lock(&vcpu->kvm->mmu_lock);
+
for_each_shadow_entry(vcpu, gva, iterator) {
level = iterator.level;
sptep = iterator.sptep;
@@ -697,8 +698,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
pte_gpa = FNAME(get_level1_sp_gpa)(sp);
pte_gpa += (sptep - sp->spt) * sizeof(pt_element_t);
- if (mmu_page_zap_pte(vcpu->kvm, sp, sptep))
- kvm_flush_remote_tlbs(vcpu->kvm);
+ mmu_page_zap_pte(vcpu->kvm, sp, sptep);
if (!rmap_can_add(vcpu))
break;
@@ -713,6 +713,8 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
if (!is_shadow_present_pte(*sptep) || !sp->unsync_children)
break;
}
+
+ kvm_flush_remote_tlbs(vcpu->kvm);
spin_unlock(&vcpu->kvm->mmu_lock);
It is obvious wrong, i do not think all tlbs always need be flushed...
What do you mean by "obvious wrong" ?

Even before this patch, we were always flushing TLBs from the caller.

I have a question: your patches apparently changed the timing of TLB flush
but all I could see from the changelogs were:


KVM: MMU: cleanup FNAME(invlpg)

Directly Use mmu_page_zap_pte to zap spte in FNAME(invlpg), also remove the
same code between FNAME(invlpg) and FNAME(sync_page)


KVM: MMU: fast prefetch spte on invlpg path

Fast prefetch spte for the unsync shadow page on invlpg path


Please explain what your patches semantically changed before checking the
details of my patch.

Thanks,
Takuya
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Xiao Guangrong
2012-02-10 07:42:14 UTC
Permalink
Post by Takuya Yoshikawa
Post by Xiao Guangrong
Post by Takuya Yoshikawa
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 1561028..69d06f5 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -682,6 +682,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
mmu_topup_memory_caches(vcpu);
spin_lock(&vcpu->kvm->mmu_lock);
+
for_each_shadow_entry(vcpu, gva, iterator) {
level = iterator.level;
sptep = iterator.sptep;
@@ -697,8 +698,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
pte_gpa = FNAME(get_level1_sp_gpa)(sp);
pte_gpa += (sptep - sp->spt) * sizeof(pt_element_t);
- if (mmu_page_zap_pte(vcpu->kvm, sp, sptep))
- kvm_flush_remote_tlbs(vcpu->kvm);
+ mmu_page_zap_pte(vcpu->kvm, sp, sptep);
if (!rmap_can_add(vcpu))
break;
@@ -713,6 +713,8 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
if (!is_shadow_present_pte(*sptep) || !sp->unsync_children)
break;
}
+
+ kvm_flush_remote_tlbs(vcpu->kvm);
spin_unlock(&vcpu->kvm->mmu_lock);
It is obvious wrong, i do not think all tlbs always need be flushed...
What do you mean by "obvious wrong" ?
In the current code, all tlbs are flushed only when s spte is zapped, but after
your change, they are always changed.
Post by Takuya Yoshikawa
Even before this patch, we were always flushing TLBs from the caller.
Oh, could you please tell me where tlbs can be flushed except when a spte is
zapped in this path?
Post by Takuya Yoshikawa
I have a question: your patches apparently changed the timing of TLB flush
KVM: MMU: cleanup FNAME(invlpg)
Directly Use mmu_page_zap_pte to zap spte in FNAME(invlpg), also remove the
same code between FNAME(invlpg) and FNAME(sync_page)
This patch dose not change the logic, the tlb flushed time is also not changed,
it just directly call kvm_flush_remote_tlbs when a spte is zapped.
Post by Takuya Yoshikawa
KVM: MMU: fast prefetch spte on invlpg path
Fast prefetch spte for the unsync shadow page on invlpg path
This patch did not change the code when kvm_flush_remote_tlbs is called.

Where cause your confused?

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Takuya Yoshikawa
2012-02-14 04:36:51 UTC
Permalink
(Sorry for the delay, I was in bad form this weekend.)

I am sorry, I see what I was misreading.
My eyes misread "kvm_mmu_flush_tlb()" as "kvm_flush_remote_tlbs()".
That's why I could not understand what you said, really sorry.
Post by Xiao Guangrong
Post by Takuya Yoshikawa
Post by Xiao Guangrong
It is obvious wrong, i do not think all tlbs always need be flushed...
What do you mean by "obvious wrong" ?
In the current code, all tlbs are flushed only when s spte is zapped, but after
your change, they are always changed.
Above description will probably tell you what I was thinking.
Post by Xiao Guangrong
Post by Takuya Yoshikawa
Even before this patch, we were always flushing TLBs from the caller.
Oh, could you please tell me where tlbs can be flushed except when a spte is
zapped in this path?
Ditto.
Post by Xiao Guangrong
Post by Takuya Yoshikawa
I have a question: your patches apparently changed the timing of TLB flush
KVM: MMU: cleanup FNAME(invlpg)
Directly Use mmu_page_zap_pte to zap spte in FNAME(invlpg), also remove the
same code between FNAME(invlpg) and FNAME(sync_page)
This patch dose not change the logic, the tlb flushed time is also not changed,
it just directly call kvm_flush_remote_tlbs when a spte is zapped.
Post by Takuya Yoshikawa
KVM: MMU: fast prefetch spte on invlpg path
Fast prefetch spte for the unsync shadow page on invlpg path
This patch did not change the code when kvm_flush_remote_tlbs is called.
Where cause your confused?
Thank you for your explanation!

Probably I should reread the code after taking enough sleep.

BTW, do you think that "kvm_mmu_flush_tlb()" should be moved inside of the
mmu_lock critical section?


Takuya
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Takuya Yoshikawa
2012-02-14 04:56:17 UTC
Permalink
Post by Takuya Yoshikawa
BTW, do you think that "kvm_mmu_flush_tlb()" should be moved inside of the
mmu_lock critical section?
Ah, forget about this. Trivially no.
I really need to take a rest.

Sorry,
Takuya
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Andrea Arcangeli
2012-02-14 17:21:50 UTC
Permalink
Post by Takuya Yoshikawa
Post by Takuya Yoshikawa
BTW, do you think that "kvm_mmu_flush_tlb()" should be moved inside of the
mmu_lock critical section?
Ah, forget about this. Trivially no.
Yes the reason is that it's the local flush and guest mode isn't
running if we're running that function so it's ok to run it later.

About the other change you did in this patch 2/2, I can't find the
code you're patching in the 3.2 upstream source, when I added the tlb
flush to invlpg, I immediately used a cumulative need_flush at the end
(before relasing mmu_lock of course).

if (need_flush)
kvm_flush_remote_tlbs(vcpu->kvm);
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Xiao Guangrong
2012-02-10 07:52:49 UTC
Permalink
Post by Takuya Yoshikawa
Other threads may process the same page in that small window and skip
TLB flush and then return before these functions do flush.
It is possible that flush tlb in mmu lock only when writeable
spte is invalided? Sometimes, kvm_flush_remote_tlbs need
long time to wait.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Takuya Yoshikawa
2012-02-13 06:00:41 UTC
Permalink
Post by Xiao Guangrong
Post by Takuya Yoshikawa
Other threads may process the same page in that small window and skip
TLB flush and then return before these functions do flush.
It is possible that flush tlb in mmu lock only when writeable
spte is invalided? Sometimes, kvm_flush_remote_tlbs need
long time to wait.
I am sorry but I don't see the point of adding yet another complexity
for that.


OK, let me clarify my position.

When I made this patch, I did a simple test to see if mmu notifier would
work normally after my patch applied: I allocated large memory behind
the guest and made the host call mmu_notifier functions.

In that situation the host as a whole became slow, e.g. I saw really bad
response for some seconds.

In such a case, do you mind the additional time VCPU threads may have to
wait until the mmu notifier finishes the flush, only when they are trying
to take the mmu_lock?


Takuya
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Andrea Arcangeli
2012-02-14 17:27:39 UTC
Permalink
Post by Xiao Guangrong
Post by Takuya Yoshikawa
Other threads may process the same page in that small window and skip
TLB flush and then return before these functions do flush.
It is possible that flush tlb in mmu lock only when writeable
spte is invalided? Sometimes, kvm_flush_remote_tlbs need
long time to wait.
readonly isn't enough to defer the flush after mmu_lock is
released... if you do it only for writable spte, then what can happen
is the guest may read random data and would crash.

However for this case, the mmu_notifier methods (and only them) are
perfectly safe to flush the shadow MMU TLB after the mmu_lock is
released because the page reference is guaranteed hold by the caller
(not the case for any other place where a spte gets dropped in KVM,
all other places dropping sptes, can only on the mmu notifier to block
on the mmu_lock in order to have a guarantee of the page not being
freed under them, so in every other place the shadow MMU TLB flush
must happen before releasing the mmu_lock so the mmu_notifier will
wait and prevent the page to be freed until all other CPUs running in
guest mode stopped accessing it).
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Marcelo Tosatti
2012-02-10 17:26:46 UTC
Permalink
Post by Takuya Yoshikawa
Other threads may process the same page in that small window and skip
TLB flush and then return before these functions do flush.
---
virt/kvm/kvm_main.c | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)
Applied, thanks.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Andrea Arcangeli
2012-02-14 17:10:44 UTC
Permalink
Post by Takuya Yoshikawa
Other threads may process the same page in that small window and skip
TLB flush and then return before these functions do flush.
It's correct to flush the shadow MMU TLB without the mmu_lock only in
the context of mmu notifier methods. So the below while won't hurt,
it's performance regression and shouldn't be applied (and
it obfuscates the code by not being strict anymore).

To the contrary every other place that does a shadow/secondary MMU smp
tlb flush _must_ happen inside the mmu_lock, otherwise the
serialization isn't correct anymore against the very below mmu_lock in
the below quoted patch taken by
kvm_mmu_notifier_invalidate_page/kvm_mmu_notifier_invalidate_range_start.

The explanation is in commit 4539b35881ae9664b0e2953438dd83f5ee02c0b4.

I'll try to explain it more clearly: the moment you drop mmu_lock,
pages can be freed. So if you invalidate a spte in any place inside
the KVM code (except the mmu notifier methods where a reference of the
page is implicitly hold by the caller and so the page can't go away
under a mmu notifier method by design), then the below
kvm_mmu_notifier_invalidate_page/kvm_mmu_notifier_invalidate_range_start
won't get their need_tlb_flush set anymore, and they won't run the tlb
flush before freeing the page.

So every other place (except mmu notifier) must flush the secondary
MMU smp tlb _before_ releasing the mmu_lock.

Only mmu notifier is safe to flush the secondary MMU TLB _after_
releasing the mmu_lock.

If we changed the mmu notifier methods to unconditionally flush the
shadow TLB (regardless if a spte was present or not), we might not
need to hold the mmu_lock in every tlb flush outside the context of
the mmu notifier methods. But then the mmu notifier methods would
become more expensive, I didn't evaluate fully what would be the side
effects of such a change. Also note, only the
kvm_mmu_notifier_invalidate_page and
kvm_mmu_notifier_invalidate_range_start would need to do that, because
they're the only two where the page reference gets dropped.

Even shorter: because the mmu notifier a implicit reference on the
page exists and is hold by the caller, they can flush outside the
mmu_lock. Every other place in KVM only holds an implicit valid
reference on the page only as long as you hold the mmu_lock, or while
a spte is still established.

Well it's not easy logic so it's not surprising it wasn't totally
clear.

It's probably not heavily documented, and the fact you changed it
still is still good so we refresh our minds on the exact rules of mmu
notifier locking, thanks!

Andrea
Post by Takuya Yoshikawa
---
virt/kvm/kvm_main.c | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 470e305..2b4bc77 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -289,15 +289,15 @@ static void kvm_mmu_notifier_invalidate_page(struct mmu_notifier *mn,
*/
idx = srcu_read_lock(&kvm->srcu);
spin_lock(&kvm->mmu_lock);
+
kvm->mmu_notifier_seq++;
need_tlb_flush = kvm_unmap_hva(kvm, address) | kvm->tlbs_dirty;
- spin_unlock(&kvm->mmu_lock);
- srcu_read_unlock(&kvm->srcu, idx);
-
/* we've to flush the tlb before the pages can be freed */
if (need_tlb_flush)
kvm_flush_remote_tlbs(kvm);
+ spin_unlock(&kvm->mmu_lock);
+ srcu_read_unlock(&kvm->srcu, idx);
}
static void kvm_mmu_notifier_change_pte(struct mmu_notifier *mn,
@@ -335,12 +335,12 @@ static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
for (; start < end; start += PAGE_SIZE)
need_tlb_flush |= kvm_unmap_hva(kvm, start);
need_tlb_flush |= kvm->tlbs_dirty;
- spin_unlock(&kvm->mmu_lock);
- srcu_read_unlock(&kvm->srcu, idx);
-
/* we've to flush the tlb before the pages can be freed */
if (need_tlb_flush)
kvm_flush_remote_tlbs(kvm);
+
+ spin_unlock(&kvm->mmu_lock);
+ srcu_read_unlock(&kvm->srcu, idx);
}
static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,
@@ -378,13 +378,14 @@ static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn,
idx = srcu_read_lock(&kvm->srcu);
spin_lock(&kvm->mmu_lock);
- young = kvm_age_hva(kvm, address);
- spin_unlock(&kvm->mmu_lock);
- srcu_read_unlock(&kvm->srcu, idx);
+ young = kvm_age_hva(kvm, address);
if (young)
kvm_flush_remote_tlbs(kvm);
+ spin_unlock(&kvm->mmu_lock);
+ srcu_read_unlock(&kvm->srcu, idx);
+
return young;
}
--
1.7.5.4
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Marcelo Tosatti
2012-02-14 17:29:47 UTC
Permalink
Post by Andrea Arcangeli
Post by Takuya Yoshikawa
Other threads may process the same page in that small window and skip
TLB flush and then return before these functions do flush.
It's correct to flush the shadow MMU TLB without the mmu_lock only in
the context of mmu notifier methods. So the below while won't hurt,
it's performance regression and shouldn't be applied (and
it obfuscates the code by not being strict anymore).
To the contrary every other place that does a shadow/secondary MMU smp
tlb flush _must_ happen inside the mmu_lock, otherwise the
serialization isn't correct anymore against the very below mmu_lock in
the below quoted patch taken by
kvm_mmu_notifier_invalidate_page/kvm_mmu_notifier_invalidate_range_start.
The explanation is in commit 4539b35881ae9664b0e2953438dd83f5ee02c0b4.
I'll try to explain it more clearly: the moment you drop mmu_lock,
pages can be freed. So if you invalidate a spte in any place inside
the KVM code (except the mmu notifier methods where a reference of the
page is implicitly hold by the caller and so the page can't go away
under a mmu notifier method by design), then the below
kvm_mmu_notifier_invalidate_page/kvm_mmu_notifier_invalidate_range_start
won't get their need_tlb_flush set anymore, and they won't run the tlb
flush before freeing the page.
So every other place (except mmu notifier) must flush the secondary
MMU smp tlb _before_ releasing the mmu_lock.
Only mmu notifier is safe to flush the secondary MMU TLB _after_
releasing the mmu_lock.
If we changed the mmu notifier methods to unconditionally flush the
shadow TLB (regardless if a spte was present or not), we might not
need to hold the mmu_lock in every tlb flush outside the context of
the mmu notifier methods. But then the mmu notifier methods would
become more expensive, I didn't evaluate fully what would be the side
effects of such a change. Also note, only the
kvm_mmu_notifier_invalidate_page and
kvm_mmu_notifier_invalidate_range_start would need to do that, because
they're the only two where the page reference gets dropped.
Even shorter: because the mmu notifier a implicit reference on the
page exists and is hold by the caller, they can flush outside the
mmu_lock. Every other place in KVM only holds an implicit valid
reference on the page only as long as you hold the mmu_lock, or while
a spte is still established.
Well it's not easy logic so it's not surprising it wasn't totally
clear.
It's probably not heavily documented, and the fact you changed it
still is still good so we refresh our minds on the exact rules of mmu
notifier locking, thanks!
The problem the patch is fixing is not related to page freeing, but
rmap_write_protect. From 8bf3f6f06fcdfd097b6c6ec51531d8292fa0d81d
(replace "A (get_dirty_log)" with "mmu_notifier_invalidate_page"):


During protecting pages for dirty logging, other threads may also try
to protect a page in mmu_sync_children() or kvm_mmu_get_page().

In such a case, because get_dirty_log releases mmu_lock before flushing
TLB's, the following race condition can happen:

A (get_dirty_log) B (another thread)

lock(mmu_lock)
clear pte.w
unlock(mmu_lock)
lock(mmu_lock)
pte.w is already cleared
unlock(mmu_lock)
skip TLB flush
return
...
TLB flush

Though thread B assumes the page has already been protected when it
returns, the remaining TLB entry will break that assumption.
Post by Andrea Arcangeli
Andrea
Post by Takuya Yoshikawa
---
virt/kvm/kvm_main.c | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 470e305..2b4bc77 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -289,15 +289,15 @@ static void kvm_mmu_notifier_invalidate_page(struct mmu_notifier *mn,
*/
idx = srcu_read_lock(&kvm->srcu);
spin_lock(&kvm->mmu_lock);
+
kvm->mmu_notifier_seq++;
need_tlb_flush = kvm_unmap_hva(kvm, address) | kvm->tlbs_dirty;
- spin_unlock(&kvm->mmu_lock);
- srcu_read_unlock(&kvm->srcu, idx);
-
/* we've to flush the tlb before the pages can be freed */
if (need_tlb_flush)
kvm_flush_remote_tlbs(kvm);
+ spin_unlock(&kvm->mmu_lock);
+ srcu_read_unlock(&kvm->srcu, idx);
}
static void kvm_mmu_notifier_change_pte(struct mmu_notifier *mn,
@@ -335,12 +335,12 @@ static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
for (; start < end; start += PAGE_SIZE)
need_tlb_flush |= kvm_unmap_hva(kvm, start);
need_tlb_flush |= kvm->tlbs_dirty;
- spin_unlock(&kvm->mmu_lock);
- srcu_read_unlock(&kvm->srcu, idx);
-
/* we've to flush the tlb before the pages can be freed */
if (need_tlb_flush)
kvm_flush_remote_tlbs(kvm);
+
+ spin_unlock(&kvm->mmu_lock);
+ srcu_read_unlock(&kvm->srcu, idx);
}
static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,
@@ -378,13 +378,14 @@ static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn,
idx = srcu_read_lock(&kvm->srcu);
spin_lock(&kvm->mmu_lock);
- young = kvm_age_hva(kvm, address);
- spin_unlock(&kvm->mmu_lock);
- srcu_read_unlock(&kvm->srcu, idx);
+ young = kvm_age_hva(kvm, address);
if (young)
kvm_flush_remote_tlbs(kvm);
+ spin_unlock(&kvm->mmu_lock);
+ srcu_read_unlock(&kvm->srcu, idx);
+
return young;
}
--
1.7.5.4
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Andrea Arcangeli
2012-02-14 18:53:56 UTC
Permalink
Post by Marcelo Tosatti
The problem the patch is fixing is not related to page freeing, but
rmap_write_protect. From 8bf3f6f06fcdfd097b6c6ec51531d8292fa0d81d
Can't find the commit on kvm.git.
Post by Marcelo Tosatti
During protecting pages for dirty logging, other threads may also try
to protect a page in mmu_sync_children() or kvm_mmu_get_page().
In such a case, because get_dirty_log releases mmu_lock before flushing
A (get_dirty_log) B (another thread)
lock(mmu_lock)
clear pte.w
unlock(mmu_lock)
lock(mmu_lock)
pte.w is already cleared
unlock(mmu_lock)
skip TLB flush
Not sure which tree it is, but in kvm and upstream I see an
unconditional tlb flush here, not skip (both
kvm_mmu_slot_remove_write_access and kvm_mmu_rmap_write_protect). So I
assume this has been updated in your tree to eb conditional.

Also note kvm_mmu_rmap_write_protect, flushes outside of the mmu_lock
in the kvm_mmu_rmap_write_protect case (like in quoted description),
so two write_protect_slot in parallel against each other may not be
ok, but that may be enforced by design if qemu won't ever call that
ioctl from two different userland threads (it doesn't sounds security
related so it should be ok to enforce its safety by userland design).
Post by Marcelo Tosatti
return
...
TLB flush
Though thread B assumes the page has already been protected when it
returns, the remaining TLB entry will break that assumption.
Now I get the question of why not running the TLB flush inside the
mmu_lock only if the spte was writable :).

kvm_mmu_get_page as long as it only runs in the context of a kvm page
fault is ok, because the page fault would be inhibited by the mmu
notifier invalidates, so maybe it's safe.

mmu_sync_children seems to have a problem instead, in your tree
get_dirty_log also has an issue if it has been updated to skip the
flush on readonly sptes, like I guess.

Interesting how the spte is already non present, the page is just
being freed shortly later, but yet we still need to trigger write
faults synchronously and prevent other CPUs in guest mode to further
modify the page to avoid losing dirty bits updates or updates on
pagetables that maps pagetables in the not NPT/EPT case. If the page
was really only going to be freed it would be ok if the other cpus
would still write to it for a little longer until the page was freed.

Like I wrote in previous email, I was thinking if we'd change the mmu
notifier methods to do an unconditional flush, then every other flush
could also run outside of the mmu_lock. But then I didn't think enough
about this to be sure. My guess is we could move all flushes outside
the mmu_lock if we stop flushling the tlb conditonally to the current
spte values. It'd clearly be slower for an UP guest though :). Large
SMP guests might benefit, if that is feasible at all... It depends how
problematic the mmu_lock is on the large SMP guests and how much we're
saving by doing conditional TLB flushes.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Marcelo Tosatti
2012-02-14 19:43:42 UTC
Permalink
Post by Andrea Arcangeli
Post by Marcelo Tosatti
The problem the patch is fixing is not related to page freeing, but
rmap_write_protect. From 8bf3f6f06fcdfd097b6c6ec51531d8292fa0d81d
Can't find the commit on kvm.git.
Sorry, we got kvm.git out of sync. But you can see an equivalent below.
Post by Andrea Arcangeli
Post by Marcelo Tosatti
During protecting pages for dirty logging, other threads may also try
to protect a page in mmu_sync_children() or kvm_mmu_get_page().
In such a case, because get_dirty_log releases mmu_lock before flushing
A (get_dirty_log) B (another thread)
lock(mmu_lock)
clear pte.w
unlock(mmu_lock)
lock(mmu_lock)
pte.w is already cleared
unlock(mmu_lock)
skip TLB flush
Not sure which tree it is, but in kvm and upstream I see an
unconditional tlb flush here, not skip (both
kvm_mmu_slot_remove_write_access and kvm_mmu_rmap_write_protect). So I
assume this has been updated in your tree to eb conditional.
if (!direct) {
if (rmap_write_protect(vcpu->kvm, gfn))
kvm_flush_remote_tlbs(vcpu->kvm);
Post by Andrea Arcangeli
Also note kvm_mmu_rmap_write_protect, flushes outside of the mmu_lock
in the kvm_mmu_rmap_write_protect case (like in quoted description),
so two write_protect_slot in parallel against each other may not be
ok, but that may be enforced by design if qemu won't ever call that
ioctl from two different userland threads (it doesn't sounds security
related so it should be ok to enforce its safety by userland design).
Yes, here is the fix:

http://git.kernel.org/?p=virt/kvm/kvm.git;a=commit;h=02b48d00d7f1853bdf8a06da19ca5413ebe334c6

This is an equivalent of 8bf3f6f06fcdfd097b6c6ec51531d8292fa0d81d.
Post by Andrea Arcangeli
Post by Marcelo Tosatti
return
...
TLB flush
Though thread B assumes the page has already been protected when it
returns, the remaining TLB entry will break that assumption.
Now I get the question of why not running the TLB flush inside the
mmu_lock only if the spte was writable :).
kvm_mmu_get_page as long as it only runs in the context of a kvm page
fault is ok, because the page fault would be inhibited by the mmu
notifier invalidates, so maybe it's safe.
Ah, perhaps, but this was not taken into account before. Can you confirm
this is the case so we can revert the invalidate_page patch?
Post by Andrea Arcangeli
mmu_sync_children seems to have a problem instead, in your tree
get_dirty_log also has an issue if it has been updated to skip the
flush on readonly sptes, like I guess.
Interesting how the spte is already non present, the page is just
being freed shortly later, but yet we still need to trigger write
faults synchronously and prevent other CPUs in guest mode to further
modify the page to avoid losing dirty bits updates or updates on
pagetables that maps pagetables in the not NPT/EPT case. If the page
was really only going to be freed it would be ok if the other cpus
would still write to it for a little longer until the page was freed.
Like I wrote in previous email, I was thinking if we'd change the mmu
notifier methods to do an unconditional flush, then every other flush
could also run outside of the mmu_lock. But then I didn't think enough
about this to be sure. My guess is we could move all flushes outside
the mmu_lock if we stop flushling the tlb conditonally to the current
spte values. It'd clearly be slower for an UP guest though :). Large
SMP guests might benefit, if that is feasible at all... It depends how
problematic the mmu_lock is on the large SMP guests and how much we're
saving by doing conditional TLB flushes.
Also it should not be necessary for these flushes to be inside mmu_lock
on EPT/NPT case (since there is no write protection there). But it would
be awkward to differentiate the unlock position based on EPT/NPT.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Avi Kivity
2012-02-15 09:18:31 UTC
Permalink
Post by Marcelo Tosatti
Also it should not be necessary for these flushes to be inside mmu_lock
on EPT/NPT case (since there is no write protection there).
We do write protect with TDP, if nested virt is active. The question is
whether we have indirect pages or not, not whether TDP is active or not
(even without TDP, if you don't enable paging in the guest, you don't
have to write protect).
Post by Marcelo Tosatti
But it would
be awkward to differentiate the unlock position based on EPT/NPT.
I would really like to move the IPI back out of the lock.

How about something like a sequence lock:


spin_lock(mmu_lock)
need_flush = write_protect_stuff();
atomic_add(kvm->want_flush_counter, need_flush);
spin_unlock(mmu_lock);

while ((done = atomic_read(kvm->done_flush_counter)) < (want =
atomic_read(kvm->want_flush_counter)) {
kvm_make_request(flush)
atomic_cmpxchg(kvm->done_flush_counter, done, want)
}

This (or maybe a corrected and optimized version) ensures that any
need_flush cannot pass the while () barrier, no matter which thread
encounters it first. However it violates the "do not invent new locking
techniques" commandment. Can we map it to some existing method?
--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Avi Kivity
2012-02-15 09:47:11 UTC
Permalink
Post by Avi Kivity
Post by Marcelo Tosatti
Also it should not be necessary for these flushes to be inside mmu_lock
on EPT/NPT case (since there is no write protection there).
We do write protect with TDP, if nested virt is active. The question is
whether we have indirect pages or not, not whether TDP is active or not
(even without TDP, if you don't enable paging in the guest, you don't
have to write protect).
Post by Marcelo Tosatti
But it would
be awkward to differentiate the unlock position based on EPT/NPT.
I would really like to move the IPI back out of the lock.
spin_lock(mmu_lock)
need_flush = write_protect_stuff();
atomic_add(kvm->want_flush_counter, need_flush);
spin_unlock(mmu_lock);
while ((done = atomic_read(kvm->done_flush_counter)) < (want =
atomic_read(kvm->want_flush_counter)) {
kvm_make_request(flush)
atomic_cmpxchg(kvm->done_flush_counter, done, want)
}
This (or maybe a corrected and optimized version) ensures that any
need_flush cannot pass the while () barrier, no matter which thread
encounters it first. However it violates the "do not invent new locking
techniques" commandment. Can we map it to some existing method?
There is no need to advance 'want' in the loop. So we could do

/* must call with mmu_lock held */
void kvm_mmu_defer_remote_flush(kvm, need_flush)
{
if (need_flush)
++kvm->flush_counter.want;
}

/* may call without mmu_lock */
void kvm_mmu_commit_remote_flush(kvm)
{
want = ACCESS_ONCE(kvm->flush_counter.want)
while ((done = atomic_read(kvm->flush_counter.done) < want) {
kvm_make_request(flush)
atomic_cmpxchg(kvm->flush_counter.done, done, want)
}
}
--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Xiao Guangrong
2012-02-15 11:37:38 UTC
Permalink
Post by Avi Kivity
Post by Avi Kivity
Post by Marcelo Tosatti
Also it should not be necessary for these flushes to be inside mmu_lock
on EPT/NPT case (since there is no write protection there).
We do write protect with TDP, if nested virt is active. The question is
whether we have indirect pages or not, not whether TDP is active or not
(even without TDP, if you don't enable paging in the guest, you don't
have to write protect).
Post by Marcelo Tosatti
But it would
be awkward to differentiate the unlock position based on EPT/NPT.
I would really like to move the IPI back out of the lock.
spin_lock(mmu_lock)
need_flush = write_protect_stuff();
atomic_add(kvm->want_flush_counter, need_flush);
spin_unlock(mmu_lock);
while ((done = atomic_read(kvm->done_flush_counter)) < (want =
atomic_read(kvm->want_flush_counter)) {
kvm_make_request(flush)
atomic_cmpxchg(kvm->done_flush_counter, done, want)
}
This (or maybe a corrected and optimized version) ensures that any
need_flush cannot pass the while () barrier, no matter which thread
encounters it first. However it violates the "do not invent new locking
techniques" commandment. Can we map it to some existing method?
There is no need to advance 'want' in the loop. So we could do
/* must call with mmu_lock held */
void kvm_mmu_defer_remote_flush(kvm, need_flush)
{
if (need_flush)
++kvm->flush_counter.want;
}
/* may call without mmu_lock */
void kvm_mmu_commit_remote_flush(kvm)
{
want = ACCESS_ONCE(kvm->flush_counter.want)
while ((done = atomic_read(kvm->flush_counter.done) < want) {
kvm_make_request(flush)
atomic_cmpxchg(kvm->flush_counter.done, done, want)
}
}
Hmm, we already have kvm->tlbs_dirty, so, we can do it like this:

#define SPTE_INVALID_UNCLEAN (1 << 63 )

in invalid page path:
lock mmu_lock
if (spte is invalid)
kvm->tlbs_dirty |= SPTE_INVALID_UNCLEAN;
need_tlb_flush = kvm->tlbs_dirty;
unlock mmu_lock
if (need_tlb_flush)
kvm_flush_remote_tlbs()

And in page write-protected path:
lock mmu_lock
if (it has spte change to readonly |
kvm->tlbs_dirty & SPTE_INVALID_UNCLEAN)
kvm_flush_remote_tlbs()
unlock mmu_lock

How about this?

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Avi Kivity
2012-02-15 14:07:49 UTC
Permalink
Post by Xiao Guangrong
Post by Avi Kivity
Post by Avi Kivity
I would really like to move the IPI back out of the lock.
spin_lock(mmu_lock)
need_flush = write_protect_stuff();
atomic_add(kvm->want_flush_counter, need_flush);
spin_unlock(mmu_lock);
while ((done = atomic_read(kvm->done_flush_counter)) < (want =
atomic_read(kvm->want_flush_counter)) {
kvm_make_request(flush)
atomic_cmpxchg(kvm->done_flush_counter, done, want)
}
This (or maybe a corrected and optimized version) ensures that any
need_flush cannot pass the while () barrier, no matter which thread
encounters it first. However it violates the "do not invent new locking
techniques" commandment. Can we map it to some existing method?
There is no need to advance 'want' in the loop. So we could do
/* must call with mmu_lock held */
void kvm_mmu_defer_remote_flush(kvm, need_flush)
{
if (need_flush)
++kvm->flush_counter.want;
}
/* may call without mmu_lock */
void kvm_mmu_commit_remote_flush(kvm)
{
want = ACCESS_ONCE(kvm->flush_counter.want)
while ((done = atomic_read(kvm->flush_counter.done) < want) {
kvm_make_request(flush)
atomic_cmpxchg(kvm->flush_counter.done, done, want)
}
}
#define SPTE_INVALID_UNCLEAN (1 << 63 )
lock mmu_lock
if (spte is invalid)
kvm->tlbs_dirty |= SPTE_INVALID_UNCLEAN;
need_tlb_flush = kvm->tlbs_dirty;
unlock mmu_lock
if (need_tlb_flush)
kvm_flush_remote_tlbs()
lock mmu_lock
if (it has spte change to readonly |
kvm->tlbs_dirty & SPTE_INVALID_UNCLEAN)
kvm_flush_remote_tlbs()
unlock mmu_lock
How about this?
Well, it still has flushes inside the lock. And it seems to be more
complicated, but maybe that's because I thought of my idea and didn't
fully grok yours yet.
--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Andrea Arcangeli
2012-02-15 19:16:24 UTC
Permalink
Post by Avi Kivity
Well, it still has flushes inside the lock. And it seems to be more
complicated, but maybe that's because I thought of my idea and didn't
fully grok yours yet.
If we go more complicated I prefer Avi's suggestion to move them all
outside the lock.

Yesterday I was also thinking at the regular pagetables and how we do
not have similar issues there. On the regular pagetables we just do
unconditional flush in fork when we make it readonly and KSM (the
other place that ptes stuff readonly that later can cow) uses
ptep_clear_flush which does an unconditional flush and furthermore it
does it inside the PT lock, so generally we don't optimize for those
things on the regular pagetables. But then these events don't happen
as frequently as they can on KVM without EPT/NPT.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Xiao Guangrong
2012-02-16 04:50:18 UTC
Permalink
Post by Avi Kivity
Post by Xiao Guangrong
Post by Avi Kivity
Post by Avi Kivity
I would really like to move the IPI back out of the lock.
spin_lock(mmu_lock)
need_flush = write_protect_stuff();
atomic_add(kvm->want_flush_counter, need_flush);
spin_unlock(mmu_lock);
while ((done = atomic_read(kvm->done_flush_counter)) < (want =
atomic_read(kvm->want_flush_counter)) {
kvm_make_request(flush)
atomic_cmpxchg(kvm->done_flush_counter, done, want)
}
This (or maybe a corrected and optimized version) ensures that any
need_flush cannot pass the while () barrier, no matter which thread
encounters it first. However it violates the "do not invent new locking
techniques" commandment. Can we map it to some existing method?
There is no need to advance 'want' in the loop. So we could do
/* must call with mmu_lock held */
void kvm_mmu_defer_remote_flush(kvm, need_flush)
{
if (need_flush)
++kvm->flush_counter.want;
}
/* may call without mmu_lock */
void kvm_mmu_commit_remote_flush(kvm)
{
want = ACCESS_ONCE(kvm->flush_counter.want)
while ((done = atomic_read(kvm->flush_counter.done) < want) {
kvm_make_request(flush)
atomic_cmpxchg(kvm->flush_counter.done, done, want)
}
}
#define SPTE_INVALID_UNCLEAN (1 << 63 )
lock mmu_lock
if (spte is invalid)
kvm->tlbs_dirty |= SPTE_INVALID_UNCLEAN;
need_tlb_flush = kvm->tlbs_dirty;
unlock mmu_lock
if (need_tlb_flush)
kvm_flush_remote_tlbs()
lock mmu_lock
if (it has spte change to readonly |
kvm->tlbs_dirty & SPTE_INVALID_UNCLEAN)
kvm_flush_remote_tlbs()
unlock mmu_lock
How about this?
Well, it still has flushes inside the lock. And it seems to be more
complicated, but maybe that's because I thought of my idea and didn't
fully grok yours yet.
Oh, i was not think that flush all tlbs out of mmu-lock, just invalid page
path.

But, there still have some paths need flush tlbs inside of mmu-lock(like
sync_children, get_page).
Post by Avi Kivity
Post by Xiao Guangrong
Post by Avi Kivity
/* must call with mmu_lock held */
void kvm_mmu_defer_remote_flush(kvm, need_flush)
{
if (need_flush)
++kvm->flush_counter.want;
}
/* may call without mmu_lock */
void kvm_mmu_commit_remote_flush(kvm)
{
want = ACCESS_ONCE(kvm->flush_counter.want)
while ((done = atomic_read(kvm->flush_counter.done) < want) {
kvm_make_request(flush)
atomic_cmpxchg(kvm->flush_counter.done, done, want)
}
}
I think we do not need handle all tlb-flushed request here since all of these
request can be delayed to the point where mmu-lock is released , we can simply
do it:

void kvm_mmu_defer_remote_flush(kvm, need_flush)
{
if (need_flush)
++kvm->tlbs_dirty;
}

void kvm_mmu_commit_remote_flush(struct kvm *kvm)
{
int dirty_count = kvm->tlbs_dirty;

smp_mb();

if (!dirty_count)
return;

if (make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH))
++kvm->stat.remote_tlb_flush;
cmpxchg(&kvm->tlbs_dirty, dirty_count, 0);
}

if this is ok, we only need do small change in the current code, since
kvm_mmu_commit_remote_flush is very similar with kvm_flush_remote_tlbs().

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Avi Kivity
2012-02-16 11:57:10 UTC
Permalink
Post by Xiao Guangrong
I think we do not need handle all tlb-flushed request here since all of these
request can be delayed to the point where mmu-lock is released , we can simply
void kvm_mmu_defer_remote_flush(kvm, need_flush)
{
if (need_flush)
++kvm->tlbs_dirty;
}
void kvm_mmu_commit_remote_flush(struct kvm *kvm)
{
int dirty_count = kvm->tlbs_dirty;
smp_mb();
if (!dirty_count)
return;
if (make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH))
++kvm->stat.remote_tlb_flush;
<-- point A
Post by Xiao Guangrong
cmpxchg(&kvm->tlbs_dirty, dirty_count, 0);
}
if this is ok, we only need do small change in the current code, since
kvm_mmu_commit_remote_flush is very similar with kvm_flush_remote_tlbs().
Suppose at point A another thread executes defer_remote_flush(),
commit_remote_flush(), and defer_remote_flush() again. This brings the
balue of tlbs_dirty back to 1 again, with the tlbs dirty. The cmpxchg()
then resets tlbs_dirty, leaving the actual tlbs dirty.
--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Xiao Guangrong
2012-02-17 02:36:45 UTC
Permalink
Post by Avi Kivity
Suppose at point A another thread executes defer_remote_flush(),
commit_remote_flush(), and defer_remote_flush() again. This brings the
balue of tlbs_dirty back to 1 again, with the tlbs dirty. The cmpxchg()
then resets tlbs_dirty, leaving the actual tlbs dirty.
Oh, right, sorry for my careless!

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Continue reading on narkive:
Loading...