From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [PATCH 3/3] KVM: MMU: Consolidate common code in mmu_free_roots() Date: Fri, 10 May 2013 09:43:50 +0800 Message-ID: <518C50D6.3080805@linux.vnet.ibm.com> References: <20130509154350.15b956c4.yoshikawa_takuya_b1@lab.ntt.co.jp> <20130509154602.da528c3b.yoshikawa_takuya_b1@lab.ntt.co.jp> <518B7653.60009@linux.vnet.ibm.com> <20130510100545.9e3ee98f.yoshikawa_takuya_b1@lab.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: gleb@redhat.com, pbonzini@redhat.com, mtosatti@redhat.com, kvm@vger.kernel.org To: Takuya Yoshikawa Return-path: Received: from e28smtp04.in.ibm.com ([122.248.162.4]:49642 "EHLO e28smtp04.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757286Ab3EJBn5 (ORCPT ); Thu, 9 May 2013 21:43:57 -0400 Received: from /spool/local by e28smtp04.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 10 May 2013 07:09:04 +0530 Received: from d28relay03.in.ibm.com (d28relay03.in.ibm.com [9.184.220.60]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id 43AB63940023 for ; Fri, 10 May 2013 07:13:53 +0530 (IST) Received: from d28av03.in.ibm.com (d28av03.in.ibm.com [9.184.220.65]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r4A1hiHN7799258 for ; Fri, 10 May 2013 07:13:45 +0530 Received: from d28av03.in.ibm.com (loopback [127.0.0.1]) by d28av03.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r4A1hpng022604 for ; Fri, 10 May 2013 11:43:52 +1000 In-Reply-To: <20130510100545.9e3ee98f.yoshikawa_takuya_b1@lab.ntt.co.jp> Sender: kvm-owner@vger.kernel.org List-ID: On 05/10/2013 09:05 AM, Takuya Yoshikawa wrote: > On Thu, 09 May 2013 18:11:31 +0800 > Xiao Guangrong wrote: > >> On 05/09/2013 02:46 PM, Takuya Yoshikawa wrote: >>> By making the last three statements common to both if/else cases, the >>> symmetry between the locking and unlocking becomes clearer. One note >>> here is that VCPU's root_hpa does not need to be protected by mmu_lock. >>> >>> Signed-off-by: Takuya Yoshikawa >>> --- >>> arch/x86/kvm/mmu.c | 39 +++++++++++++++++++-------------------- >>> 1 files changed, 19 insertions(+), 20 deletions(-) >> >> DO NOT think it makes any thing better. >> > > Why do we need to do the same thing differently in two paths? They are different cases, one is the long mode, anther is PAE mode, return from one case and continue to handle the anther case is common used, and it can reduce the indentation and easily follow the code. Anyway, this is the code style, using if-else is not better than if() return. > Especially one path looks like protecting root_hpa while the other does not. The simple code, "vcpu->arch.mmu.root_hpa = INVALID_PAGE;", does not hurt the lock. But moving them out of the lock is ok.