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

[Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>]

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.