[PATCH 0/3] KVM: MMU: Simple mmu cleanups

[PATCH 0/3] KVM: MMU: Simple mmu cleanups

[Takuya Yoshikawa]

Found during documenting mmu_lock usage for myself.

Takuya Yoshikawa (3):
  KVM: MMU: Clean up set_spte()'s ACC_WRITE_MASK handling
  KVM: MMU: Use kvm_mmu_sync_roots() in kvm_mmu_load()
  KVM: MMU: Consolidate common code in mmu_free_roots()

 arch/x86/kvm/mmu.c |   48 +++++++++++++++++++++---------------------------
 1 files changed, 21 insertions(+), 27 deletions(-)

-- 
1.7.5.4

[PATCH 1/3] KVM: MMU: Clean up set_spte()'s ACC_WRITE_MASK handling

[Takuya Yoshikawa]

Rather than clearing the ACC_WRITE_MASK bit of pte_access in the
"if (mmu_need_write_protect())" block not to call mark_page_dirty() in
the following if statement, simply moving the call into the appropriate
else block is better.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
 arch/x86/kvm/mmu.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 004cc87..08119a8 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2386,14 +2386,11 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 			pgprintk("%s: found shadow page for %llx, marking ro\n",
 				 __func__, gfn);
 			ret = 1;
-			pte_access &= ~ACC_WRITE_MASK;
 			spte &= ~(PT_WRITABLE_MASK | SPTE_MMU_WRITEABLE);
-		}
+		} else
+			mark_page_dirty(vcpu->kvm, gfn);
 	}
 
-	if (pte_access & ACC_WRITE_MASK)
-		mark_page_dirty(vcpu->kvm, gfn);
-
 set_pte:
 	if (mmu_spte_update(sptep, spte))
 		kvm_flush_remote_tlbs(vcpu->kvm);
-- 
1.7.5.4

Re: [PATCH 1/3] KVM: MMU: Clean up set_spte()'s ACC_WRITE_MASK handling

[Xiao Guangrong]

On 05/09/2013 02:44 PM, Takuya Yoshikawa wrote:
> Rather than clearing the ACC_WRITE_MASK bit of pte_access in the
> "if (mmu_need_write_protect())" block not to call mark_page_dirty() in
> the following if statement, simply moving the call into the appropriate
> else block is better.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> ---
>  arch/x86/kvm/mmu.c |    7 ++-----
>  1 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 004cc87..08119a8 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2386,14 +2386,11 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
>  			pgprintk("%s: found shadow page for %llx, marking ro\n",
>  				 __func__, gfn);
>  			ret = 1;
> -			pte_access &= ~ACC_WRITE_MASK;
>  			spte &= ~(PT_WRITABLE_MASK | SPTE_MMU_WRITEABLE);
> -		}
> +		} else
> +			mark_page_dirty(vcpu->kvm, gfn);
>  	}
> 
> -	if (pte_access & ACC_WRITE_MASK)
> -		mark_page_dirty(vcpu->kvm, gfn);
> -
>  set_pte:
>  	if (mmu_spte_update(sptep, spte))
>  		kvm_flush_remote_tlbs(vcpu->kvm);

That function is really magic, and this change do no really help it. I had several
patches posted some months ago to make these kind of code better understanding, but
i am too tired to update them.

Re: [PATCH 1/3] KVM: MMU: Clean up set_spte()'s ACC_WRITE_MASK handling

[Gleb Natapov]

On Thu, May 09, 2013 at 06:16:55PM +0800, Xiao Guangrong wrote:
> On 05/09/2013 02:44 PM, Takuya Yoshikawa wrote:
> > Rather than clearing the ACC_WRITE_MASK bit of pte_access in the
> > "if (mmu_need_write_protect())" block not to call mark_page_dirty() in
> > the following if statement, simply moving the call into the appropriate
> > else block is better.
> > 
> > Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> > ---
> >  arch/x86/kvm/mmu.c |    7 ++-----
> >  1 files changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > index 004cc87..08119a8 100644
> > --- a/arch/x86/kvm/mmu.c
> > +++ b/arch/x86/kvm/mmu.c
> > @@ -2386,14 +2386,11 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
> >  			pgprintk("%s: found shadow page for %llx, marking ro\n",
> >  				 __func__, gfn);
> >  			ret = 1;
> > -			pte_access &= ~ACC_WRITE_MASK;
> >  			spte &= ~(PT_WRITABLE_MASK | SPTE_MMU_WRITEABLE);
> > -		}
> > +		} else
> > +			mark_page_dirty(vcpu->kvm, gfn);
> >  	}
> > 
> > -	if (pte_access & ACC_WRITE_MASK)
> > -		mark_page_dirty(vcpu->kvm, gfn);
> > -
> >  set_pte:
> >  	if (mmu_spte_update(sptep, spte))
> >  		kvm_flush_remote_tlbs(vcpu->kvm);
> 
> That function is really magic, and this change do no really help it. I had several
> patches posted some months ago to make these kind of code better understanding, but
> i am too tired to update them.
Can you point me to them? Your work is really appreciated, I am sorry
you feel this way.

