| Message ID | 1482184705-127401-1-git-send-email-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> - for_each_gfn_valid_sp(vcpu->kvm, sp, gfn) { > + for_each_valid_sp(vcpu->kvm, sp, gfn) { > + if (sp->gfn != gfn) { > + collisions++; > + continue; > + } > + > if (!need_sync && sp->unsync) > need_sync = true; > > @@ -2152,8 +2159,8 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); > > __clear_sp_write_flooding_count(sp); > - trace_kvm_mmu_get_page(sp, false); > - return sp; > + created = false; Simply doing if (collisions > vcpu->kvm->stat.max_mmu_page_hash_collisions) vcpu->kvm->stat.max_mmu_page_hash_collisions = collisions; at this point saves a local variable, a jump label and keeps this patch minimal. > + goto out; > } > > ++vcpu->kvm->stat.mmu_cache_miss; > @@ -2164,6 +2171,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > sp->role = role; > hlist_add_head(&sp->hash_link, > &vcpu->kvm->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)]); > + unrelated change. > if (!direct) { > /* > * we should do write protection before syncing pages > @@ -2180,9 +2188,12 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > } > sp->mmu_valid_gen = vcpu->kvm->arch.mmu_valid_gen; > clear_page(sp->spt); > - trace_kvm_mmu_get_page(sp, true); > > kvm_mmu_flush_or_zap(vcpu, &invalid_list, false, flush); > +out: > + if (collisions > vcpu->kvm->stat.max_mmu_page_hash_collisions) > + vcpu->kvm->stat.max_mmu_page_hash_collisions = collisions; > + trace_kvm_mmu_get_page(sp, created); > return sp; > }
On 20/12/2016 10:48, David Hildenbrand wrote: >> >> __clear_sp_write_flooding_count(sp); >> - trace_kvm_mmu_get_page(sp, false); >> - return sp; >> + created = false; > > Simply doing > > if (collisions > vcpu->kvm->stat.max_mmu_page_hash_collisions) > vcpu->kvm->stat.max_mmu_page_hash_collisions = collisions; > > at this point saves a local variable, a jump label and keeps this patch > minimal. How so? There is a "break" above. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 20.12.2016 um 10:55 schrieb Paolo Bonzini: > > > On 20/12/2016 10:48, David Hildenbrand wrote: >>> >>> __clear_sp_write_flooding_count(sp); >>> - trace_kvm_mmu_get_page(sp, false); >>> - return sp; >>> + created = false; >> >> Simply doing >> >> if (collisions > vcpu->kvm->stat.max_mmu_page_hash_collisions) >> vcpu->kvm->stat.max_mmu_page_hash_collisions = collisions; >> >> at this point saves a local variable, a jump label and keeps this patch >> minimal. > > How so? There is a "break" above. > Keeping the same at the end of the function / (even better directly after the for loop) of course. > Paolo >
On 20/12/2016 10:58, David Hildenbrand wrote: > Am 20.12.2016 um 10:55 schrieb Paolo Bonzini: >> >> >> On 20/12/2016 10:48, David Hildenbrand wrote: >>>> >>>> __clear_sp_write_flooding_count(sp); >>>> - trace_kvm_mmu_get_page(sp, false); >>>> - return sp; >>>> + created = false; >>> >>> Simply doing >>> >>> if (collisions > vcpu->kvm->stat.max_mmu_page_hash_collisions) >>> vcpu->kvm->stat.max_mmu_page_hash_collisions = collisions; >>> >>> at this point saves a local variable, a jump label and keeps this patch >>> minimal. >> >> How so? There is a "break" above. >> > > Keeping the same at the end of the function / (even better directly > after the for loop) of course. Then the question is whether a goto is better or worse than code duplication. Here I don't see the point in introducing the created bool and factoring the call to trace_kvm_mmu_get_page, but on the other hand a single "out" label makes sense. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Then the question is whether a goto is better or worse than code > duplication. Here I don't see the point in introducing the created bool > and factoring the call to trace_kvm_mmu_get_page, but on the other hand > a single "out" label makes sense. > Agreed.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 2e25038..8ba0d64 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -815,6 +815,7 @@ struct kvm_vm_stat { ulong mmu_unsync; ulong remote_tlb_flush; ulong lpages; + ulong max_mmu_page_hash_collisions; }; struct kvm_vcpu_stat { diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index 7012de4..58995fd9 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -1904,17 +1904,17 @@ static void kvm_mmu_commit_zap_page(struct kvm *kvm, * since it has been deleted from active_mmu_pages but still can be found * at hast list. * - * for_each_gfn_valid_sp() has skipped that kind of pages. + * for_each_valid_sp() has skipped that kind of pages. */ -#define for_each_gfn_valid_sp(_kvm, _sp, _gfn) \ +#define for_each_valid_sp(_kvm, _sp, _gfn) \ hlist_for_each_entry(_sp, \ &(_kvm)->arch.mmu_page_hash[kvm_page_table_hashfn(_gfn)], hash_link) \ - if ((_sp)->gfn != (_gfn) || is_obsolete_sp((_kvm), (_sp)) \ - || (_sp)->role.invalid) {} else + if (is_obsolete_sp((_kvm), (_sp)) || (_sp)->role.invalid) { \ + } else #define for_each_gfn_indirect_valid_sp(_kvm, _sp, _gfn) \ - for_each_gfn_valid_sp(_kvm, _sp, _gfn) \ - if ((_sp)->role.direct) {} else + for_each_valid_sp(_kvm, _sp, _gfn) \ + if ((_sp)->gfn != (_gfn) || (_sp)->role.direct) {} else /* @sp->gfn should be write-protected at the call site */ static bool __kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, @@ -2116,6 +2116,8 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp; bool need_sync = false; bool flush = false; + int collisions = 0; + bool created = true; LIST_HEAD(invalid_list); role = vcpu->arch.mmu.base_role; @@ -2130,7 +2132,12 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, quadrant &= (1 << ((PT32_PT_BITS - PT64_PT_BITS) * level)) - 1; role.quadrant = quadrant; } - for_each_gfn_valid_sp(vcpu->kvm, sp, gfn) { + for_each_valid_sp(vcpu->kvm, sp, gfn) { + if (sp->gfn != gfn) { + collisions++; + continue; + } + if (!need_sync && sp->unsync) need_sync = true; @@ -2152,8 +2159,8 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); __clear_sp_write_flooding_count(sp); - trace_kvm_mmu_get_page(sp, false); - return sp; + created = false; + goto out; } ++vcpu->kvm->stat.mmu_cache_miss; @@ -2164,6 +2171,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, sp->role = role; hlist_add_head(&sp->hash_link, &vcpu->kvm->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)]); + if (!direct) { /* * we should do write protection before syncing pages @@ -2180,9 +2188,12 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, } sp->mmu_valid_gen = vcpu->kvm->arch.mmu_valid_gen; clear_page(sp->spt); - trace_kvm_mmu_get_page(sp, true); kvm_mmu_flush_or_zap(vcpu, &invalid_list, false, flush); +out: + if (collisions > vcpu->kvm->stat.max_mmu_page_hash_collisions) + vcpu->kvm->stat.max_mmu_page_hash_collisions = collisions; + trace_kvm_mmu_get_page(sp, created); return sp; } diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 8f86c0c..ee4c35e 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -190,6 +190,8 @@ struct kvm_stats_debugfs_item debugfs_entries[] = { { "mmu_unsync", VM_STAT(mmu_unsync) }, { "remote_tlb_flush", VM_STAT(remote_tlb_flush) }, { "largepages", VM_STAT(lpages) }, + { "max_mmu_page_hash_collisions", + VM_STAT(max_mmu_page_hash_collisions) }, { NULL } };
Report the maximum number of mmu_page_hash collisions as a per-VM stat. This will make it easy to identify problems with the mmu_page_hash in the future. Signed-off-by: David Matlack <dmatlack@google.com> Change-Id: I096fc6d5a3589e7f19fcc9c2a1b8a37c7368ba17 --- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/kvm/mmu.c | 31 +++++++++++++++++++++---------- arch/x86/kvm/x86.c | 2 ++ 3 files changed, 24 insertions(+), 10 deletions(-)