Re: [PATCH 07/13] KVM: x86/mmu: Make TDP MMU root refcount atomic

[Ben Gardon]

On Wed, Mar 31, 2021 at 3:22 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
>
> ...
>
> > @@ -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.