--
			Gleb.

Re: [PATCH 1/3] KVM: MMU: Clean up set_spte()'s ACC_WRITE_MASK handling

[Xiao Guangrong]

On 05/09/2013 07:18 PM, Gleb Natapov wrote:
> On Thu, May 09, 2013 at 06:16:55PM +0800, Xiao Guangrong wrote:
>> On 05/09/2013 02:44 PM, Takuya Yoshikawa wrote:
>>> Rather than clearing the ACC_WRITE_MASK bit of pte_access in the
>>> "if (mmu_need_write_protect())" block not to call mark_page_dirty() in
>>> the following if statement, simply moving the call into the appropriate
>>> else block is better.
>>>
>>> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
>>> ---
>>>  arch/x86/kvm/mmu.c |    7 ++-----
>>>  1 files changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>>> index 004cc87..08119a8 100644
>>> --- a/arch/x86/kvm/mmu.c
>>> +++ b/arch/x86/kvm/mmu.c
>>> @@ -2386,14 +2386,11 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
>>>  			pgprintk("%s: found shadow page for %llx, marking ro\n",
>>>  				 __func__, gfn);
>>>  			ret = 1;
>>> -			pte_access &= ~ACC_WRITE_MASK;
>>>  			spte &= ~(PT_WRITABLE_MASK | SPTE_MMU_WRITEABLE);
>>> -		}
>>> +		} else
>>> +			mark_page_dirty(vcpu->kvm, gfn);
>>>  	}
>>>
>>> -	if (pte_access & ACC_WRITE_MASK)
>>> -		mark_page_dirty(vcpu->kvm, gfn);
>>> -
>>>  set_pte:
>>>  	if (mmu_spte_update(sptep, spte))
>>>  		kvm_flush_remote_tlbs(vcpu->kvm);
>>
>> That function is really magic, and this change do no really help it. I had several
>> patches posted some months ago to make these kind of code better understanding, but
>> i am too tired to update them.
> Can you point me to them? Your work is really appreciated, I am sorry

There are two patches about this set_spte cleanups:

https://lkml.org/lkml/2013/1/23/125
https://lkml.org/lkml/2013/1/23/138

> you feel this way.

It is not your fault, it is mine.

Will update these patches when i finish the zap-all-page and zap-mmio-sp
related things.

Re: [PATCH 1/3] KVM: MMU: Clean up set_spte()'s ACC_WRITE_MASK handling

[Takuya Yoshikawa]

On Thu, 09 May 2013 20:16:18 +0800
Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> wrote:

> >> That function is really magic, and this change do no really help it. I had several
> >> patches posted some months ago to make these kind of code better understanding, but
> >> i am too tired to update them.

Thank you for reviewing my patches.

But please don't express your personal frustrations on your pending work
while reviewing patches from others.  "too tired" made me think you would
not send updated patches...

If you want to explain possible conflicts with your patches, I can
understand.  Please say so in that casse.

	Takuya

> > Can you point me to them? Your work is really appreciated, I am sorry
> 
> There are two patches about this set_spte cleanups:
> 
> https://lkml.org/lkml/2013/1/23/125
> https://lkml.org/lkml/2013/1/23/138
> 
> > you feel this way.
> 
> It is not your fault, it is mine.
> 
> Will update these patches when i finish the zap-all-page and zap-mmio-sp
> related things.

[PATCH 2/3] KVM: MMU: Use kvm_mmu_sync_roots() in kvm_mmu_load()

[Takuya Yoshikawa]

No need to open-code this function.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
 arch/x86/kvm/mmu.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 08119a8..d01f340 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3761,9 +3761,7 @@ int kvm_mmu_load(struct kvm_vcpu *vcpu)
 	if (r)
 		goto out;
 	r = mmu_alloc_roots(vcpu);
-	spin_lock(&vcpu->kvm->mmu_lock);
-	mmu_sync_roots(vcpu);
-	spin_unlock(&vcpu->kvm->mmu_lock);
+	kvm_mmu_sync_roots(vcpu);
 	if (r)
 		goto out;
 	/* set_cr3() should ensure TLB has been flushed */
-- 
1.7.5.4

Re: [PATCH 2/3] KVM: MMU: Use kvm_mmu_sync_roots() in kvm_mmu_load()

[Xiao Guangrong]

