Message-ID: <ZymUeC3WiIRH_Jqs@google.com>
Date: Mon, 4 Nov 2024 19:43:52 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Vipin Sharma <vipinsh@...gle.com>
Cc: pbonzini@...hat.com, dmatlack@...gle.com, zhi.wang.linux@...il.com, 
	weijiang.yang@...el.com, mizhang@...gle.com, liangchen.linux@...il.com, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/1] KVM: x86/mmu: Remove KVM mmu shrinker

On Fri, Nov 01, 2024, Vipin Sharma wrote:
> On Fri, Nov 1, 2024 at 1:14 PM Vipin Sharma <vipinsh@...gle.com> wrote:
> >
> > Remove KVM MMU shrinker and all its related code. Remove global
> > kvm_total_used_mmu_pages and page zapping flow from MMU shrinker.

Don't provide a play-by-play of the code changes.  Simply stating "all its
related code" is sufficient.  From that, readers know the *intent* of the patch,
and that allows reviewers to double check that all code is indeed removed.
Stating what the patch literally does verbatim adds a lot of noise and very little
value.

> > Remove zapped_obsolete_pages list from struct kvm_arch{} and use local
> > list in kvm_zap_obsolete_pages() since MMU shrinker is not using it
> > anymore.
> >
> > Current flow of KVM MMU shrinker is very disruptive to VMs.

Please use full sentences.

> > It picks the first VM in the vm_list, zaps the oldest page which is most
> > likely an upper level SPTEs and most like to be reused. Prior to TDP MMU,
> > this is even more disruptive in nested VMs case, considering L1 SPTEs will
> > be the oldest even though most of the entries are for L2 SPTEs.

This flaw isn't limited to the shrinker though, it's inherent to all of KVM's
force page table reclamation.

> > As discussed in [1] shrinker logic has not be very useful in actually
> > keeping VMs performant

I don't think anyone has ever claimed that the shrinker would be useful in
providing performance for VMs.  AFAICT, it's always been about memory usage, and
nothing more.

> > and reducing memory usage.

This one I definitely agree on :-)

> > There was an alternative suggested [2] to repurpose shrinker for
> > shrinking vCPU caches. But considering that in all of the KVM MMU
> > shrinker history it hasn't been used/needed/complained, and there has
> > not been any conversation regarding KVM using lots of page tables, it
> > might be better to just not have shrinker.

A complaint about KVM's page table usage would be an argument for keeping (and
improving) the current shrinker implementation, not for dropping the per-vCPU
caches.  And _that_ to me leads to the the real argument for not wiring up the
shrinker to the per-vCPU caches: it doesn't scale.  E.g. a VM with 4 vCPUs and
4 TiB of memory will, at most, reclaim a laugable 640KiB (4*40*4KiB) of memory.
That's obviously more than a bit contrived, but IMO it really shows that targeting
the per-vCPU caches is unlikely to be useful in practice.  At best, it would be
a premature memory optimization.

> > If the need arise [2] can be revisited.

Everything can be revisited, I think what's important here is to state why forcing
future developers to (re)start from scratch is a non-issue.

> >
> > [1] https://lore.kernel.org/lkml/Y45dldZnI6OIf+a5@google.com/
> > [2] https://lore.kernel.org/kvm/20241004195540.210396-3-vipinsh@google.com/
> >
> > Suggested-by: Sean Christopherson <seanjc@...gle.com>
> > Suggested-by: David Matlack <dmatlack@...gle.com>
> > Reviewed-by: David Matlack <dmatlack@...gle.com>
> 
> FYI, I carried forward David's Reviewed-by from the previous versions.
> Extra change from the previous version is removing registration of KVM
> MMU shrinker in kvm_mmu_vendor_module_init() and mmu_shrinker object
> along with its callback functions.

Heh, I'm going to drop David's review, because I am going to split this into two
patches when applying.  One to yank out the shrinker, and one to use a local list
in kvm_zap_obsolete_pages() and drop zapped_obsolete_pages.

Somewhat subtly, the only reason dropping zapped_obsolete_pages doesn't introduce
a functional change is because kvm_zap_obsolete_pages() is called under slots_lock,
and doesn't drop said lock when yielding.  I.e. there can't be multiple writers
(or readers) of zapped_obsolete_pages, and so the list is guaranteed to be empty
on entry and exit.  That's worth a changelog and bisection point of its own.

With the patch split in two, this is what I ended up with for the main changelog.

Please speak up if you want to change anything!

    KVM: x86/mmu: Remove KVM's MMU shrinker
    
    Remove KVM's MMU shrinker and (almost) all of its related code, as the
    current implementation is very disruptive to VMs (if it ever runs),
    without providing any meaningful benefit[1].
    
    Alternatively, KVM could repurpose its shrinker, e.g. to reclaim pages
    from the per-vCPU caches[2], but given that no one has complained about
    lack of TDP MMU support for the shrinker in the 3+ years since the TDP MMU
    was enabled by default, it's safe to say that there is likely no real use
    case for initiating reclaim of KVM's page tables from the shrinker.
    
    And while clever/cute, reclaiming the per-vCPU caches doesn't scale the
    same way that reclaiming in-use page table pages does.  E.g. the amount of
    memory being used by a VM doesn't always directly correlate with the
    number vCPUs, and even when it does, reclaiming a few pages from per-vCPU
    caches likely won't make much of a dent in the VM's total memory usage,
    especially for VMs with huge amounts of memory.
    
    Lastly, if it turns out that there is a strong use case for dropping the
    per-vCPU caches, re-introducing the shrinker registration is trivial
    compared to the complexity of actually reclaiming pages from the caches.
    
    [1] https://lore.kernel.org/lkml/Y45dldZnI6OIf+a5@google.com
    [2] https://lore.kernel.org/kvm/20241004195540.210396-3-vipinsh@google.com

Powered by blists - more mailing lists