From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH v3 1/7] KVM: MMU: fix release noslot pfn Date: Thu, 27 Sep 2012 18:25:47 +0200 Message-ID: <50647E0B.6080303@redhat.com> References: <505C0FA8.5070007@linux.vnet.ibm.com> <505C0FCF.2070308@linux.vnet.ibm.com> <20120923091335.GA20907@redhat.com> <505FE8B4.8030309@linux.vnet.ibm.com> <20120924112416.GA23096@redhat.com> <506048D1.70403@linux.vnet.ibm.com> <20120924120424.GB23096@redhat.com> <506052D1.5000006@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Gleb Natapov , Marcelo Tosatti , LKML , KVM To: Xiao Guangrong Return-path: In-Reply-To: <506052D1.5000006@linux.vnet.ibm.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 09/24/2012 02:32 PM, Xiao Guangrong wrote: > 3 places after the whole patchset (There are some cleanups after this patch). > >> and one by even stronger is_error_pfn(). > > This one is: > > | if (!is_error_pfn(pfn)) { > | kvm_release_pfn_clean(pfn); > | return true; > | } > | > | return false; > > We can change it to: > > | if (is_error_pfn(pfn)) > | return false; > | > | kvm_release_pfn_clean(pfn); > | return true; > >> I guess when/if other architectures will add MMIO MMU >> caching they will need to guard kvm_release_pfn_clean() by is_noslot_pfn() >> too in most cases. I am not insisting, but as this patch shows it is >> easy to miss the check before calling the function. > > Sounds reasonable. I will consider it if Avi/Marcelo have no object on > it. I think it's a good idea. Looks like we traded the unscalable error pages for these branches, I think it's a reasonable tradeoff. -- error compiling committee.c: too many arguments to function