On 05/09/2013 02:45 PM, Takuya Yoshikawa wrote:
> No need to open-code this function.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> ---
>  arch/x86/kvm/mmu.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 08119a8..d01f340 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -3761,9 +3761,7 @@ int kvm_mmu_load(struct kvm_vcpu *vcpu)
>  	if (r)
>  		goto out;
>  	r = mmu_alloc_roots(vcpu);
> -	spin_lock(&vcpu->kvm->mmu_lock);
> -	mmu_sync_roots(vcpu);
> -	spin_unlock(&vcpu->kvm->mmu_lock);
> +	kvm_mmu_sync_roots(vcpu);

Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>

Re: [PATCH 2/3] KVM: MMU: Use kvm_mmu_sync_roots() in kvm_mmu_load()

[Gleb Natapov]

On Thu, May 09, 2013 at 03:45:12PM +0900, Takuya Yoshikawa wrote:
> No need to open-code this function.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> ---
>  arch/x86/kvm/mmu.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
Applied, thanks.

> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 08119a8..d01f340 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -3761,9 +3761,7 @@ int kvm_mmu_load(struct kvm_vcpu *vcpu)
>  	if (r)
>  		goto out;
>  	r = mmu_alloc_roots(vcpu);
> -	spin_lock(&vcpu->kvm->mmu_lock);
> -	mmu_sync_roots(vcpu);
> -	spin_unlock(&vcpu->kvm->mmu_lock);
> +	kvm_mmu_sync_roots(vcpu);
>  	if (r)
>  		goto out;
>  	/* set_cr3() should ensure TLB has been flushed */
> -- 
> 1.7.5.4

--
			Gleb.

[PATCH 3/3] KVM: MMU: Consolidate common code in mmu_free_roots()

