From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH RESEND] KVM: MMU: Fix mmu_shrink() so that it can free
 mmu pages as intended
Date: Wed, 15 Aug 2012 15:22:04 -0300
Message-ID: <20120815182204.GA21271@amt.cnet>
References: <20120810171612.f1e48237.yoshikawa.takuya@oss.ntt.co.jp>
 <20120813221523.GA21083@amt.cnet>
 <20120814090651.d7aa468cbdafe6a18ce5c269@gmail.com>
 <20120814151712.GA14582@amt.cnet>
 <20120815111151.96fc2553d25fc3bb6b69d4e4@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>, avi@redhat.com,
	kvm@vger.kernel.org, gleb@redhat.com
To: Takuya Yoshikawa <takuya.yoshikawa@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:17191 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752740Ab2HOTJl (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 15 Aug 2012 15:09:41 -0400
Content-Disposition: inline
In-Reply-To: <20120815111151.96fc2553d25fc3bb6b69d4e4@gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Wed, Aug 15, 2012 at 11:11:51AM +0900, Takuya Yoshikawa wrote:
> On Tue, 14 Aug 2012 12:17:12 -0300
> Marcelo Tosatti <mtosatti@redhat.com> wrote:
> 
> > -               if (kvm->arch.n_used_mmu_pages > 0) {
> > -                       if (!nr_to_scan--)
> > -                               break;
> 
> -- (*1)
> 
> > +               if (!kvm->arch.n_used_mmu_pages)
> >                         continue;
> 
> -- (*2)
> 
> > -               }
> > 
> >                 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.
> 
> IIUC, there was no successful loop from the beginning:
> 
>   if (kvm->arch.n_used_mmu_pages > 0) {
>     if (!nr_to_scan--)
>       break;  -- (*1)
>     continue; -- (*2)
>   }
> 
> Before the patch even when we find a VM with kvm->arch.n_used_mmu_pages
> greater than 0, we just do either:
>   skip it (*2) or
>   break   (*1) if nr_to_scan becomes 0.
> 
> We only reach to
>   kvm_mmu_remove_some_alloc_mmu_pages(kvm, &invalid_list);
>   kvm_mmu_commit_zap_page(kvm, &invalid_list);
> when (kvm->arch.n_used_mmu_pages == 0) that is probably why 
>   commit 85b7059169e128c57a3a8a3e588fb89cb2031da1
>   KVM: MMU: fix shrinking page from the empty mmu
> 
> could hit the very unlikely condition so easily.
> 
> So we are just looping for trying to free from empty MMUs.
> 
> > The description above where you say 'possibility that we see
> > "n_used_mmu_pages == 0" 128 times' does not match the patch above.
> 
> Sorry about that.
> 
> > If the patch is correct, then please explain it clearly in the
> > changelog.
> 
> Yes, I will do so.
> 
> > What is the reasoning to remove nr_to_scan? What tests did you perform?
> 
> I just confirmed:
>  - mmu_shrink() did not free any pages:
>    just checked all VMs and did "continue"
>  - with my patch, it could free from the first VM with (n_used_mmu_pages > 0)
> 
> About nr_to_scan:
> If my explanation above is right, this is not functioning at all.
> But since it will not hurt anyone and may help us when we change
> our batch size, I won't remove it in the next version.
> 
> Thanks,
> 	Takuya

Ouch.

OK, please resend with dumb-proof changelog.