From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH RESEND] KVM: MMU: Fix mmu_shrink() so that it can free mmu pages as intended Date: Tue, 14 Aug 2012 12:17:12 -0300 Message-ID: <20120814151712.GA14582@amt.cnet> References: <20120810171612.f1e48237.yoshikawa.takuya@oss.ntt.co.jp> <20120813221523.GA21083@amt.cnet> <20120814090651.d7aa468cbdafe6a18ce5c269@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Takuya Yoshikawa , avi@redhat.com, kvm@vger.kernel.org, gleb@redhat.com To: Takuya Yoshikawa Return-path: Received: from mx1.redhat.com ([209.132.183.28]:38981 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752809Ab2HNPRZ (ORCPT ); Tue, 14 Aug 2012 11:17:25 -0400 Content-Disposition: inline In-Reply-To: <20120814090651.d7aa468cbdafe6a18ce5c269@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Aug 14, 2012 at 09:06:51AM +0900, Takuya Yoshikawa wrote: > On Mon, 13 Aug 2012 19:15:23 -0300 > Marcelo Tosatti 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 > > > Cc: Gleb Natapov > > > --- > > > 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?