Bechmark result:
I have tested this patchset and the previous version that only zaps the
pages linked on invalid slot's rmap. The benchmark is written by myself
which has been attached, it writes large memory when do pci rom read.
Host: Intel(R) Xeon(R) CPU X5690 @ 3.47GHz + 36G Memory
Guest: 12 VCPU + 32G Memory
Current code: This patchset Previous Version
2405434959 ns 2323016424 ns 2368810003 ns
The interesting thing is, the previous version is slower than this patch,
i guess the reason is that the former keeps lots of invalid pages in mmu
which cause shadow page to be reclaimed due to used-pages > request-pages
or host memory shrink.
Changlog:
V5:
1): rename is_valid_sp to is_obsolete_sp
2): use lock-break technique to zap all old pages instead of only pages
linked on invalid slot's rmap suggested by Marcelo.
3): trace invalid pages and kvm_mmu_invalidate_memslot_pages()
4): rename kvm_mmu_invalid_memslot_pages to kvm_mmu_invalidate_memslot_pages
according to Takuya's comments.
V4:
1): drop unmapping invalid rmap out of mmu-lock and use lock-break technique
instead. Thanks to Gleb's comments.
2): needn't handle invalid-gen pages specially due to page table always
switched by KVM_REQ_MMU_RELOAD. Thanks to Marcelo's comments.
V3:
completely redesign the algorithm, please see below.
V2:
- do not reset n_requested_mmu_pages and n_max_mmu_pages
- batch free root shadow pages to reduce vcpu notification and mmu-lock
contention
- remove the first patch that introduce kvm->arch.mmu_cache since we only
'memset zero' on hashtable rather than all mmu cache members in this
version
- remove unnecessary kvm_reload_remote_mmus after kvm_mmu_zap_all
* Issue
The current kvm_mmu_zap_all is really slow - it is holding mmu-lock to
walk and zap all shadow pages one by one, also it need to zap all guest
page's rmap and all shadow page's parent spte list. Particularly, things
become worse if guest uses more memory or vcpus. It is not good for
scalability.
* Idea
KVM maintains a global mmu invalid generation-number which is stored in
kvm->arch.mmu_valid_gen and every shadow page stores the current global
generation-number into sp->mmu_valid_gen when it is created.
When KVM need zap all shadow pages sptes, it just simply increase the
global generation-number then reload root shadow pages on all vcpus.
Vcpu will create a new shadow page table according to current kvm's
generation-number. It ensures the old pages are not used any more.
Then the invalid-gen pages (sp->mmu_valid_gen != kvm->arch.mmu_valid_gen)
are zapped by using lock-break technique.
Xiao Guangrong (8):
KVM: MMU: drop unnecessary kvm_reload_remote_mmus
KVM: MMU: delete shadow page from hash list in
kvm_mmu_prepare_zap_page
KVM: MMU: fast invalidate all pages
KVM: x86: use the fast way to invalidate all pages
KVM: MMU: make kvm_mmu_zap_all preemptable
KVM: MMU: show mmu_valid_gen in shadow page related tracepoints
KVM: MMU: add tracepoint for kvm_mmu_invalidate_memslot_pages
KVM: MMU: zap pages in batch
arch/x86/include/asm/kvm_host.h | 2 +
arch/x86/kvm/mmu.c | 124 ++++++++++++++++++++++++++++++++++++++-
arch/x86/kvm/mmu.h | 2 +
arch/x86/kvm/mmutrace.h | 45 +++++++++++---
arch/x86/kvm/x86.c | 9 +--
5 files changed, 163 insertions(+), 19 deletions(-)
--
1.7.7.6
Zap at lease 10 pages before releasing mmu-lock to reduce the overload
caused by requiring lock
[ It improves kernel building 0.6% ~ 1% ]
Signed-off-by: Xiao Guangrong <[email protected]>
---
arch/x86/kvm/mmu.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index e12f431..9c27fda 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4216,10 +4216,12 @@ restart:
spin_unlock(&kvm->mmu_lock);
}
+#define BATCH_ZAP_PAGES 10
static void zap_invalid_pages(struct kvm *kvm)
{
struct kvm_mmu_page *sp, *node;
LIST_HEAD(invalid_list);
+ int batch = 0;
restart:
list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
@@ -4256,11 +4258,14 @@ restart:
* Need not flush tlb since we only zap the sp with invalid
* generation number.
*/
- if (cond_resched_lock(&kvm->mmu_lock))
+ if ((batch >= BATCH_ZAP_PAGES) &&
+ cond_resched_lock(&kvm->mmu_lock)) {
+ batch = 0;
goto restart;
+ }
- if (kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list))
- goto restart;
+ batch += kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list);
+ goto restart;
}
/*
--
1.7.7.6
Show sp->mmu_valid_gen
Signed-off-by: Xiao Guangrong <[email protected]>
---
arch/x86/kvm/mmutrace.h | 22 ++++++++++++----------
1 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
index b8f6172..697f466 100644
--- a/arch/x86/kvm/mmutrace.h
+++ b/arch/x86/kvm/mmutrace.h
@@ -7,16 +7,18 @@
#undef TRACE_SYSTEM
#define TRACE_SYSTEM kvmmmu
-#define KVM_MMU_PAGE_FIELDS \
- __field(__u64, gfn) \
- __field(__u32, role) \
- __field(__u32, root_count) \
+#define KVM_MMU_PAGE_FIELDS \
+ __field(unsigned long, mmu_valid_gen) \
+ __field(__u64, gfn) \
+ __field(__u32, role) \
+ __field(__u32, root_count) \
__field(bool, unsync)
-#define KVM_MMU_PAGE_ASSIGN(sp) \
- __entry->gfn = sp->gfn; \
- __entry->role = sp->role.word; \
- __entry->root_count = sp->root_count; \
+#define KVM_MMU_PAGE_ASSIGN(sp) \
+ __entry->mmu_valid_gen = sp->mmu_valid_gen; \
+ __entry->gfn = sp->gfn; \
+ __entry->role = sp->role.word; \
+ __entry->root_count = sp->root_count; \
__entry->unsync = sp->unsync;
#define KVM_MMU_PAGE_PRINTK() ({ \
@@ -28,8 +30,8 @@
\
role.word = __entry->role; \
\
- trace_seq_printf(p, "sp gfn %llx %u%s q%u%s %s%s" \
- " %snxe root %u %s%c", \
+ trace_seq_printf(p, "sp gen %lx gfn %llx %u%s q%u%s %s%s" \
+ " %snxe root %u %s%c", __entry->mmu_valid_gen, \
__entry->gfn, role.level, \
role.cr4_pae ? " pae" : "", \
role.quadrant, \
--
1.7.7.6
Move deletion shadow page from the hash list from kvm_mmu_commit_zap_page to
kvm_mmu_prepare_zap_page so that we can call kvm_mmu_commit_zap_page
once for multiple kvm_mmu_prepare_zap_page that can help us to avoid
unnecessary TLB flush
Signed-off-by: Xiao Guangrong <[email protected]>
---
arch/x86/kvm/mmu.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 40d7b2d..682ecb4 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1466,7 +1466,7 @@ static inline void kvm_mod_used_mmu_pages(struct kvm *kvm, int nr)
static void kvm_mmu_free_page(struct kvm_mmu_page *sp)
{
ASSERT(is_empty_shadow_page(sp->spt));
- hlist_del(&sp->hash_link);
+
list_del(&sp->link);
free_page((unsigned long)sp->spt);
if (!sp->role.direct)
@@ -1655,7 +1655,8 @@ static void kvm_mmu_commit_zap_page(struct kvm *kvm,
#define for_each_gfn_indirect_valid_sp(_kvm, _sp, _gfn) \
for_each_gfn_sp(_kvm, _sp, _gfn) \
- if ((_sp)->role.direct || (_sp)->role.invalid) {} else
+ if ((_sp)->role.direct || \
+ ((_sp)->role.invalid && WARN_ON(1))) {} else
/* @sp->gfn should be write-protected at the call site */
static int __kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
@@ -2074,6 +2075,9 @@ static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
unaccount_shadowed(kvm, sp->gfn);
if (sp->unsync)
kvm_unlink_unsync_page(kvm, sp);
+
+ hlist_del_init(&sp->hash_link);
+
if (!sp->root_count) {
/* Count self */
ret++;
--
1.7.7.6
Now, kvm_mmu_zap_all is only called in the path of mmu_notifier->release,
at that time, vcpu has stopped that means no new page will be create, we
can use lock-break technique to avoid potential soft lockup
(Note: at this time, the mmu-lock still has contention between ->release
and other mmu-notify handlers.)
Signed-off-by: Xiao Guangrong <[email protected]>
---
arch/x86/kvm/mmu.c | 11 ++++++++++-
1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index d9343fe..268b2ff 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4197,11 +4197,20 @@ void kvm_mmu_zap_all(struct kvm *kvm)
struct kvm_mmu_page *sp, *node;
LIST_HEAD(invalid_list);
+ might_sleep();
+
spin_lock(&kvm->mmu_lock);
restart:
- list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link)
+ list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
+ if (need_resched() || spin_needbreak(&kvm->mmu_lock)) {
+ kvm_mmu_commit_zap_page(kvm, &invalid_list);
+ cond_resched_lock(&kvm->mmu_lock);
+ goto restart;
+ }
+
if (kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list))
goto restart;
+ }
kvm_mmu_commit_zap_page(kvm, &invalid_list);
spin_unlock(&kvm->mmu_lock);
--
1.7.7.6
Replace kvm_mmu_zap_all by kvm_mmu_invalidate_memslot_pages except on
the path of mmu_notifier->release() which will be fixed in
the later patch
Signed-off-by: Xiao Guangrong <[email protected]>
---
arch/x86/kvm/x86.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d885418..c2dd732 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5528,7 +5528,7 @@ static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
* to ensure that the updated hypercall appears atomically across all
* VCPUs.
*/
- kvm_mmu_zap_all(vcpu->kvm);
+ kvm_mmu_invalidate_memslot_pages(vcpu->kvm, NULL);
kvm_x86_ops->patch_hypercall(vcpu, instruction);
@@ -7079,7 +7079,7 @@ void kvm_arch_flush_shadow_all(struct kvm *kvm)
void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
struct kvm_memory_slot *slot)
{
- kvm_arch_flush_shadow_all(kvm);
+ kvm_mmu_invalidate_memslot_pages(kvm, slot);
}
int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
--
1.7.7.6
It is the responsibility of kvm_mmu_zap_all that keeps the
consistent of mmu and tlbs. And it is also unnecessary after
zap all mmio sptes since no mmio spte exists on root shadow
page and it can not be cached into tlb
Signed-off-by: Xiao Guangrong <[email protected]>
---
arch/x86/kvm/x86.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8d28810..d885418 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7067,16 +7067,13 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
* If memory slot is created, or moved, we need to clear all
* mmio sptes.
*/
- if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) {
+ if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE))
kvm_mmu_zap_mmio_sptes(kvm);
- kvm_reload_remote_mmus(kvm);
- }
}
void kvm_arch_flush_shadow_all(struct kvm *kvm)
{
kvm_mmu_zap_all(kvm);
- kvm_reload_remote_mmus(kvm);
}
void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
--
1.7.7.6
The current kvm_mmu_zap_all is really slow - it is holding mmu-lock to
walk and zap all shadow pages one by one, also it need to zap all guest
page's rmap and all shadow page's parent spte list. Particularly, things
become worse if guest uses more memory or vcpus. It is not good for
scalability
In this patch, we introduce a faster way to invalidate all shadow pages.
KVM maintains a global mmu invalid generation-number which is stored in
kvm->arch.mmu_valid_gen and every shadow page stores the current global
generation-number into sp->mmu_valid_gen when it is created
When KVM need zap all shadow pages sptes, it just simply increase the
global generation-number then reload root shadow pages on all vcpus.
Vcpu will create a new shadow page table according to current kvm's
generation-number. It ensures the old pages are not used any more.
Then the invalid-gen pages (sp->mmu_valid_gen != kvm->arch.mmu_valid_gen)
are zapped by using lock-break technique
Signed-off-by: Xiao Guangrong <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 2 +
arch/x86/kvm/mmu.c | 98 +++++++++++++++++++++++++++++++++++++++
arch/x86/kvm/mmu.h | 2 +
3 files changed, 102 insertions(+), 0 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 3741c65..bff7d46 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -222,6 +222,7 @@ struct kvm_mmu_page {
int root_count; /* Currently serving as active root */
unsigned int unsync_children;
unsigned long parent_ptes; /* Reverse mapping for parent_pte */
+ unsigned long mmu_valid_gen;
DECLARE_BITMAP(unsync_child_bitmap, 512);
#ifdef CONFIG_X86_32
@@ -529,6 +530,7 @@ struct kvm_arch {
unsigned int n_requested_mmu_pages;
unsigned int n_max_mmu_pages;
unsigned int indirect_shadow_pages;
+ unsigned long mmu_valid_gen;
struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
/*
* Hash table of struct kvm_mmu_page.
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 682ecb4..d9343fe 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1839,6 +1839,11 @@ static void clear_sp_write_flooding_count(u64 *spte)
__clear_sp_write_flooding_count(sp);
}
+static bool is_obsolete_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
+{
+ return unlikely(sp->mmu_valid_gen != kvm->arch.mmu_valid_gen);
+}
+
static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
gfn_t gfn,
gva_t gaddr,
@@ -1865,6 +1870,9 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
role.quadrant = quadrant;
}
for_each_gfn_sp(vcpu->kvm, sp, gfn) {
+ if (is_obsolete_sp(vcpu->kvm, sp))
+ continue;
+
if (!need_sync && sp->unsync)
need_sync = true;
@@ -1901,6 +1909,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
account_shadowed(vcpu->kvm, gfn);
}
+ sp->mmu_valid_gen = vcpu->kvm->arch.mmu_valid_gen;
init_shadow_page_table(sp);
trace_kvm_mmu_get_page(sp, true);
return sp;
@@ -2071,8 +2080,10 @@ static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
ret = mmu_zap_unsync_children(kvm, sp, invalid_list);
kvm_mmu_page_unlink_children(kvm, sp);
kvm_mmu_unlink_parents(kvm, sp);
+
if (!sp->role.invalid && !sp->role.direct)
unaccount_shadowed(kvm, sp->gfn);
+
if (sp->unsync)
kvm_unlink_unsync_page(kvm, sp);
@@ -4196,6 +4207,93 @@ restart:
spin_unlock(&kvm->mmu_lock);
}
+static void zap_invalid_pages(struct kvm *kvm)
+{
+ struct kvm_mmu_page *sp, *node;
+ LIST_HEAD(invalid_list);
+
+restart:
+ list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
+ if (!is_obsolete_sp(kvm, sp))
+ continue;
+
+ /*
+ * Do not repeatedly zap a root page to avoid unnecessary
+ * KVM_REQ_MMU_RELOAD, otherwise we may not be able to
+ * progress:
+ * vcpu 0 vcpu 1
+ * call vcpu_enter_guest():
+ * 1): handle KVM_REQ_MMU_RELOAD
+ * and require mmu-lock to
+ * load mmu
+ * repeat:
+ * 1): zap root page and
+ * send KVM_REQ_MMU_RELOAD
+ *
+ * 2): if (cond_resched_lock(mmu-lock))
+ *
+ * 2): hold mmu-lock and load mmu
+ *
+ * 3): see KVM_REQ_MMU_RELOAD bit
+ * on vcpu->requests is set
+ * then return 1 to call
+ * vcpu_enter_guest() again.
+ * goto repeat;
+ *
+ */
+ if (sp->role.invalid)
+ continue;
+ /*
+ * Need not flush tlb since we only zap the sp with invalid
+ * generation number.
+ */
+ if (cond_resched_lock(&kvm->mmu_lock))
+ goto restart;
+
+ if (kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list))
+ goto restart;
+ }
+
+ /*
+ * Should flush tlb before free page tables since lockless-walking
+ * may use the pages.
+ */
+ kvm_mmu_commit_zap_page(kvm, &invalid_list);
+}
+
+/*
+ * Fast invalidate all shadow pages belong to @slot.
+ *
+ * @slot != NULL means the invalidation is caused the memslot specified
+ * by @slot is being deleted, in this case, we should ensure that rmap
+ * and lpage-info of the @slot can not be used after calling the function.
+ *
+ * @slot == NULL means the invalidation due to other reasons, we need
+ * not care rmap and lpage-info since they are still valid after calling
+ * the function.
+ */
+void kvm_mmu_invalidate_memslot_pages(struct kvm *kvm,
+ struct kvm_memory_slot *slot)
+{
+ spin_lock(&kvm->mmu_lock);
+ kvm->arch.mmu_valid_gen++;
+
+ /*
+ * Notify all vcpus to reload its shadow page table
+ * and flush TLB. Then all vcpus will switch to new
+ * shadow page table with the new mmu_valid_gen.
+ *
+ * Note: we should do this under the protection of
+ * mmu-lock, otherwise, vcpu would purge shadow page
+ * but miss tlb flush.
+ */
+ kvm_reload_remote_mmus(kvm);
+
+ if (slot)
+ zap_invalid_pages(kvm);
+ spin_unlock(&kvm->mmu_lock);
+}
+
void kvm_mmu_zap_mmio_sptes(struct kvm *kvm)
{
struct kvm_mmu_page *sp, *node;
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 2adcbc2..bd57466 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -97,4 +97,6 @@ static inline bool permission_fault(struct kvm_mmu *mmu, unsigned pte_access,
return (mmu->permissions[pfec >> 1] >> pte_access) & 1;
}
+void kvm_mmu_invalidate_memslot_pages(struct kvm *kvm,
+ struct kvm_memory_slot *slot);
#endif
--
1.7.7.6
It is good for debug and development
Signed-off-by: Xiao Guangrong <[email protected]>
---
arch/x86/kvm/mmu.c | 2 ++
arch/x86/kvm/mmutrace.h | 23 +++++++++++++++++++++++
2 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 268b2ff..e12f431 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4300,6 +4300,8 @@ void kvm_mmu_invalidate_memslot_pages(struct kvm *kvm,
if (slot)
zap_invalid_pages(kvm);
+
+ trace_kvm_mmu_invalidate_memslot_pages(kvm, slot);
spin_unlock(&kvm->mmu_lock);
}
diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
index 697f466..8ef3e0e 100644
--- a/arch/x86/kvm/mmutrace.h
+++ b/arch/x86/kvm/mmutrace.h
@@ -276,6 +276,29 @@ TRACE_EVENT(
__spte_satisfied(old_spte), __spte_satisfied(new_spte)
)
);
+
+TRACE_EVENT(
+ kvm_mmu_invalidate_memslot_pages,
+ TP_PROTO(struct kvm *kvm, struct kvm_memory_slot *slot),
+ TP_ARGS(kvm, slot),
+
+ TP_STRUCT__entry(
+ __field(unsigned long, mmu_valid_gen)
+ __field(unsigned int, mmu_used_pages)
+ __field(int, slot_id)
+ ),
+
+ TP_fast_assign(
+ __entry->mmu_valid_gen = kvm->arch.mmu_valid_gen;
+ __entry->mmu_used_pages = kvm->arch.n_used_mmu_pages;
+ __entry->slot_id = slot ? slot->id : -1;
+ ),
+
+ TP_printk("kvm-mmu-valid-gen %lx slot_id %d used_pages %x",
+ __entry->mmu_valid_gen, __entry->slot_id,
+ __entry->mmu_used_pages
+ )
+);
#endif /* _TRACE_KVMMMU_H */
#undef TRACE_INCLUDE_PATH
--
1.7.7.6
Attach the benchmark.
On 05/16/2013 08:17 PM, Xiao Guangrong wrote:
> Bechmark result:
> I have tested this patchset and the previous version that only zaps the
> pages linked on invalid slot's rmap. The benchmark is written by myself
> which has been attached, it writes large memory when do pci rom read.
>
> Host: Intel(R) Xeon(R) CPU X5690 @ 3.47GHz + 36G Memory
> Guest: 12 VCPU + 32G Memory
>
> Current code: This patchset Previous Version
> 2405434959 ns 2323016424 ns 2368810003 ns
>
> The interesting thing is, the previous version is slower than this patch,
> i guess the reason is that the former keeps lots of invalid pages in mmu
> which cause shadow page to be reclaimed due to used-pages > request-pages
> or host memory shrink.
>
> Changlog:
> V5:
> 1): rename is_valid_sp to is_obsolete_sp
> 2): use lock-break technique to zap all old pages instead of only pages
> linked on invalid slot's rmap suggested by Marcelo.
> 3): trace invalid pages and kvm_mmu_invalidate_memslot_pages()
> 4): rename kvm_mmu_invalid_memslot_pages to kvm_mmu_invalidate_memslot_pages
> according to Takuya's comments.
>
> V4:
> 1): drop unmapping invalid rmap out of mmu-lock and use lock-break technique
> instead. Thanks to Gleb's comments.
>
> 2): needn't handle invalid-gen pages specially due to page table always
> switched by KVM_REQ_MMU_RELOAD. Thanks to Marcelo's comments.
>
> V3:
> completely redesign the algorithm, please see below.
>
> V2:
> - do not reset n_requested_mmu_pages and n_max_mmu_pages
> - batch free root shadow pages to reduce vcpu notification and mmu-lock
> contention
> - remove the first patch that introduce kvm->arch.mmu_cache since we only
> 'memset zero' on hashtable rather than all mmu cache members in this
> version
> - remove unnecessary kvm_reload_remote_mmus after kvm_mmu_zap_all
>
> * Issue
> The current kvm_mmu_zap_all is really slow - it is holding mmu-lock to
> walk and zap all shadow pages one by one, also it need to zap all guest
> page's rmap and all shadow page's parent spte list. Particularly, things
> become worse if guest uses more memory or vcpus. It is not good for
> scalability.
>
> * Idea
> KVM maintains a global mmu invalid generation-number which is stored in
> kvm->arch.mmu_valid_gen and every shadow page stores the current global
> generation-number into sp->mmu_valid_gen when it is created.
>
> When KVM need zap all shadow pages sptes, it just simply increase the
> global generation-number then reload root shadow pages on all vcpus.
> Vcpu will create a new shadow page table according to current kvm's
> generation-number. It ensures the old pages are not used any more.
>
> Then the invalid-gen pages (sp->mmu_valid_gen != kvm->arch.mmu_valid_gen)
> are zapped by using lock-break technique.
>
> Xiao Guangrong (8):
> KVM: MMU: drop unnecessary kvm_reload_remote_mmus
> KVM: MMU: delete shadow page from hash list in
> kvm_mmu_prepare_zap_page
> KVM: MMU: fast invalidate all pages
> KVM: x86: use the fast way to invalidate all pages
> KVM: MMU: make kvm_mmu_zap_all preemptable
> KVM: MMU: show mmu_valid_gen in shadow page related tracepoints
> KVM: MMU: add tracepoint for kvm_mmu_invalidate_memslot_pages
> KVM: MMU: zap pages in batch
>
> arch/x86/include/asm/kvm_host.h | 2 +
> arch/x86/kvm/mmu.c | 124 ++++++++++++++++++++++++++++++++++++++-
> arch/x86/kvm/mmu.h | 2 +
> arch/x86/kvm/mmutrace.h | 45 +++++++++++---
> arch/x86/kvm/x86.c | 9 +--
> 5 files changed, 163 insertions(+), 19 deletions(-)
>
On Thu, May 16, 2013 at 08:17:48PM +0800, Xiao Guangrong wrote:
> The current kvm_mmu_zap_all is really slow - it is holding mmu-lock to
> walk and zap all shadow pages one by one, also it need to zap all guest
> page's rmap and all shadow page's parent spte list. Particularly, things
> become worse if guest uses more memory or vcpus. It is not good for
> scalability
>
> In this patch, we introduce a faster way to invalidate all shadow pages.
> KVM maintains a global mmu invalid generation-number which is stored in
> kvm->arch.mmu_valid_gen and every shadow page stores the current global
> generation-number into sp->mmu_valid_gen when it is created
>
> When KVM need zap all shadow pages sptes, it just simply increase the
> global generation-number then reload root shadow pages on all vcpus.
> Vcpu will create a new shadow page table according to current kvm's
> generation-number. It ensures the old pages are not used any more.
> Then the invalid-gen pages (sp->mmu_valid_gen != kvm->arch.mmu_valid_gen)
> are zapped by using lock-break technique
>
> Signed-off-by: Xiao Guangrong <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 2 +
> arch/x86/kvm/mmu.c | 98 +++++++++++++++++++++++++++++++++++++++
> arch/x86/kvm/mmu.h | 2 +
> 3 files changed, 102 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 3741c65..bff7d46 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -222,6 +222,7 @@ struct kvm_mmu_page {
> int root_count; /* Currently serving as active root */
> unsigned int unsync_children;
> unsigned long parent_ptes; /* Reverse mapping for parent_pte */
> + unsigned long mmu_valid_gen;
> DECLARE_BITMAP(unsync_child_bitmap, 512);
>
> #ifdef CONFIG_X86_32
> @@ -529,6 +530,7 @@ struct kvm_arch {
> unsigned int n_requested_mmu_pages;
> unsigned int n_max_mmu_pages;
> unsigned int indirect_shadow_pages;
> + unsigned long mmu_valid_gen;
> struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
> /*
> * Hash table of struct kvm_mmu_page.
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 682ecb4..d9343fe 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -1839,6 +1839,11 @@ static void clear_sp_write_flooding_count(u64 *spte)
> __clear_sp_write_flooding_count(sp);
> }
>
> +static bool is_obsolete_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
> +{
> + return unlikely(sp->mmu_valid_gen != kvm->arch.mmu_valid_gen);
> +}
> +
> static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> gfn_t gfn,
> gva_t gaddr,
> @@ -1865,6 +1870,9 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> role.quadrant = quadrant;
> }
> for_each_gfn_sp(vcpu->kvm, sp, gfn) {
> + if (is_obsolete_sp(vcpu->kvm, sp))
> + continue;
> +
> if (!need_sync && sp->unsync)
> need_sync = true;
>
> @@ -1901,6 +1909,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
>
> account_shadowed(vcpu->kvm, gfn);
> }
> + sp->mmu_valid_gen = vcpu->kvm->arch.mmu_valid_gen;
> init_shadow_page_table(sp);
> trace_kvm_mmu_get_page(sp, true);
> return sp;
> @@ -2071,8 +2080,10 @@ static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
> ret = mmu_zap_unsync_children(kvm, sp, invalid_list);
> kvm_mmu_page_unlink_children(kvm, sp);
> kvm_mmu_unlink_parents(kvm, sp);
> +
> if (!sp->role.invalid && !sp->role.direct)
> unaccount_shadowed(kvm, sp->gfn);
> +
> if (sp->unsync)
> kvm_unlink_unsync_page(kvm, sp);
>
> @@ -4196,6 +4207,93 @@ restart:
> spin_unlock(&kvm->mmu_lock);
> }
>
> +static void zap_invalid_pages(struct kvm *kvm)
> +{
> + struct kvm_mmu_page *sp, *node;
> + LIST_HEAD(invalid_list);
> +
> +restart:
> + list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
> + if (!is_obsolete_sp(kvm, sp))
> + continue;
What if we save kvm->arch.active_mmu_pages on the stack and init
kvm->arch.active_mmu_pages to be empty at the entrance to
zap_invalid_pages(). This loop will iterate over saved list. This will
allow us to drop the is_obsolete_sp() check and will save time since we
will not be iterating over newly created sps.
> +
> + /*
> + * Do not repeatedly zap a root page to avoid unnecessary
> + * KVM_REQ_MMU_RELOAD, otherwise we may not be able to
> + * progress:
> + * vcpu 0 vcpu 1
> + * call vcpu_enter_guest():
> + * 1): handle KVM_REQ_MMU_RELOAD
> + * and require mmu-lock to
> + * load mmu
> + * repeat:
> + * 1): zap root page and
> + * send KVM_REQ_MMU_RELOAD
> + *
> + * 2): if (cond_resched_lock(mmu-lock))
> + *
> + * 2): hold mmu-lock and load mmu
> + *
> + * 3): see KVM_REQ_MMU_RELOAD bit
> + * on vcpu->requests is set
> + * then return 1 to call
> + * vcpu_enter_guest() again.
> + * goto repeat;
> + *
> + */
> + if (sp->role.invalid)
> + continue;
> + /*
> + * Need not flush tlb since we only zap the sp with invalid
> + * generation number.
> + */
> + if (cond_resched_lock(&kvm->mmu_lock))
> + goto restart;
> +
> + if (kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list))
> + goto restart;
> + }
> +
> + /*
> + * Should flush tlb before free page tables since lockless-walking
> + * may use the pages.
> + */
> + kvm_mmu_commit_zap_page(kvm, &invalid_list);
> +}
> +
> +/*
> + * Fast invalidate all shadow pages belong to @slot.
> + *
> + * @slot != NULL means the invalidation is caused the memslot specified
> + * by @slot is being deleted, in this case, we should ensure that rmap
> + * and lpage-info of the @slot can not be used after calling the function.
> + *
> + * @slot == NULL means the invalidation due to other reasons, we need
> + * not care rmap and lpage-info since they are still valid after calling
> + * the function.
> + */
> +void kvm_mmu_invalidate_memslot_pages(struct kvm *kvm,
> + struct kvm_memory_slot *slot)
> +{
> + spin_lock(&kvm->mmu_lock);
> + kvm->arch.mmu_valid_gen++;
> +
> + /*
> + * Notify all vcpus to reload its shadow page table
> + * and flush TLB. Then all vcpus will switch to new
> + * shadow page table with the new mmu_valid_gen.
> + *
> + * Note: we should do this under the protection of
> + * mmu-lock, otherwise, vcpu would purge shadow page
> + * but miss tlb flush.
> + */
> + kvm_reload_remote_mmus(kvm);
> +
> + if (slot)
> + zap_invalid_pages(kvm);
> + spin_unlock(&kvm->mmu_lock);
> +}
> +
> void kvm_mmu_zap_mmio_sptes(struct kvm *kvm)
> {
> struct kvm_mmu_page *sp, *node;
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 2adcbc2..bd57466 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -97,4 +97,6 @@ static inline bool permission_fault(struct kvm_mmu *mmu, unsigned pte_access,
> return (mmu->permissions[pfec >> 1] >> pte_access) & 1;
> }
>
> +void kvm_mmu_invalidate_memslot_pages(struct kvm *kvm,
> + struct kvm_memory_slot *slot);
> #endif
> --
> 1.7.7.6
--
Gleb.
Il 16/05/2013 14:17, Xiao Guangrong ha scritto:
> Zap at lease 10 pages before releasing mmu-lock to reduce the overload
> caused by requiring lock
>
> [ It improves kernel building 0.6% ~ 1% ]
>
> Signed-off-by: Xiao Guangrong <[email protected]>
> ---
> arch/x86/kvm/mmu.c | 11 ++++++++---
> 1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index e12f431..9c27fda 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4216,10 +4216,12 @@ restart:
> spin_unlock(&kvm->mmu_lock);
> }
>
> +#define BATCH_ZAP_PAGES 10
> static void zap_invalid_pages(struct kvm *kvm)
> {
> struct kvm_mmu_page *sp, *node;
> LIST_HEAD(invalid_list);
> + int batch = 0;
>
> restart:
> list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
> @@ -4256,11 +4258,14 @@ restart:
> * Need not flush tlb since we only zap the sp with invalid
> * generation number.
> */
> - if (cond_resched_lock(&kvm->mmu_lock))
> + if ((batch >= BATCH_ZAP_PAGES) &&
> + cond_resched_lock(&kvm->mmu_lock)) {
> + batch = 0;
> goto restart;
> + }
>
> - if (kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list))
> - goto restart;
> + batch += kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list);
> + goto restart;
Would this look again and again at the same page if
kvm_mmu_prepare_zap_page returns 0?
Paolo
> }
>
> /*
>
Il 16/05/2013 14:43, Gleb Natapov ha scritto:
>> > +restart:
>> > + list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
>> > + if (!is_obsolete_sp(kvm, sp))
>> > + continue;
> What if we save kvm->arch.active_mmu_pages on the stack and init
> kvm->arch.active_mmu_pages to be empty at the entrance to
> zap_invalid_pages(). This loop will iterate over saved list. This will
> allow us to drop the is_obsolete_sp() check and will save time since we
> will not be iterating over newly created sps.
>
But when you add cond_resched_lock a thread may want to zap pages itself
(e.g. from prepare_zap_oldest_mmu_page) and it won't find them.
Here is another proposal... The idea is to avoid looking at new pages
more than necessary after a "goto restart".
Basically, you alternate between two phases:
- look for pages to be zapped, group them together
- zap the pages
Something like:
moved = 0;
restart:
zapping = true;
for each page in active_mmu_pages [reverse and safe] {
if (!is_obsolete || invalid) {
/*
* Found a new page, stop zapping for now and
* try to segregate the invalid ones at one end
* of the list.
*/
zapping = false;
continue;
}
if (batch > 10 && ...) {
cond_resched_lock
batch = 0;
goto restart;
}
if (!zapping) {
/*
* Segregate pages to one end of the list where
* new pages don't get in the way.
*/
list_move_tail(page, active_mmu_pages)
batch++; /* or maybe not? */
moved++;
} else {
batch += prepare_zap_page
goto restart;
}
}
/* Need another pass to look at segregated pages? */
if (moved) {
moved = 0;
goto restart;
}
On 05/16/2013 08:43 PM, Gleb Natapov wrote:
> On Thu, May 16, 2013 at 08:17:48PM +0800, Xiao Guangrong wrote:
>> The current kvm_mmu_zap_all is really slow - it is holding mmu-lock to
>> walk and zap all shadow pages one by one, also it need to zap all guest
>> page's rmap and all shadow page's parent spte list. Particularly, things
>> become worse if guest uses more memory or vcpus. It is not good for
>> scalability
>>
>> In this patch, we introduce a faster way to invalidate all shadow pages.
>> KVM maintains a global mmu invalid generation-number which is stored in
>> kvm->arch.mmu_valid_gen and every shadow page stores the current global
>> generation-number into sp->mmu_valid_gen when it is created
>>
>> When KVM need zap all shadow pages sptes, it just simply increase the
>> global generation-number then reload root shadow pages on all vcpus.
>> Vcpu will create a new shadow page table according to current kvm's
>> generation-number. It ensures the old pages are not used any more.
>> Then the invalid-gen pages (sp->mmu_valid_gen != kvm->arch.mmu_valid_gen)
>> are zapped by using lock-break technique
>>
>> Signed-off-by: Xiao Guangrong <[email protected]>
>> ---
>> arch/x86/include/asm/kvm_host.h | 2 +
>> arch/x86/kvm/mmu.c | 98 +++++++++++++++++++++++++++++++++++++++
>> arch/x86/kvm/mmu.h | 2 +
>> 3 files changed, 102 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 3741c65..bff7d46 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -222,6 +222,7 @@ struct kvm_mmu_page {
>> int root_count; /* Currently serving as active root */
>> unsigned int unsync_children;
>> unsigned long parent_ptes; /* Reverse mapping for parent_pte */
>> + unsigned long mmu_valid_gen;
>> DECLARE_BITMAP(unsync_child_bitmap, 512);
>>
>> #ifdef CONFIG_X86_32
>> @@ -529,6 +530,7 @@ struct kvm_arch {
>> unsigned int n_requested_mmu_pages;
>> unsigned int n_max_mmu_pages;
>> unsigned int indirect_shadow_pages;
>> + unsigned long mmu_valid_gen;
>> struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
>> /*
>> * Hash table of struct kvm_mmu_page.
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index 682ecb4..d9343fe 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -1839,6 +1839,11 @@ static void clear_sp_write_flooding_count(u64 *spte)
>> __clear_sp_write_flooding_count(sp);
>> }
>>
>> +static bool is_obsolete_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
>> +{
>> + return unlikely(sp->mmu_valid_gen != kvm->arch.mmu_valid_gen);
>> +}
>> +
>> static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
>> gfn_t gfn,
>> gva_t gaddr,
>> @@ -1865,6 +1870,9 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
>> role.quadrant = quadrant;
>> }
>> for_each_gfn_sp(vcpu->kvm, sp, gfn) {
>> + if (is_obsolete_sp(vcpu->kvm, sp))
>> + continue;
>> +
>> if (!need_sync && sp->unsync)
>> need_sync = true;
>>
>> @@ -1901,6 +1909,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
>>
>> account_shadowed(vcpu->kvm, gfn);
>> }
>> + sp->mmu_valid_gen = vcpu->kvm->arch.mmu_valid_gen;
>> init_shadow_page_table(sp);
>> trace_kvm_mmu_get_page(sp, true);
>> return sp;
>> @@ -2071,8 +2080,10 @@ static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
>> ret = mmu_zap_unsync_children(kvm, sp, invalid_list);
>> kvm_mmu_page_unlink_children(kvm, sp);
>> kvm_mmu_unlink_parents(kvm, sp);
>> +
>> if (!sp->role.invalid && !sp->role.direct)
>> unaccount_shadowed(kvm, sp->gfn);
>> +
>> if (sp->unsync)
>> kvm_unlink_unsync_page(kvm, sp);
>>
>> @@ -4196,6 +4207,93 @@ restart:
>> spin_unlock(&kvm->mmu_lock);
>> }
>>
>> +static void zap_invalid_pages(struct kvm *kvm)
>> +{
>> + struct kvm_mmu_page *sp, *node;
>> + LIST_HEAD(invalid_list);
>> +
>> +restart:
>> + list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
>> + if (!is_obsolete_sp(kvm, sp))
>> + continue;
> What if we save kvm->arch.active_mmu_pages on the stack and init
> kvm->arch.active_mmu_pages to be empty at the entrance to
> zap_invalid_pages(). This loop will iterate over saved list. This will
> allow us to drop the is_obsolete_sp() check and will save time since we
> will not be iterating over newly created sps.
This idea is really smart.
It also seems tricky, vcpu can see the page in its page table and hash table but
it has already been deleted from kvm->active_list, but i do not see any issue.
Hmm, can we walk kvm->ative_mmu_pages from tail to head then break the walking
if we meet the sp->valid_gen == kvm->valid_gen? This way also can skip walking
new created sps and more straight.
On 05/16/2013 08:45 PM, Paolo Bonzini wrote:
> Il 16/05/2013 14:17, Xiao Guangrong ha scritto:
>> Zap at lease 10 pages before releasing mmu-lock to reduce the overload
>> caused by requiring lock
>>
>> [ It improves kernel building 0.6% ~ 1% ]
>>
>> Signed-off-by: Xiao Guangrong <[email protected]>
>> ---
>> arch/x86/kvm/mmu.c | 11 ++++++++---
>> 1 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index e12f431..9c27fda 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -4216,10 +4216,12 @@ restart:
>> spin_unlock(&kvm->mmu_lock);
>> }
>>
>> +#define BATCH_ZAP_PAGES 10
>> static void zap_invalid_pages(struct kvm *kvm)
>> {
>> struct kvm_mmu_page *sp, *node;
>> LIST_HEAD(invalid_list);
>> + int batch = 0;
>>
>> restart:
>> list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
>> @@ -4256,11 +4258,14 @@ restart:
>> * Need not flush tlb since we only zap the sp with invalid
>> * generation number.
>> */
>> - if (cond_resched_lock(&kvm->mmu_lock))
>> + if ((batch >= BATCH_ZAP_PAGES) &&
>> + cond_resched_lock(&kvm->mmu_lock)) {
>> + batch = 0;
>> goto restart;
>> + }
>>
>> - if (kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list))
>> - goto restart;
>> + batch += kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list);
>> + goto restart;
>
> Would this look again and again at the same page if
> kvm_mmu_prepare_zap_page returns 0?
We skip the invalid page (sp->role.invalid) before call
kvm_mmu_prepare_zap_page so that kvm_mmu_prepare_zap_page can not
meet the same page. ;)
On Thu, May 16, 2013 at 03:14:35PM +0200, Paolo Bonzini wrote:
> Il 16/05/2013 14:43, Gleb Natapov ha scritto:
> >> > +restart:
> >> > + list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
> >> > + if (!is_obsolete_sp(kvm, sp))
> >> > + continue;
> > What if we save kvm->arch.active_mmu_pages on the stack and init
> > kvm->arch.active_mmu_pages to be empty at the entrance to
> > zap_invalid_pages(). This loop will iterate over saved list. This will
> > allow us to drop the is_obsolete_sp() check and will save time since we
> > will not be iterating over newly created sps.
> >
>
> But when you add cond_resched_lock a thread may want to zap pages itself
> (e.g. from prepare_zap_oldest_mmu_page) and it won't find them.
>
Yes, this will break mmu pages accounting. We can make
prepare_zap_oldest_mmu_page() wait while zap_invalid_pages()
frees needed amount of pages if one is in progress.
> Here is another proposal... The idea is to avoid looking at new pages
> more than necessary after a "goto restart".
>
> Basically, you alternate between two phases:
>
> - look for pages to be zapped, group them together
>
> - zap the pages
>
> Something like:
>
> moved = 0;
> restart:
> zapping = true;
> for each page in active_mmu_pages [reverse and safe] {
> if (!is_obsolete || invalid) {
> /*
> * Found a new page, stop zapping for now and
> * try to segregate the invalid ones at one end
> * of the list.
> */
> zapping = false;
> continue;
> }
>
> if (batch > 10 && ...) {
> cond_resched_lock
> batch = 0;
> goto restart;
> }
>
> if (!zapping) {
> /*
> * Segregate pages to one end of the list where
> * new pages don't get in the way.
> */
> list_move_tail(page, active_mmu_pages)
> batch++; /* or maybe not? */
> moved++;
> } else {
> batch += prepare_zap_page
> goto restart;
> }
> }
>
> /* Need another pass to look at segregated pages? */
> if (moved) {
> moved = 0;
> goto restart;
> }
Not sure what are you trying to achieve with "moved" tricks. Just
walking the list from the end and stopping on first valid sp should be
enough since active_mmu_pages list is a FIFO right now.
--
Gleb.
On Thu, May 16, 2013 at 09:25:28PM +0800, Xiao Guangrong wrote:
> On 05/16/2013 08:43 PM, Gleb Natapov wrote:
> > On Thu, May 16, 2013 at 08:17:48PM +0800, Xiao Guangrong wrote:
> >> The current kvm_mmu_zap_all is really slow - it is holding mmu-lock to
> >> walk and zap all shadow pages one by one, also it need to zap all guest
> >> page's rmap and all shadow page's parent spte list. Particularly, things
> >> become worse if guest uses more memory or vcpus. It is not good for
> >> scalability
> >>
> >> In this patch, we introduce a faster way to invalidate all shadow pages.
> >> KVM maintains a global mmu invalid generation-number which is stored in
> >> kvm->arch.mmu_valid_gen and every shadow page stores the current global
> >> generation-number into sp->mmu_valid_gen when it is created
> >>
> >> When KVM need zap all shadow pages sptes, it just simply increase the
> >> global generation-number then reload root shadow pages on all vcpus.
> >> Vcpu will create a new shadow page table according to current kvm's
> >> generation-number. It ensures the old pages are not used any more.
> >> Then the invalid-gen pages (sp->mmu_valid_gen != kvm->arch.mmu_valid_gen)
> >> are zapped by using lock-break technique
> >>
> >> Signed-off-by: Xiao Guangrong <[email protected]>
> >> ---
> >> arch/x86/include/asm/kvm_host.h | 2 +
> >> arch/x86/kvm/mmu.c | 98 +++++++++++++++++++++++++++++++++++++++
> >> arch/x86/kvm/mmu.h | 2 +
> >> 3 files changed, 102 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> >> index 3741c65..bff7d46 100644
> >> --- a/arch/x86/include/asm/kvm_host.h
> >> +++ b/arch/x86/include/asm/kvm_host.h
> >> @@ -222,6 +222,7 @@ struct kvm_mmu_page {
> >> int root_count; /* Currently serving as active root */
> >> unsigned int unsync_children;
> >> unsigned long parent_ptes; /* Reverse mapping for parent_pte */
> >> + unsigned long mmu_valid_gen;
> >> DECLARE_BITMAP(unsync_child_bitmap, 512);
> >>
> >> #ifdef CONFIG_X86_32
> >> @@ -529,6 +530,7 @@ struct kvm_arch {
> >> unsigned int n_requested_mmu_pages;
> >> unsigned int n_max_mmu_pages;
> >> unsigned int indirect_shadow_pages;
> >> + unsigned long mmu_valid_gen;
> >> struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
> >> /*
> >> * Hash table of struct kvm_mmu_page.
> >> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> >> index 682ecb4..d9343fe 100644
> >> --- a/arch/x86/kvm/mmu.c
> >> +++ b/arch/x86/kvm/mmu.c
> >> @@ -1839,6 +1839,11 @@ static void clear_sp_write_flooding_count(u64 *spte)
> >> __clear_sp_write_flooding_count(sp);
> >> }
> >>
> >> +static bool is_obsolete_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
> >> +{
> >> + return unlikely(sp->mmu_valid_gen != kvm->arch.mmu_valid_gen);
> >> +}
> >> +
> >> static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> >> gfn_t gfn,
> >> gva_t gaddr,
> >> @@ -1865,6 +1870,9 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> >> role.quadrant = quadrant;
> >> }
> >> for_each_gfn_sp(vcpu->kvm, sp, gfn) {
> >> + if (is_obsolete_sp(vcpu->kvm, sp))
> >> + continue;
> >> +
> >> if (!need_sync && sp->unsync)
> >> need_sync = true;
> >>
> >> @@ -1901,6 +1909,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> >>
> >> account_shadowed(vcpu->kvm, gfn);
> >> }
> >> + sp->mmu_valid_gen = vcpu->kvm->arch.mmu_valid_gen;
> >> init_shadow_page_table(sp);
> >> trace_kvm_mmu_get_page(sp, true);
> >> return sp;
> >> @@ -2071,8 +2080,10 @@ static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
> >> ret = mmu_zap_unsync_children(kvm, sp, invalid_list);
> >> kvm_mmu_page_unlink_children(kvm, sp);
> >> kvm_mmu_unlink_parents(kvm, sp);
> >> +
> >> if (!sp->role.invalid && !sp->role.direct)
> >> unaccount_shadowed(kvm, sp->gfn);
> >> +
> >> if (sp->unsync)
> >> kvm_unlink_unsync_page(kvm, sp);
> >>
> >> @@ -4196,6 +4207,93 @@ restart:
> >> spin_unlock(&kvm->mmu_lock);
> >> }
> >>
> >> +static void zap_invalid_pages(struct kvm *kvm)
> >> +{
> >> + struct kvm_mmu_page *sp, *node;
> >> + LIST_HEAD(invalid_list);
> >> +
> >> +restart:
> >> + list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
> >> + if (!is_obsolete_sp(kvm, sp))
> >> + continue;
> > What if we save kvm->arch.active_mmu_pages on the stack and init
> > kvm->arch.active_mmu_pages to be empty at the entrance to
> > zap_invalid_pages(). This loop will iterate over saved list. This will
> > allow us to drop the is_obsolete_sp() check and will save time since we
> > will not be iterating over newly created sps.
>
> This idea is really smart.
>
> It also seems tricky, vcpu can see the page in its page table and hash table but
> it has already been deleted from kvm->active_list, but i do not see any issue.
>
Paolo pointed that it breaks mmu pages accounting. Can be solved, but
not trivial.
> Hmm, can we walk kvm->ative_mmu_pages from tail to head then break the walking
> if we meet the sp->valid_gen == kvm->valid_gen? This way also can skip walking
> new created sps and more straight.
>
Yes, that should be better than walking it from the start each time.
--
Gleb.
Il 16/05/2013 15:41, Gleb Natapov ha scritto:
> On Thu, May 16, 2013 at 03:14:35PM +0200, Paolo Bonzini wrote:
>> Il 16/05/2013 14:43, Gleb Natapov ha scritto:
>>>>> +restart:
>>>>> + list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
>>>>> + if (!is_obsolete_sp(kvm, sp))
>>>>> + continue;
>>> What if we save kvm->arch.active_mmu_pages on the stack and init
>>> kvm->arch.active_mmu_pages to be empty at the entrance to
>>> zap_invalid_pages(). This loop will iterate over saved list. This will
>>> allow us to drop the is_obsolete_sp() check and will save time since we
>>> will not be iterating over newly created sps.
>>>
>>
>> But when you add cond_resched_lock a thread may want to zap pages itself
>> (e.g. from prepare_zap_oldest_mmu_page) and it won't find them.
>>
> Yes, this will break mmu pages accounting. We can make
> prepare_zap_oldest_mmu_page() wait while zap_invalid_pages()
> frees needed amount of pages if one is in progress.
>
>> Here is another proposal... The idea is to avoid looking at new pages
>> more than necessary after a "goto restart".
>>
>> Basically, you alternate between two phases:
>>
>> - look for pages to be zapped, group them together
>>
>> - zap the pages
>>
>> Something like:
>>
>> moved = 0;
>> restart:
>> zapping = true;
>> for each page in active_mmu_pages [reverse and safe] {
>> if (!is_obsolete || invalid) {
>> /*
>> * Found a new page, stop zapping for now and
>> * try to segregate the invalid ones at one end
>> * of the list.
>> */
>> zapping = false;
>> continue;
>> }
>>
>> if (batch > 10 && ...) {
>> cond_resched_lock
>> batch = 0;
>> goto restart;
>> }
>>
>> if (!zapping) {
>> /*
>> * Segregate pages to one end of the list where
>> * new pages don't get in the way.
>> */
>> list_move_tail(page, active_mmu_pages)
>> batch++; /* or maybe not? */
>> moved++;
>> } else {
>> batch += prepare_zap_page
>> goto restart;
>> }
>> }
>>
>> /* Need another pass to look at segregated pages? */
>> if (moved) {
>> moved = 0;
>> goto restart;
>> }
> Not sure what are you trying to achieve with "moved" tricks. Just
> walking the list from the end and stopping on first valid sp should be
> enough since active_mmu_pages list is a FIFO right now.
Right, I missed that "sp->role.invalid = 1" will ensure anyway that
pages are visited at most twice.
Paolo
On Thu, 16 May 2013 20:17:45 +0800
Xiao Guangrong <[email protected]> wrote:
> Bechmark result:
> I have tested this patchset and the previous version that only zaps the
> pages linked on invalid slot's rmap. The benchmark is written by myself
> which has been attached, it writes large memory when do pci rom read.
>
> Host: Intel(R) Xeon(R) CPU X5690 @ 3.47GHz + 36G Memory
> Guest: 12 VCPU + 32G Memory
>
> Current code: This patchset Previous Version
> 2405434959 ns 2323016424 ns 2368810003 ns
>
> The interesting thing is, the previous version is slower than this patch,
> i guess the reason is that the former keeps lots of invalid pages in mmu
> which cause shadow page to be reclaimed due to used-pages > request-pages
> or host memory shrink.
This patch series looks very nice!
Minor issues may still need to be improved, but I really hope to see this
get merged during this cycle.
[for the future] Do you think that postponing some zapping/freeing of
obsolete(already invalidated) pages to make_mmu_pages_available() time
can improve the situation more? -- say, for big guests.
If accounting kept correct, make_mmu_pages_available() only needs to free
some obsolete pages instead of valid pages.
Takuya
>
> Changlog:
> V5:
> 1): rename is_valid_sp to is_obsolete_sp
> 2): use lock-break technique to zap all old pages instead of only pages
> linked on invalid slot's rmap suggested by Marcelo.
> 3): trace invalid pages and kvm_mmu_invalidate_memslot_pages()
> 4): rename kvm_mmu_invalid_memslot_pages to kvm_mmu_invalidate_memslot_pages
> according to Takuya's comments.
>
> V4:
> 1): drop unmapping invalid rmap out of mmu-lock and use lock-break technique
> instead. Thanks to Gleb's comments.
>
> 2): needn't handle invalid-gen pages specially due to page table always
> switched by KVM_REQ_MMU_RELOAD. Thanks to Marcelo's comments.
>
> V3:
> completely redesign the algorithm, please see below.
>
> V2:
> - do not reset n_requested_mmu_pages and n_max_mmu_pages
> - batch free root shadow pages to reduce vcpu notification and mmu-lock
> contention
> - remove the first patch that introduce kvm->arch.mmu_cache since we only
> 'memset zero' on hashtable rather than all mmu cache members in this
> version
> - remove unnecessary kvm_reload_remote_mmus after kvm_mmu_zap_all
>
> * Issue
> The current kvm_mmu_zap_all is really slow - it is holding mmu-lock to
> walk and zap all shadow pages one by one, also it need to zap all guest
> page's rmap and all shadow page's parent spte list. Particularly, things
> become worse if guest uses more memory or vcpus. It is not good for
> scalability.
>
> * Idea
> KVM maintains a global mmu invalid generation-number which is stored in
> kvm->arch.mmu_valid_gen and every shadow page stores the current global
> generation-number into sp->mmu_valid_gen when it is created.
>
> When KVM need zap all shadow pages sptes, it just simply increase the
> global generation-number then reload root shadow pages on all vcpus.
> Vcpu will create a new shadow page table according to current kvm's
> generation-number. It ensures the old pages are not used any more.
>
> Then the invalid-gen pages (sp->mmu_valid_gen != kvm->arch.mmu_valid_gen)
> are zapped by using lock-break technique.
>
> Xiao Guangrong (8):
> KVM: MMU: drop unnecessary kvm_reload_remote_mmus
> KVM: MMU: delete shadow page from hash list in
> kvm_mmu_prepare_zap_page
> KVM: MMU: fast invalidate all pages
> KVM: x86: use the fast way to invalidate all pages
> KVM: MMU: make kvm_mmu_zap_all preemptable
> KVM: MMU: show mmu_valid_gen in shadow page related tracepoints
> KVM: MMU: add tracepoint for kvm_mmu_invalidate_memslot_pages
> KVM: MMU: zap pages in batch
>
> arch/x86/include/asm/kvm_host.h | 2 +
> arch/x86/kvm/mmu.c | 124 ++++++++++++++++++++++++++++++++++++++-
> arch/x86/kvm/mmu.h | 2 +
> arch/x86/kvm/mmutrace.h | 45 +++++++++++---
> arch/x86/kvm/x86.c | 9 +--
> 5 files changed, 163 insertions(+), 19 deletions(-)
>
> --
> 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
--
Takuya Yoshikawa <[email protected]>
On Thu, May 16, 2013 at 04:43:21PM +0300, Gleb Natapov wrote:
> On Thu, May 16, 2013 at 09:25:28PM +0800, Xiao Guangrong wrote:
> > On 05/16/2013 08:43 PM, Gleb Natapov wrote:
> > > On Thu, May 16, 2013 at 08:17:48PM +0800, Xiao Guangrong wrote:
> > >> The current kvm_mmu_zap_all is really slow - it is holding mmu-lock to
> > >> walk and zap all shadow pages one by one, also it need to zap all guest
> > >> page's rmap and all shadow page's parent spte list. Particularly, things
> > >> become worse if guest uses more memory or vcpus. It is not good for
> > >> scalability
> > >>
> > >> In this patch, we introduce a faster way to invalidate all shadow pages.
> > >> KVM maintains a global mmu invalid generation-number which is stored in
> > >> kvm->arch.mmu_valid_gen and every shadow page stores the current global
> > >> generation-number into sp->mmu_valid_gen when it is created
> > >>
> > >> When KVM need zap all shadow pages sptes, it just simply increase the
> > >> global generation-number then reload root shadow pages on all vcpus.
> > >> Vcpu will create a new shadow page table according to current kvm's
> > >> generation-number. It ensures the old pages are not used any more.
> > >> Then the invalid-gen pages (sp->mmu_valid_gen != kvm->arch.mmu_valid_gen)
> > >> are zapped by using lock-break technique
> > >>
> > >> Signed-off-by: Xiao Guangrong <[email protected]>
> > >> ---
> > >> arch/x86/include/asm/kvm_host.h | 2 +
> > >> arch/x86/kvm/mmu.c | 98 +++++++++++++++++++++++++++++++++++++++
> > >> arch/x86/kvm/mmu.h | 2 +
> > >> 3 files changed, 102 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > >> index 3741c65..bff7d46 100644
> > >> --- a/arch/x86/include/asm/kvm_host.h
> > >> +++ b/arch/x86/include/asm/kvm_host.h
> > >> @@ -222,6 +222,7 @@ struct kvm_mmu_page {
> > >> int root_count; /* Currently serving as active root */
> > >> unsigned int unsync_children;
> > >> unsigned long parent_ptes; /* Reverse mapping for parent_pte */
> > >> + unsigned long mmu_valid_gen;
> > >> DECLARE_BITMAP(unsync_child_bitmap, 512);
> > >>
> > >> #ifdef CONFIG_X86_32
> > >> @@ -529,6 +530,7 @@ struct kvm_arch {
> > >> unsigned int n_requested_mmu_pages;
> > >> unsigned int n_max_mmu_pages;
> > >> unsigned int indirect_shadow_pages;
> > >> + unsigned long mmu_valid_gen;
> > >> struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
> > >> /*
> > >> * Hash table of struct kvm_mmu_page.
> > >> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > >> index 682ecb4..d9343fe 100644
> > >> --- a/arch/x86/kvm/mmu.c
> > >> +++ b/arch/x86/kvm/mmu.c
> > >> @@ -1839,6 +1839,11 @@ static void clear_sp_write_flooding_count(u64 *spte)
> > >> __clear_sp_write_flooding_count(sp);
> > >> }
> > >>
> > >> +static bool is_obsolete_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
> > >> +{
> > >> + return unlikely(sp->mmu_valid_gen != kvm->arch.mmu_valid_gen);
> > >> +}
> > >> +
> > >> static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> > >> gfn_t gfn,
> > >> gva_t gaddr,
> > >> @@ -1865,6 +1870,9 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> > >> role.quadrant = quadrant;
> > >> }
> > >> for_each_gfn_sp(vcpu->kvm, sp, gfn) {
> > >> + if (is_obsolete_sp(vcpu->kvm, sp))
> > >> + continue;
> > >> +
> > >> if (!need_sync && sp->unsync)
> > >> need_sync = true;
> > >>
> > >> @@ -1901,6 +1909,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> > >>
> > >> account_shadowed(vcpu->kvm, gfn);
> > >> }
> > >> + sp->mmu_valid_gen = vcpu->kvm->arch.mmu_valid_gen;
> > >> init_shadow_page_table(sp);
> > >> trace_kvm_mmu_get_page(sp, true);
> > >> return sp;
> > >> @@ -2071,8 +2080,10 @@ static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
> > >> ret = mmu_zap_unsync_children(kvm, sp, invalid_list);
> > >> kvm_mmu_page_unlink_children(kvm, sp);
> > >> kvm_mmu_unlink_parents(kvm, sp);
> > >> +
> > >> if (!sp->role.invalid && !sp->role.direct)
> > >> unaccount_shadowed(kvm, sp->gfn);
> > >> +
> > >> if (sp->unsync)
> > >> kvm_unlink_unsync_page(kvm, sp);
> > >>
> > >> @@ -4196,6 +4207,93 @@ restart:
> > >> spin_unlock(&kvm->mmu_lock);
> > >> }
> > >>
> > >> +static void zap_invalid_pages(struct kvm *kvm)
> > >> +{
> > >> + struct kvm_mmu_page *sp, *node;
> > >> + LIST_HEAD(invalid_list);
> > >> +
> > >> +restart:
> > >> + list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
> > >> + if (!is_obsolete_sp(kvm, sp))
> > >> + continue;
> > > What if we save kvm->arch.active_mmu_pages on the stack and init
> > > kvm->arch.active_mmu_pages to be empty at the entrance to
> > > zap_invalid_pages(). This loop will iterate over saved list. This will
> > > allow us to drop the is_obsolete_sp() check and will save time since we
> > > will not be iterating over newly created sps.
> >
> > This idea is really smart.
> >
> > It also seems tricky, vcpu can see the page in its page table and hash table but
> > it has already been deleted from kvm->active_list, but i do not see any issue.
> >
> Paolo pointed that it breaks mmu pages accounting. Can be solved, but
> not trivial.
>
> > Hmm, can we walk kvm->ative_mmu_pages from tail to head then break the walking
> > if we meet the sp->valid_gen == kvm->valid_gen? This way also can skip walking
> > new created sps and more straight.
> >
> Yes, that should be better than walking it from the start each time.
>
One more thought. With current patch if zap_invalid_page() will be
called second time while another zap_invalid_page() is still running
(can that happen?) they will both run concurrently fighting for the
mmu_lock. Is this a problem?
--
Gleb.
On Thu, May 16, 2013 at 08:17:48PM +0800, Xiao Guangrong wrote:
> The current kvm_mmu_zap_all is really slow - it is holding mmu-lock to
> walk and zap all shadow pages one by one, also it need to zap all guest
> page's rmap and all shadow page's parent spte list. Particularly, things
> become worse if guest uses more memory or vcpus. It is not good for
> scalability
>
> In this patch, we introduce a faster way to invalidate all shadow pages.
> KVM maintains a global mmu invalid generation-number which is stored in
> kvm->arch.mmu_valid_gen and every shadow page stores the current global
> generation-number into sp->mmu_valid_gen when it is created
>
> When KVM need zap all shadow pages sptes, it just simply increase the
> global generation-number then reload root shadow pages on all vcpus.
> Vcpu will create a new shadow page table according to current kvm's
> generation-number. It ensures the old pages are not used any more.
> Then the invalid-gen pages (sp->mmu_valid_gen != kvm->arch.mmu_valid_gen)
> are zapped by using lock-break technique
>
> Signed-off-by: Xiao Guangrong <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 2 +
> arch/x86/kvm/mmu.c | 98 +++++++++++++++++++++++++++++++++++++++
> arch/x86/kvm/mmu.h | 2 +
> 3 files changed, 102 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 3741c65..bff7d46 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -222,6 +222,7 @@ struct kvm_mmu_page {
> int root_count; /* Currently serving as active root */
> unsigned int unsync_children;
> unsigned long parent_ptes; /* Reverse mapping for parent_pte */
> + unsigned long mmu_valid_gen;
> DECLARE_BITMAP(unsync_child_bitmap, 512);
>
> #ifdef CONFIG_X86_32
> @@ -529,6 +530,7 @@ struct kvm_arch {
> unsigned int n_requested_mmu_pages;
> unsigned int n_max_mmu_pages;
> unsigned int indirect_shadow_pages;
> + unsigned long mmu_valid_gen;
> struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
> /*
> * Hash table of struct kvm_mmu_page.
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 682ecb4..d9343fe 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -1839,6 +1839,11 @@ static void clear_sp_write_flooding_count(u64 *spte)
> __clear_sp_write_flooding_count(sp);
> }
>
> +static bool is_obsolete_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
> +{
> + return unlikely(sp->mmu_valid_gen != kvm->arch.mmu_valid_gen);
> +}
> +
> static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> gfn_t gfn,
> gva_t gaddr,
> @@ -1865,6 +1870,9 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> role.quadrant = quadrant;
> }
> for_each_gfn_sp(vcpu->kvm, sp, gfn) {
> + if (is_obsolete_sp(vcpu->kvm, sp))
> + continue;
> +
> if (!need_sync && sp->unsync)
> need_sync = true;
>
> @@ -1901,6 +1909,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
>
> account_shadowed(vcpu->kvm, gfn);
> }
> + sp->mmu_valid_gen = vcpu->kvm->arch.mmu_valid_gen;
> init_shadow_page_table(sp);
> trace_kvm_mmu_get_page(sp, true);
> return sp;
> @@ -2071,8 +2080,10 @@ static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
> ret = mmu_zap_unsync_children(kvm, sp, invalid_list);
> kvm_mmu_page_unlink_children(kvm, sp);
> kvm_mmu_unlink_parents(kvm, sp);
> +
> if (!sp->role.invalid && !sp->role.direct)
> unaccount_shadowed(kvm, sp->gfn);
> +
> if (sp->unsync)
> kvm_unlink_unsync_page(kvm, sp);
>
> @@ -4196,6 +4207,93 @@ restart:
> spin_unlock(&kvm->mmu_lock);
> }
>
> +static void zap_invalid_pages(struct kvm *kvm)
> +{
> + struct kvm_mmu_page *sp, *node;
> + LIST_HEAD(invalid_list);
> +
> +restart:
> + list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
> + if (!is_obsolete_sp(kvm, sp))
> + continue;
> +
> + /*
> + * Do not repeatedly zap a root page to avoid unnecessary
> + * KVM_REQ_MMU_RELOAD, otherwise we may not be able to
> + * progress:
> + * vcpu 0 vcpu 1
> + * call vcpu_enter_guest():
> + * 1): handle KVM_REQ_MMU_RELOAD
> + * and require mmu-lock to
> + * load mmu
> + * repeat:
> + * 1): zap root page and
> + * send KVM_REQ_MMU_RELOAD
> + *
> + * 2): if (cond_resched_lock(mmu-lock))
> + *
> + * 2): hold mmu-lock and load mmu
> + *
> + * 3): see KVM_REQ_MMU_RELOAD bit
> + * on vcpu->requests is set
> + * then return 1 to call
> + * vcpu_enter_guest() again.
> + * goto repeat;
> + *
> + */
> + if (sp->role.invalid)
> + continue;
> + /*
> + * Need not flush tlb since we only zap the sp with invalid
> + * generation number.
> + */
> + if (cond_resched_lock(&kvm->mmu_lock))
> + goto restart;
> +
> + if (kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list))
> + goto restart;
> + }
> +
> + /*
> + * Should flush tlb before free page tables since lockless-walking
> + * may use the pages.
> + */
> + kvm_mmu_commit_zap_page(kvm, &invalid_list);
> +}
> +
> +/*
> + * Fast invalidate all shadow pages belong to @slot.
> + *
> + * @slot != NULL means the invalidation is caused the memslot specified
> + * by @slot is being deleted, in this case, we should ensure that rmap
> + * and lpage-info of the @slot can not be used after calling the function.
> + *
> + * @slot == NULL means the invalidation due to other reasons, we need
> + * not care rmap and lpage-info since they are still valid after calling
> + * the function.
> + */
> +void kvm_mmu_invalidate_memslot_pages(struct kvm *kvm,
> + struct kvm_memory_slot *slot)
Why pass "slot" here? If we want the function to sometimes wait for purge
and sometimes not the more straightforward way is to have a "bool wait"
parameter instead.
> +{
> + spin_lock(&kvm->mmu_lock);
> + kvm->arch.mmu_valid_gen++;
> +
> + /*
> + * Notify all vcpus to reload its shadow page table
> + * and flush TLB. Then all vcpus will switch to new
> + * shadow page table with the new mmu_valid_gen.
> + *
> + * Note: we should do this under the protection of
> + * mmu-lock, otherwise, vcpu would purge shadow page
> + * but miss tlb flush.
> + */
> + kvm_reload_remote_mmus(kvm);
> +
> + if (slot)
> + zap_invalid_pages(kvm);
> + spin_unlock(&kvm->mmu_lock);
> +}
> +
> void kvm_mmu_zap_mmio_sptes(struct kvm *kvm)
> {
> struct kvm_mmu_page *sp, *node;
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 2adcbc2..bd57466 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -97,4 +97,6 @@ static inline bool permission_fault(struct kvm_mmu *mmu, unsigned pte_access,
> return (mmu->permissions[pfec >> 1] >> pte_access) & 1;
> }
>
> +void kvm_mmu_invalidate_memslot_pages(struct kvm *kvm,
> + struct kvm_memory_slot *slot);
> #endif
> --
> 1.7.7.6
--
Gleb.
On Thu, May 16, 2013 at 08:17:49PM +0800, Xiao Guangrong wrote:
> Replace kvm_mmu_zap_all by kvm_mmu_invalidate_memslot_pages except on
> the path of mmu_notifier->release() which will be fixed in
> the later patch
>
Why ->release() cannot use kvm_mmu_invalidate_memslot_pages()?
> Signed-off-by: Xiao Guangrong <[email protected]>
> ---
> arch/x86/kvm/x86.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d885418..c2dd732 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5528,7 +5528,7 @@ static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
> * to ensure that the updated hypercall appears atomically across all
> * VCPUs.
> */
> - kvm_mmu_zap_all(vcpu->kvm);
> + kvm_mmu_invalidate_memslot_pages(vcpu->kvm, NULL);
>
> kvm_x86_ops->patch_hypercall(vcpu, instruction);
>
> @@ -7079,7 +7079,7 @@ void kvm_arch_flush_shadow_all(struct kvm *kvm)
> void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
> struct kvm_memory_slot *slot)
> {
> - kvm_arch_flush_shadow_all(kvm);
> + kvm_mmu_invalidate_memslot_pages(kvm, slot);
> }
>
> int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
> --
> 1.7.7.6
--
Gleb.
On 05/16/2013 10:36 PM, Takuya Yoshikawa wrote:
> On Thu, 16 May 2013 20:17:45 +0800
> Xiao Guangrong <[email protected]> wrote:
>
>> Bechmark result:
>> I have tested this patchset and the previous version that only zaps the
>> pages linked on invalid slot's rmap. The benchmark is written by myself
>> which has been attached, it writes large memory when do pci rom read.
>>
>> Host: Intel(R) Xeon(R) CPU X5690 @ 3.47GHz + 36G Memory
>> Guest: 12 VCPU + 32G Memory
>>
>> Current code: This patchset Previous Version
>> 2405434959 ns 2323016424 ns 2368810003 ns
>>
>> The interesting thing is, the previous version is slower than this patch,
>> i guess the reason is that the former keeps lots of invalid pages in mmu
>> which cause shadow page to be reclaimed due to used-pages > request-pages
>> or host memory shrink.
>
> This patch series looks very nice!
Thank you, Takuya!
>
> Minor issues may still need to be improved, but I really hope to see this
> get merged during this cycle.
>
> [for the future] Do you think that postponing some zapping/freeing of
> obsolete(already invalidated) pages to make_mmu_pages_available() time
> can improve the situation more? -- say, for big guests.
Yes, i think it can. :)
We have made many efforts on this but still lack a straight way to
achieve it.
>
> If accounting kept correct, make_mmu_pages_available() only needs to free
> some obsolete pages instead of valid pages.
>
Yes.
On 05/16/2013 11:57 PM, Gleb Natapov wrote:
> One more thought. With current patch if zap_invalid_page() will be
> called second time while another zap_invalid_page() is still running
> (can that happen?) they will both run concurrently fighting for the
Currently, it can not happen since zap_invalid_page is needed when slot
is being deleted which protected by slot-lock.
But we allow it to be concurrent as you commented: we can use it in
->release() instead of calling kvm_mmu_zap_all(), in that case, multiple
call zap_invalid_page() can happen.
> mmu_lock. Is this a problem?
Are you worry about that it can not progress due to lock contention when
walking active_list? Zapping at least 10 pages before releasing the lock
should ensure that it can progress.
Do you see any potential issue?
On 05/17/2013 12:18 AM, Gleb Natapov wrote:
>> +
>> +/*
>> + * Fast invalidate all shadow pages belong to @slot.
>> + *
>> + * @slot != NULL means the invalidation is caused the memslot specified
>> + * by @slot is being deleted, in this case, we should ensure that rmap
>> + * and lpage-info of the @slot can not be used after calling the function.
>> + *
>> + * @slot == NULL means the invalidation due to other reasons, we need
>> + * not care rmap and lpage-info since they are still valid after calling
>> + * the function.
>> + */
>> +void kvm_mmu_invalidate_memslot_pages(struct kvm *kvm,
>> + struct kvm_memory_slot *slot)
>
> Why pass "slot" here? If we want the function to sometimes wait for purge
> and sometimes not the more straightforward way is to have a "bool wait"
> parameter instead.
>
That's my fault, i forgot to update it. Will use 'bool zap_invalid_pages'
instead.
On 05/17/2013 12:19 AM, Gleb Natapov wrote:
> On Thu, May 16, 2013 at 08:17:49PM +0800, Xiao Guangrong wrote:
>> Replace kvm_mmu_zap_all by kvm_mmu_invalidate_memslot_pages except on
>> the path of mmu_notifier->release() which will be fixed in
>> the later patch
>>
> Why ->release() cannot use kvm_mmu_invalidate_memslot_pages()?
Good eyes.
Forgot to update this part after applying new approach of
kvm_mmu_invalidate_memslot_pages(). Will update it.
On Fri, May 17, 2013 at 02:39:07AM +0800, Xiao Guangrong wrote:
> On 05/16/2013 11:57 PM, Gleb Natapov wrote:
>
> > One more thought. With current patch if zap_invalid_page() will be
> > called second time while another zap_invalid_page() is still running
> > (can that happen?) they will both run concurrently fighting for the
>
> Currently, it can not happen since zap_invalid_page is needed when slot
> is being deleted which protected by slot-lock.
>
> But we allow it to be concurrent as you commented: we can use it in
> ->release() instead of calling kvm_mmu_zap_all(), in that case, multiple
> call zap_invalid_page() can happen.
>
That's only during VM destruction, no need to optimize for.
> > mmu_lock. Is this a problem?
>
> Are you worry about that it can not progress due to lock contention when
> walking active_list? Zapping at least 10 pages before releasing the lock
> should ensure that it can progress.
Yes, it will progress, but will bounce between two threads after each 10
pages. This is less efficient that letting one thread to finish zapping.
Theoretically we can make one thread wait for the other.
>
> Do you see any potential issue?
>
Just thinking out loud :). If this cannot happen during normal
operation, no reason to optimize for it.
--
Gleb.