[Takuya Yoshikawa]

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 <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
 arch/x86/kvm/mmu.c |   39 +++++++++++++++++++--------------------
 1 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index d01f340..bf80b46 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2861,42 +2861,41 @@ out_unlock:
 static void mmu_free_roots(struct kvm_vcpu *vcpu)
 {
 	int i;
+	hpa_t root;
 	struct kvm_mmu_page *sp;
 	LIST_HEAD(invalid_list);
 
 	if (!VALID_PAGE(vcpu->arch.mmu.root_hpa))
 		return;
+
 	spin_lock(&vcpu->kvm->mmu_lock);
+
 	if (vcpu->arch.mmu.shadow_root_level == PT64_ROOT_LEVEL &&
 	    (vcpu->arch.mmu.root_level == PT64_ROOT_LEVEL ||
 	     vcpu->arch.mmu.direct_map)) {
-		hpa_t root = vcpu->arch.mmu.root_hpa;
-
+		root = vcpu->arch.mmu.root_hpa;
 		sp = page_header(root);
 		--sp->root_count;
-		if (!sp->root_count && sp->role.invalid) {
+		if (!sp->root_count && sp->role.invalid)
 			kvm_mmu_prepare_zap_page(vcpu->kvm, sp, &invalid_list);
-			kvm_mmu_commit_zap_page(vcpu->kvm, &invalid_list);
+	} else {
+		for (i = 0; i < 4; ++i) {
+			root = vcpu->arch.mmu.pae_root[i];
+			if (root) {
+				root &= PT64_BASE_ADDR_MASK;
+				sp = page_header(root);
+				--sp->root_count;
+				if (!sp->root_count && sp->role.invalid)
+					kvm_mmu_prepare_zap_page(vcpu->kvm, sp,
+								 &invalid_list);
+			}
+			vcpu->arch.mmu.pae_root[i] = INVALID_PAGE;
 		}
-		vcpu->arch.mmu.root_hpa = INVALID_PAGE;
-		spin_unlock(&vcpu->kvm->mmu_lock);
-		return;
 	}
-	for (i = 0; i < 4; ++i) {
-		hpa_t root = vcpu->arch.mmu.pae_root[i];
 
-		if (root) {
-			root &= PT64_BASE_ADDR_MASK;
-			sp = page_header(root);
-			--sp->root_count;
-			if (!sp->root_count && sp->role.invalid)
-				kvm_mmu_prepare_zap_page(vcpu->kvm, sp,
-							 &invalid_list);
-		}
-		vcpu->arch.mmu.pae_root[i] = INVALID_PAGE;
-	}
 	kvm_mmu_commit_zap_page(vcpu->kvm, &invalid_list);
 	spin_unlock(&vcpu->kvm->mmu_lock);
+
 	vcpu->arch.mmu.root_hpa = INVALID_PAGE;
 }
 
-- 
1.7.5.4

Re: [PATCH 3/3] KVM: MMU: Consolidate common code in mmu_free_roots()

[Xiao Guangrong]

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 <yoshikawa_takuya_b1@lab.ntt.co.jp>
> ---
>  arch/x86/kvm/mmu.c |   39 +++++++++++++++++++--------------------
>  1 files changed, 19 insertions(+), 20 deletions(-)

DO NOT think it makes any thing better.

Re: [PATCH 3/3] KVM: MMU: Consolidate common code in mmu_free_roots()

[Takuya Yoshikawa]

On Thu, 09 May 2013 18:11:31 +0800
Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> 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 <yoshikawa_takuya_b1@lab.ntt.co.jp>
> > ---
> >  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?
Especially one path looks like protecting root_hpa while the other does not.

This one may be a small issue, but these small issues make it difficult
to see what mmu_lock is protecting.

	Takuya

Re: [PATCH 3/3] KVM: MMU: Consolidate common code in mmu_free_roots()

[Xiao Guangrong]

On 05/10/2013 09:05 AM, Takuya Yoshikawa wrote:
> On Thu, 09 May 2013 18:11:31 +0800
> Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> 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 <yoshikawa_takuya_b1@lab.ntt.co.jp>
>>> ---
>>>  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.

Re: [PATCH 3/3] KVM: MMU: Consolidate common code in mmu_free_roots()

[Gleb Natapov]

On Fri, May 10, 2013 at 09:43:50AM +0800, Xiao Guangrong wrote:
> On 05/10/2013 09:05 AM, Takuya Yoshikawa wrote:
> > On Thu, 09 May 2013 18:11:31 +0800
> > Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> 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 <yoshikawa_takuya_b1@lab.ntt.co.jp>
> >>> ---
> >>>  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.
> 
I agree that this is mostly code style issue and with Takuya patch the
indentation is dipper. Also the structure of mmu_free_roots() resembles
mmu_alloc_shadow_roots() currently. The latter has the same if(){return}
pattern. I also agree with Takuya that locking symmetry can be improved.
What about something like this (untested):

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 40d7b2d..f8ca2f3 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2869,22 +2869,25 @@ static void mmu_free_roots(struct kvm_vcpu *vcpu)
 
 	if (!VALID_PAGE(vcpu->arch.mmu.root_hpa))
 		return;
-	spin_lock(&vcpu->kvm->mmu_lock);
+
 	if (vcpu->arch.mmu.shadow_root_level == PT64_ROOT_LEVEL &&
 	    (vcpu->arch.mmu.root_level == PT64_ROOT_LEVEL ||
 	     vcpu->arch.mmu.direct_map)) {
 		hpa_t root = vcpu->arch.mmu.root_hpa;
 
+		spin_lock(&vcpu->kvm->mmu_lock);
 		sp = page_header(root);
 		--sp->root_count;
 		if (!sp->root_count && sp->role.invalid) {
 			kvm_mmu_prepare_zap_page(vcpu->kvm, sp, &invalid_list);
 			kvm_mmu_commit_zap_page(vcpu->kvm, &invalid_list);
 		}
-		vcpu->arch.mmu.root_hpa = INVALID_PAGE;
 		spin_unlock(&vcpu->kvm->mmu_lock);
+		vcpu->arch.mmu.root_hpa = INVALID_PAGE;
 		return;
 	}
+
+	spin_lock(&vcpu->kvm->mmu_lock);
 	for (i = 0; i < 4; ++i) {
 		hpa_t root = vcpu->arch.mmu.pae_root[i];
 

--
			Gleb.

Re: [PATCH 3/3] KVM: MMU: Consolidate common code in mmu_free_roots()

[Xiao Guangrong]

On 05/13/2013 07:24 PM, Gleb Natapov wrote:
> On Fri, May 10, 2013 at 09:43:50AM +0800, Xiao Guangrong wrote:
>> On 05/10/2013 09:05 AM, Takuya Yoshikawa wrote:
>>> On Thu, 09 May 2013 18:11:31 +0800
>>> Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> 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 <yoshikawa_takuya_b1@lab.ntt.co.jp>
>>>>> ---
>>>>>  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.
>>
> I agree that this is mostly code style issue and with Takuya patch the
> indentation is dipper. Also the structure of mmu_free_roots() resembles
> mmu_alloc_shadow_roots() currently. The latter has the same if(){return}
> pattern. I also agree with Takuya that locking symmetry can be improved.
> What about something like this (untested):

It is good to me! ;)

Re: [PATCH 3/3] KVM: MMU: Consolidate common code in mmu_free_roots()

[Takuya Yoshikawa]

On Mon, 13 May 2013 21:02:10 +0800
Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> wrote:

> On 05/13/2013 07:24 PM, Gleb Natapov wrote:

> > I agree that this is mostly code style issue and with Takuya patch the
> > indentation is dipper. Also the structure of mmu_free_roots() resembles
> > mmu_alloc_shadow_roots() currently. The latter has the same if(){return}
> > pattern. I also agree with Takuya that locking symmetry can be improved.
> > What about something like this (untested):
> 
> It is good to me! ;)
> 

I agree with you two!  Thank you for reviewing.

Gleb, could you please make it your official patch?

	Takuya