[07/13] KVM: x86/mmu: Make TDP MMU root refcount atomic
diff mbox series

Message ID 20210331210841.3996155-8-bgardon@google.com
State New, archived
Headers show
Series
  • More parallel operations for the TDP MMU
Related show

Commit Message

Ben Gardon March 31, 2021, 9:08 p.m. UTC
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(-)

Comments

Sean Christopherson March 31, 2021, 10:21 p.m. UTC | #1
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.
Ben Gardon April 1, 2021, 4:50 p.m. UTC | #2
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.

Patch
diff mbox series

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);