Re: [PATCH 3/6] KVM MMU: optimize/cleanup for marking parent unsync
On Wed, Apr 14, 2010 at 11:23:38AM +0800, Xiao Guangrong wrote:
Marcelo Tosatti wrote:
I'd prefer to not touch it.
This patch avoids walk all parents and i think this overload is really
unnecessary.
It has other tricks in this codepath but i not noticed? :-)
My point is that there is no point in optimizing something unless its
performance sensitive.
Hi Marcelo,
I think optimizing not only means 'performance' but also means 'smaller
code'(maybe 'cleanup'
is more suitable) and 'logic optimize'(do little things), i'm not sure this
patch whether can
improve system performance obviously but it optimize the code logic and
reduce code size, and
it not harm other code and system performance, right? :-)
Right, but this walking code already is compact and stable. Removing the
unused code variables/definitions is fine, but i'd prefer to not change
the logic just for the sake of code reduction.
Actually, the origin code has a bug, the code segment in mmu_parent_walk():
| if (!sp-multimapped sp-parent_pte) {
| ..
| return;
| }
| hlist_for_each_entry(pte_chain, node, sp-parent_ptes, link)
| for (i = 0; i NR_PTE_CHAIN_ENTRIES; ++i) {
| ..
| }
So, if sp-parent_pte == NULL, it's unsafe...
And as i recall, mmu_unsync_walk was much more
sensitive performance wise than parent walking. Actually, gfn_to_memslot
seems more important since its also noticeable on EPT/NPT hosts.
Yeah, i also noticed these and i'm looking into these code.
Great.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] KVM MMU: optimize/cleanup for marking parent unsync
On Tue, Apr 13, 2010 at 09:53:07AM +0800, Xiao Guangrong wrote: Marcelo Tosatti wrote: Xiao, Did you actually see this codepath as being performance sensitive? Actually, i not run benchmarks to contrast the performance before this patch and after this patch. I'd prefer to not touch it. This patch avoids walk all parents and i think this overload is really unnecessary. It has other tricks in this codepath but i not noticed? :-) My point is that there is no point in optimizing something unless its performance sensitive. And as i recall, mmu_unsync_walk was much more sensitive performance wise than parent walking. Actually, gfn_to_memslot seems more important since its also noticeable on EPT/NPT hosts. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] KVM MMU: optimize/cleanup for marking parent unsync
Marcelo Tosatti wrote:
I'd prefer to not touch it.
This patch avoids walk all parents and i think this overload is really
unnecessary.
It has other tricks in this codepath but i not noticed? :-)
My point is that there is no point in optimizing something unless its
performance sensitive.
Hi Marcelo,
I think optimizing not only means 'performance' but also means 'smaller
code'(maybe 'cleanup'
is more suitable) and 'logic optimize'(do little things), i'm not sure this
patch whether can
improve system performance obviously but it optimize the code logic and reduce
code size, and
it not harm other code and system performance, right? :-)
Actually, the origin code has a bug, the code segment in mmu_parent_walk():
| if (!sp-multimapped sp-parent_pte) {
| ..
| return;
| }
| hlist_for_each_entry(pte_chain, node, sp-parent_ptes, link)
| for (i = 0; i NR_PTE_CHAIN_ENTRIES; ++i) {
| ..
| }
So, if sp-parent_pte == NULL, it's unsafe...
And as i recall, mmu_unsync_walk was much more
sensitive performance wise than parent walking. Actually, gfn_to_memslot
seems more important since its also noticeable on EPT/NPT hosts.
Yeah, i also noticed these and i'm looking into these code.
Thanks,
Xiao
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/6] KVM MMU: optimize/cleanup for marking parent unsync
- 'vcpu' is not used while mark parent unsync, so remove it
- if it has alread marked unsync, no need to walk it's parent
Signed-off-by: Xiao Guangrong xiaoguangr...@cn.fujitsu.com
---
arch/x86/kvm/mmu.c | 69 +--
1 files changed, 23 insertions(+), 46 deletions(-)
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 8f4f781..5154d70 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -172,7 +172,7 @@ struct kvm_shadow_walk_iterator {
shadow_walk_okay((_walker)); \
shadow_walk_next((_walker)))
-typedef int (*mmu_parent_walk_fn) (struct kvm_vcpu *vcpu, struct kvm_mmu_page
*sp);
+typedef int (*mmu_parent_walk_fn) (struct kvm_mmu_page *sp, u64 *spte);
static struct kmem_cache *pte_chain_cache;
static struct kmem_cache *rmap_desc_cache;
@@ -1000,74 +1000,51 @@ static void mmu_page_remove_parent_pte(struct
kvm_mmu_page *sp,
}
-static void mmu_parent_walk(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
- mmu_parent_walk_fn fn)
+static void mmu_parent_walk(struct kvm_mmu_page *sp, mmu_parent_walk_fn fn)
{
struct kvm_pte_chain *pte_chain;
struct hlist_node *node;
struct kvm_mmu_page *parent_sp;
int i;
- if (!sp-multimapped sp-parent_pte) {
+ if (!sp-parent_pte)
+ return;
+
+ if (!sp-multimapped) {
parent_sp = page_header(__pa(sp-parent_pte));
- fn(vcpu, parent_sp);
- mmu_parent_walk(vcpu, parent_sp, fn);
+ if (fn(parent_sp, sp-parent_pte))
+ mmu_parent_walk(parent_sp, fn);
return;
}
+
hlist_for_each_entry(pte_chain, node, sp-parent_ptes, link)
for (i = 0; i NR_PTE_CHAIN_ENTRIES; ++i) {
- if (!pte_chain-parent_ptes[i])
+ u64 *spte = pte_chain-parent_ptes[i];
+ if (!spte)
break;
- parent_sp =
page_header(__pa(pte_chain-parent_ptes[i]));
- fn(vcpu, parent_sp);
- mmu_parent_walk(vcpu, parent_sp, fn);
+ parent_sp = page_header(__pa(spte));
+ if (fn(parent_sp, spte))
+ mmu_parent_walk(parent_sp, fn);
}
}
-static void kvm_mmu_update_unsync_bitmap(u64 *spte)
+static int mark_unsync(struct kvm_mmu_page *sp, u64 *spte)
{
unsigned int index;
- struct kvm_mmu_page *sp = page_header(__pa(spte));
index = spte - sp-spt;
- if (!__test_and_set_bit(index, sp-unsync_child_bitmap))
- sp-unsync_children++;
- WARN_ON(!sp-unsync_children);
-}
-
-static void kvm_mmu_update_parents_unsync(struct kvm_mmu_page *sp)
-{
- struct kvm_pte_chain *pte_chain;
- struct hlist_node *node;
- int i;
-
- if (!sp-parent_pte)
- return;
-
- if (!sp-multimapped) {
- kvm_mmu_update_unsync_bitmap(sp-parent_pte);
- return;
- }
+ if (__test_and_set_bit(index, sp-unsync_child_bitmap))
+ return 0;
- hlist_for_each_entry(pte_chain, node, sp-parent_ptes, link)
- for (i = 0; i NR_PTE_CHAIN_ENTRIES; ++i) {
- if (!pte_chain-parent_ptes[i])
- break;
- kvm_mmu_update_unsync_bitmap(pte_chain-parent_ptes[i]);
- }
-}
+ if (sp-unsync_children++)
+ return 0;
-static int unsync_walk_fn(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
-{
- kvm_mmu_update_parents_unsync(sp);
return 1;
}
-static void kvm_mmu_mark_parents_unsync(struct kvm_vcpu *vcpu,
- struct kvm_mmu_page *sp)
+static void kvm_mmu_mark_parents_unsync(struct kvm_mmu_page *sp)
{
- mmu_parent_walk(vcpu, sp, unsync_walk_fn);
- kvm_mmu_update_parents_unsync(sp);
+ mmu_parent_walk(sp, mark_unsync);
}
static void nonpaging_prefetch_page(struct kvm_vcpu *vcpu,
@@ -1344,7 +1321,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct
kvm_vcpu *vcpu,
mmu_page_add_parent_pte(vcpu, sp, parent_pte);
if (sp-unsync_children) {
set_bit(KVM_REQ_MMU_SYNC, vcpu-requests);
- kvm_mmu_mark_parents_unsync(vcpu, sp);
+ kvm_mmu_mark_parents_unsync(sp);
}
trace_kvm_mmu_get_page(sp, false);
return sp;
@@ -1756,7 +1733,7 @@ static int kvm_unsync_page(struct kvm_vcpu *vcpu, struct
kvm_mmu_page *sp)
++vcpu-kvm-stat.mmu_unsync;
sp-unsync = 1;
- kvm_mmu_mark_parents_unsync(vcpu, sp);
+ kvm_mmu_mark_parents_unsync(sp);
mmu_convert_notrap(sp);
Re: [PATCH 3/6] KVM MMU: optimize/cleanup for marking parent unsync
On 04/12/2010 11:02 AM, Xiao Guangrong wrote: - 'vcpu' is not used while mark parent unsync, so remove it - if it has alread marked unsync, no need to walk it's parent Please separate these two changes. The optimization looks good. Perhaps it can be done even nicer using mutually recursive functions (mmu_parent_walk calls mmu_parent_walk_fn which calls mmu_parent_walk on the parent), but let's not change too many things at a time. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] KVM MMU: optimize/cleanup for marking parent unsync
Avi Kivity wrote: On 04/12/2010 11:02 AM, Xiao Guangrong wrote: - 'vcpu' is not used while mark parent unsync, so remove it - if it has alread marked unsync, no need to walk it's parent Please separate these two changes. The optimization looks good. Perhaps it can be done even nicer using mutually recursive functions (mmu_parent_walk calls mmu_parent_walk_fn which calls mmu_parent_walk on the parent), but let's not change too many things at a time. OK, i'll separate it in the next version Thanks, Xiao -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] KVM MMU: optimize/cleanup for marking parent unsync
On Mon, Apr 12, 2010 at 04:02:24PM +0800, Xiao Guangrong wrote: - 'vcpu' is not used while mark parent unsync, so remove it - if it has alread marked unsync, no need to walk it's parent Signed-off-by: Xiao Guangrong xiaoguangr...@cn.fujitsu.com Xiao, Did you actually see this codepath as being performance sensitive? I'd prefer to not touch it. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] KVM MMU: optimize/cleanup for marking parent unsync
Marcelo Tosatti wrote: Xiao, Did you actually see this codepath as being performance sensitive? Actually, i not run benchmarks to contrast the performance before this patch and after this patch. I'd prefer to not touch it. This patch avoids walk all parents and i think this overload is really unnecessary. It has other tricks in this codepath but i not noticed? :-) Thanks, Xiao -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
