Re: [PATCH RESEND] KVM: MMU: Fix mmu_shrink() so that it can free mmu pages as intended

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Aug 14, 2012 at 09:06:51AM +0900, Takuya Yoshikawa wrote:
> On Mon, 13 Aug 2012 19:15:23 -0300
> Marcelo Tosatti <mtosatti@redhat.com> wrote:
> 
> > On Fri, Aug 10, 2012 at 05:16:12PM +0900, Takuya Yoshikawa wrote:
> > > The following commit changed mmu_shrink() so that it would skip VMs
> > > whose n_used_mmu_pages was not zero and try to free pages from others:
> > > 
> > >   commit 1952639665e92481c34c34c3e2a71bf3e66ba362
> > >   KVM: MMU: do not iterate over all VMs in mmu_shrink()
> > > 
> > > This patch fixes the function so that it can free mmu pages as before.
> > > Note that "if (!nr_to_scan--)" check is removed since we do not try to
> > > free mmu pages from more than one VM.
> > > 
> > > Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> > > Cc: Gleb Natapov <gleb@redhat.com>
> > > ---
> > >  This patch just recovers the original behaviour and is not related
> > >  to how to improve mmu_shrink() further; so please apply.
> > 
> > Before 1952639665e92481c34 the code was maxed at nr_to_scan loops. So
> > removing if (!nr_to_scan--) patch does change behaviour.
> > 
> > Am i missing something here?
> 
> No.  You are right about that.
> 
> But as Gleb and I confirmed when I first sent this patch, the possiblity
> that we see "n_used_mmu_pages == 0" 128 times is quite low that it is
> almost impossible to see the effect.
> 
> If you prefer to have the check, I will do so.

"kvm->arch.n_used_mmu_pages == 0" is an unlikely scenario, agree
with that.

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 9651c2c..4aeec72 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4154,11 +4154,8 @@ static int mmu_shrink(struct shrinker *shrink, struct shrink_control *sc)
                 * want to shrink a VM that only started to populate its
                 * MMU
                 * anyway.
                 */
-               if (kvm->arch.n_used_mmu_pages > 0) {
-                       if (!nr_to_scan--)
-                               break;
+               if (!kvm->arch.n_used_mmu_pages)
                        continue;
-               }

                idx = srcu_read_lock(&kvm->srcu);
                spin_lock(&kvm->mmu_lock);

This patch removes the maximum (successful) loops, which is nr_scan ==
sc->nr_to_scan.

The description above where you say 'possibility that we see
"n_used_mmu_pages == 0" 128 times' does not match the patch above.
If the patch is correct, then please explain it clearly in the
changelog.

What is the reasoning to remove nr_to_scan? What tests did you perform?