KVM MMU: optimize/cleanup for marking parent unsync
in [Kernel]
|
From: Marcelo Tosatti on 13 Apr 2010 11:20 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 linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Xiao Guangrong on 13 Apr 2010 23:30 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 linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Xiao Guangrong on 14 Apr 2010 00:10 Xiao Guangrong wrote: > > 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... Marcelo, please ignore this, it not a bug, just my mistake, sorry... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Marcelo Tosatti on 14 Apr 2010 13:00 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 linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
First
|
Prev
|
Pages: 1 2 Prev: KVM MMU: fix kvm_mmu_zap_page() and its calling path Next: Socket Direct Protocol: help |