| Message ID | 20210331210841.3996155-8-bgardon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More parallel operations for the TDP MMU | expand |
On Wed, Mar 31, 2021, Ben Gardon wrote: > In order to parallelize more operations for the TDP MMU, make the > refcount on TDP MMU roots atomic, so that a future patch can allow > multiple threads to take a reference on the root concurrently, while > holding the MMU lock in read mode. > > Signed-off-by: Ben Gardon <bgardon@google.com> > --- ... > @@ -88,10 +88,12 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm, > next_root = list_first_entry(&kvm->arch.tdp_mmu_roots, > typeof(*next_root), link); > > + while (!list_entry_is_head(next_root, &kvm->arch.tdp_mmu_roots, link) && > + !kvm_tdp_mmu_get_root(kvm, next_root)) > + next_root = list_next_entry(next_root, link); > + > if (list_entry_is_head(next_root, &kvm->arch.tdp_mmu_roots, link)) > next_root = NULL; > - else > - kvm_tdp_mmu_get_root(kvm, next_root); > > if (prev_root) > kvm_tdp_mmu_put_root(kvm, prev_root); > @@ -158,14 +160,13 @@ hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu) > > /* Check for an existing root before allocating a new one. */ > for_each_tdp_mmu_root(kvm, root) { > - if (root->role.word == role.word) { > - kvm_tdp_mmu_get_root(kvm, root); > + if (root->role.word == role.word && > + kvm_tdp_mmu_get_root(kvm, root)) I'm not opposed to changing this logic while making the refcount atomic, but it needs to be explained in the changelog. As is, the changelog makes it sound like the patch is a pure refactoring of the type.
On Wed, Mar 31, 2021 at 3:22 PM Sean Christopherson <seanjc@google.com> wrote: > > On Wed, Mar 31, 2021, Ben Gardon wrote: > > In order to parallelize more operations for the TDP MMU, make the > > refcount on TDP MMU roots atomic, so that a future patch can allow > > multiple threads to take a reference on the root concurrently, while > > holding the MMU lock in read mode. > > > > Signed-off-by: Ben Gardon <bgardon@google.com> > > --- > > ... > > > @@ -88,10 +88,12 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm, > > next_root = list_first_entry(&kvm->arch.tdp_mmu_roots, > > typeof(*next_root), link); > > > > + while (!list_entry_is_head(next_root, &kvm->arch.tdp_mmu_roots, link) && > > + !kvm_tdp_mmu_get_root(kvm, next_root)) > > + next_root = list_next_entry(next_root, link); > > + > > if (list_entry_is_head(next_root, &kvm->arch.tdp_mmu_roots, link)) > > next_root = NULL; > > - else > > - kvm_tdp_mmu_get_root(kvm, next_root); > > > > if (prev_root) > > kvm_tdp_mmu_put_root(kvm, prev_root); > > @@ -158,14 +160,13 @@ hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu) > > > > /* Check for an existing root before allocating a new one. */ > > for_each_tdp_mmu_root(kvm, root) { > > - if (root->role.word == role.word) { > > - kvm_tdp_mmu_get_root(kvm, root); > > + if (root->role.word == role.word && > > + kvm_tdp_mmu_get_root(kvm, root)) > > I'm not opposed to changing this logic while making the refcount atomic, but it > needs to be explained in the changelog. As is, the changelog makes it sound > like the patch is a pure refactoring of the type. Thanks for pointing that out. I'll add a note in the description in v2. Those felt like natural changes since the introduction of the atomic requires additional failure handling. I don't think there's any way to add it as a separate commit without just introducing dead code, but that would certainly be preferable.
diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h index 788dcf77c957..0a040d6a4f35 100644 --- a/arch/x86/kvm/mmu/mmu_internal.h +++ b/arch/x86/kvm/mmu/mmu_internal.h @@ -50,7 +50,11 @@ struct kvm_mmu_page { u64 *spt; /* hold the gfn of each spte inside spt */ gfn_t *gfns; - int root_count; /* Currently serving as active root */ + /* Currently serving as active root */ + union { + int root_count; + refcount_t tdp_mmu_root_count; + }; unsigned int unsync_children; struct kvm_rmap_head parent_ptes; /* rmap pointers to parent sptes */ DECLARE_BITMAP(unsync_child_bitmap, 512); diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index ab1d26b40164..1f0b2d6124a2 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -56,7 +56,7 @@ void kvm_tdp_mmu_put_root(struct kvm *kvm, struct kvm_mmu_page *root) lockdep_assert_held_write(&kvm->mmu_lock); - if (--root->root_count) + if (!refcount_dec_and_test(&root->tdp_mmu_root_count)) return; WARN_ON(!root->tdp_mmu_page); @@ -88,10 +88,12 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm, next_root = list_first_entry(&kvm->arch.tdp_mmu_roots, typeof(*next_root), link); + while (!list_entry_is_head(next_root, &kvm->arch.tdp_mmu_roots, link) && + !kvm_tdp_mmu_get_root(kvm, next_root)) + next_root = list_next_entry(next_root, link); + if (list_entry_is_head(next_root, &kvm->arch.tdp_mmu_roots, link)) next_root = NULL; - else - kvm_tdp_mmu_get_root(kvm, next_root); if (prev_root) kvm_tdp_mmu_put_root(kvm, prev_root); @@ -158,14 +160,13 @@ hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu) /* Check for an existing root before allocating a new one. */ for_each_tdp_mmu_root(kvm, root) { - if (root->role.word == role.word) { - kvm_tdp_mmu_get_root(kvm, root); + if (root->role.word == role.word && + kvm_tdp_mmu_get_root(kvm, root)) goto out; - } } root = alloc_tdp_mmu_page(vcpu, 0, vcpu->arch.mmu->shadow_root_level); - root->root_count = 1; + refcount_set(&root->tdp_mmu_root_count, 1); list_add(&root->link, &kvm->arch.tdp_mmu_roots); diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h index 5d950e987fc7..9961df505067 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.h +++ b/arch/x86/kvm/mmu/tdp_mmu.h @@ -7,13 +7,10 @@ hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu); -static inline void kvm_tdp_mmu_get_root(struct kvm *kvm, - struct kvm_mmu_page *root) +__must_check static inline bool kvm_tdp_mmu_get_root(struct kvm *kvm, + struct kvm_mmu_page *root) { - BUG_ON(!root->root_count); - lockdep_assert_held(&kvm->mmu_lock); - - ++root->root_count; + return refcount_inc_not_zero(&root->tdp_mmu_root_count); } void kvm_tdp_mmu_put_root(struct kvm *kvm, struct kvm_mmu_page *root);
In order to parallelize more operations for the TDP MMU, make the refcount on TDP MMU roots atomic, so that a future patch can allow multiple threads to take a reference on the root concurrently, while holding the MMU lock in read mode. Signed-off-by: Ben Gardon <bgardon@google.com> --- arch/x86/kvm/mmu/mmu_internal.h | 6 +++++- arch/x86/kvm/mmu/tdp_mmu.c | 15 ++++++++------- arch/x86/kvm/mmu/tdp_mmu.h | 9 +++------ 3 files changed, 16 insertions(+), 14 deletions(-)