MIME-Version: 1.0
References: <20230306224127.1689967-1-vipinsh@google.com> <20230306224127.1689967-6-vipinsh@google.com>
 <20230309175849.0000565e@gmail.com> <CAHVum0fhU2PAQEerG5t92R1ropoh1-ML4Yv1CzwGThRtbbvWHg@mail.gmail.com>
 <ZAp0W9eH6XbWhiap@google.com>
In-Reply-To: <ZAp0W9eH6XbWhiap@google.com>
From:   David Matlack <dmatlack@google.com>
Date:   Thu, 9 Mar 2023 16:06:59 -0800
Message-ID: <CALzav=cYb1a1fNWUHvR9AoN_+KTthoJE4Gfg_gn4tz-vrAORNQ@mail.gmail.com>
Subject: Re: [Patch v4 05/18] KVM: x86/mmu: Add split_shadow_page_cache pages
 to global count of MMU cache pages
To:     Vipin Sharma <vipinsh@google.com>
Cc:     Zhi Wang <zhi.wang.linux@gmail.com>, seanjc@google.com,
        pbonzini@redhat.com, bgardon@google.com, jmattson@google.com,
        mizhang@google.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Mar 9, 2023 at 4:05=E2=80=AFPM David Matlack <dmatlack@google.com> =
wrote:
>
> On Thu, Mar 09, 2023 at 11:59:00AM -0800, Vipin Sharma wrote:
> > On Thu, Mar 9, 2023 at 7:58=E2=80=AFAM Zhi Wang <zhi.wang.linux@gmail.c=
om> wrote:
> > >
> > > On Mon,  6 Mar 2023 14:41:14 -0800
> > > Vipin Sharma <vipinsh@google.com> wrote:
> > >
> > > > Add pages in split_shadow_page_cache to the global counter
> > > > kvm_total_unused_cached_pages. These pages will be freed by MMU shr=
inker
> > > > in future commit.
> > > >
> > > > Signed-off-by: Vipin Sharma <vipinsh@google.com>
> > > > ---
> > > >  arch/x86/kvm/mmu/mmu.c | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > > > index df8dcb7e5de7..0ebb8a2eaf47 100644
> > > > --- a/arch/x86/kvm/mmu/mmu.c
> > > > +++ b/arch/x86/kvm/mmu/mmu.c
> > > > @@ -6149,7 +6149,9 @@ static void mmu_free_vm_memory_caches(struct =
kvm *kvm)
> > > >  {
> > > >       kvm_mmu_free_memory_cache(&kvm->arch.split_desc_cache);
> > > >       kvm_mmu_free_memory_cache(&kvm->arch.split_page_header_cache)=
;
> > > > -     kvm_mmu_free_memory_cache(&kvm->arch.split_shadow_page_cache)=
;
> > > > +     mutex_lock(&kvm->slots_lock);
> > > > +     mmu_free_sp_memory_cache(&kvm->arch.split_shadow_page_cache);
> > > > +     mutex_unlock(&kvm->slots_lock);
> > >
> > > Taking the lock of the calling path in the layer of cache topping/fre=
e layer
> > > seems off.
> > >
> > > My vote goes to have a lock for each cache and take the lock of the c=
ache when
> > > topping/free the cache. It is more self-contained and architecturally=
 nice.
> > >
> >
> > Yeah, this can be one way. However, in future patches when I am adding
> > per NUMA node cache, it will add up a lot of locks for the same code
> > path before a topup. In split huge page case we know what NUMA node we
> > need to allocate from so we can fine tune which lock to take but  in
> > fault path code we don't know what NUMA node the page will be coming
> > from so we need to topup all of the NUMA caches. Having a single lock
> > simplifies code a little bit.
> >
> > I agree with you on being more self-contained. I will wait for others
> > to also chime in on this and go from there.
>
> As a general rule, please only added locking when it's needed. Adding
> the lock in this commit is just confusing.
>
> But that aside, I don't think acquiring the slots lock is even needed in
> this commit.

Correction: even needed in the *next* commit

> mmu_free_vm_memory_caches() is never called while the the
> VM is on vm_list. i.e. This can never race with the shrinker.
>
> If you want to be paranoid you can add a WARN to ensure that stays true
> going forward:
>
>   /* ... comment ... */
>   WARN_ON_ONCE(!list_empty(&kvm->vm_list));