On Fri, May 08, 2020 at 11:24:25AM -0700, Jon Cargille wrote: > From: Peter Feiner <[email protected]> > > Optimization for avoiding lookups in mmu_page_hash. When there's a > single direct root, a shadow page has at most one parent SPTE > (non-root SPs have exactly one; the root has none). Thus, if an SPTE > is non-present, it can be linked to a newly allocated SP without > first checking if the SP already exists.
Some mechanical comments below. I'll think through the actual logic next week, my brain needs to be primed anytime the MMU is involved :-) > This optimization has proven significant in batch large SP shattering > where the hash lookup accounted for 95% of the overhead. > > Signed-off-by: Peter Feiner <[email protected]> > Signed-off-by: Jon Cargille <[email protected]> > Reviewed-by: Jim Mattson <[email protected]> > > --- > arch/x86/include/asm/kvm_host.h | 13 ++++++++ > arch/x86/kvm/mmu/mmu.c | 55 +++++++++++++++++++-------------- > 2 files changed, 45 insertions(+), 23 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index a239a297be33..9b70d764b626 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -913,6 +913,19 @@ struct kvm_arch { > struct kvm_page_track_notifier_node mmu_sp_tracker; > struct kvm_page_track_notifier_head track_notifier_head; > > + /* > + * Optimization for avoiding lookups in mmu_page_hash. When there's a > + * single direct root, a shadow page has at most one parent SPTE > + * (non-root SPs have exactly one; the root has none). Thus, if an SPTE > + * is non-present, it can be linked to a newly allocated SP without > + * first checking if the SP already exists. > + * > + * False initially because there are no indirect roots. > + * > + * Guarded by mmu_lock. > + */ > + bool shadow_page_may_have_multiple_parents; Why make this a one-way bool? Wouldn't it be better to let this transition back to '0' once all nested guests go away? And maybe a shorter name that reflects what it tracks instead of how its used, e.g. has_indirect_mmu or indirect_mmu_count. > + > struct list_head assigned_dev_head; > struct iommu_domain *iommu_domain; > bool iommu_noncoherent; > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index e618472c572b..d94552b0ed77 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -2499,35 +2499,40 @@ 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_valid_sp(vcpu->kvm, sp, gfn) { > - if (sp->gfn != gfn) { > - collisions++; > - continue; > - } > > - if (!need_sync && sp->unsync) > - need_sync = true; > + if (vcpu->kvm->arch.shadow_page_may_have_multiple_parents || > + level == vcpu->arch.mmu->root_level) { Might be worth a goto to preserve the for-loop. > + for_each_valid_sp(vcpu->kvm, sp, gfn) { > + if (sp->gfn != gfn) { > + collisions++; > + continue; > + } > > - if (sp->role.word != role.word) > - continue; > + if (!need_sync && sp->unsync) > + need_sync = true; > > - if (sp->unsync) { > - /* The page is good, but __kvm_sync_page might still end > - * up zapping it. If so, break in order to rebuild it. > - */ > - if (!__kvm_sync_page(vcpu, sp, &invalid_list)) > - break; > + if (sp->role.word != role.word) > + continue; > > - WARN_ON(!list_empty(&invalid_list)); > - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); > - } > + if (sp->unsync) { > + /* The page is good, but __kvm_sync_page might > + * still end up zapping it. If so, break in > + * order to rebuild it. > + */ > + if (!__kvm_sync_page(vcpu, sp, &invalid_list)) > + break; > > - if (sp->unsync_children) > - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); > + WARN_ON(!list_empty(&invalid_list)); > + kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, > vcpu); > + } > > - __clear_sp_write_flooding_count(sp); > - trace_kvm_mmu_get_page(sp, false); > - goto out; > + if (sp->unsync_children) > + kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, > vcpu); > + > + __clear_sp_write_flooding_count(sp); > + trace_kvm_mmu_get_page(sp, false); > + goto out; > + } > } > > ++vcpu->kvm->stat.mmu_cache_miss; > @@ -3735,6 +3740,10 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu > *vcpu) > gfn_t root_gfn, root_pgd; > int i; > > + spin_lock(&vcpu->kvm->mmu_lock); > + vcpu->kvm->arch.shadow_page_may_have_multiple_parents = true; > + spin_unlock(&vcpu->kvm->mmu_lock); Taking the lock every time is unnecessary, even if this is changed to a refcount type variable, e.g. if (!has_indirect_mmu) { lock_and_set } or if (atomic_inc_return(&indirect_mmu_count) == 1) lock_and_unlock; > + > root_pgd = vcpu->arch.mmu->get_guest_pgd(vcpu); > root_gfn = root_pgd >> PAGE_SHIFT; > > -- > 2.26.2.303.gf8c07b1a785-goog >
