[PATCH] KVM: x86: small optimization for is_mtrr_mask calculation

[PATCH] KVM: x86: small optimization for is_mtrr_mask calculation

[linmiaohe]

From: Miaohe Lin <linmiaohe@huawei.com>

We can get is_mtrr_mask by calculating (msr - 0x200) % 2 directly.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 arch/x86/kvm/mtrr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
index 7f0059aa30e1..a98701d9f2bf 100644
--- a/arch/x86/kvm/mtrr.c
+++ b/arch/x86/kvm/mtrr.c
@@ -348,7 +348,7 @@ static void set_var_mtrr_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	int index, is_mtrr_mask;
 
 	index = (msr - 0x200) / 2;
-	is_mtrr_mask = msr - 0x200 - 2 * index;
+	is_mtrr_mask = (msr - 0x200) % 2;
 	cur = &mtrr_state->var_ranges[index];
 
 	/* remove the entry if it's in the list. */
@@ -424,7 +424,7 @@ int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 		int is_mtrr_mask;
 
 		index = (msr - 0x200) / 2;
-		is_mtrr_mask = msr - 0x200 - 2 * index;
+		is_mtrr_mask = (msr - 0x200) % 2;
 		if (!is_mtrr_mask)
 			*pdata = vcpu->arch.mtrr_state.var_ranges[index].base;
 		else
-- 
2.19.1

Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation

[Paolo Bonzini]

On 05/03/20 03:48, linmiaohe wrote:
> From: Miaohe Lin <linmiaohe@huawei.com>
> 
> We can get is_mtrr_mask by calculating (msr - 0x200) % 2 directly.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  arch/x86/kvm/mtrr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
> index 7f0059aa30e1..a98701d9f2bf 100644
> --- a/arch/x86/kvm/mtrr.c
> +++ b/arch/x86/kvm/mtrr.c
> @@ -348,7 +348,7 @@ static void set_var_mtrr_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  	int index, is_mtrr_mask;
>  
>  	index = (msr - 0x200) / 2;
> -	is_mtrr_mask = msr - 0x200 - 2 * index;
> +	is_mtrr_mask = (msr - 0x200) % 2;
>  	cur = &mtrr_state->var_ranges[index];
>  
>  	/* remove the entry if it's in the list. */
> @@ -424,7 +424,7 @@ int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
>  		int is_mtrr_mask;
>  
>  		index = (msr - 0x200) / 2;
> -		is_mtrr_mask = msr - 0x200 - 2 * index;
> +		is_mtrr_mask = (msr - 0x200) % 2;
>  		if (!is_mtrr_mask)
>  			*pdata = vcpu->arch.mtrr_state.var_ranges[index].base;
>  		else
> 

If you're going to do that, might as well use ">> 1" for index instead
of "/ 2", and "msr & 1" for is_mtrr_mask.

Paolo

RE: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation

[David Laight]

From: Paolo Bonzini
> Sent: 05 March 2020 14:36
> 
> On 05/03/20 03:48, linmiaohe wrote:
> > From: Miaohe Lin <linmiaohe@huawei.com>
> >
> > We can get is_mtrr_mask by calculating (msr - 0x200) % 2 directly.
> >
> > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> > ---
> >  arch/x86/kvm/mtrr.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
> > index 7f0059aa30e1..a98701d9f2bf 100644
> > --- a/arch/x86/kvm/mtrr.c
> > +++ b/arch/x86/kvm/mtrr.c
> > @@ -348,7 +348,7 @@ static void set_var_mtrr_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> >  	int index, is_mtrr_mask;
> >
> >  	index = (msr - 0x200) / 2;
> > -	is_mtrr_mask = msr - 0x200 - 2 * index;
> > +	is_mtrr_mask = (msr - 0x200) % 2;
> >  	cur = &mtrr_state->var_ranges[index];
> >
> >  	/* remove the entry if it's in the list. */
> > @@ -424,7 +424,7 @@ int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
> >  		int is_mtrr_mask;
> >
> >  		index = (msr - 0x200) / 2;
> > -		is_mtrr_mask = msr - 0x200 - 2 * index;
> > +		is_mtrr_mask = (msr - 0x200) % 2;
> >  		if (!is_mtrr_mask)
> >  			*pdata = vcpu->arch.mtrr_state.var_ranges[index].base;
> >  		else
> >
> 
> If you're going to do that, might as well use ">> 1" for index instead
> of "/ 2", and "msr & 1" for is_mtrr_mask.

Provided the variables are unsigned it makes little difference
whether you use / % or >> &.
At least with / % the two values are the same.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation

[Paolo Bonzini]

On 05/03/20 16:10, David Laight wrote:
>>>  	index = (msr - 0x200) / 2;
>>> -	is_mtrr_mask = msr - 0x200 - 2 * index;
>>> +	is_mtrr_mask = (msr - 0x200) % 2;
>>>  	cur = &mtrr_state->var_ranges[index];
>>>
>>>  	/* remove the entry if it's in the list. */
>>> @@ -424,7 +424,7 @@ int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
>>>  		int is_mtrr_mask;
>>>
>>>  		index = (msr - 0x200) / 2;
>>> -		is_mtrr_mask = msr - 0x200 - 2 * index;
>>> +		is_mtrr_mask = (msr - 0x200) % 2;
>>>  		if (!is_mtrr_mask)
>>>  			*pdata = vcpu->arch.mtrr_state.var_ranges[index].base;
>>>  		else
>>>
>> If you're going to do that, might as well use ">> 1" for index instead
>> of "/ 2", and "msr & 1" for is_mtrr_mask.
> Provided the variables are unsigned it makes little difference
> whether you use / % or >> &.
> At least with / % the two values are the same.

Yes, I'm old-fashioned, but also I prefer ">>" and "&" for both signed
and unsigned, because if ever I need to switch from unsigned to signed I
will get floor-division instead of round-to-zero division (most likely
the code doesn't expect negative remainders if it was using unsigned).

(That perhaps also reflects on me working a lot with Smalltalk long
before switching to the kernel...).

Paolo

Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation

[linmiaohe]

Hi,
Paolo Bonzini <pbonzini@redhat.com> wrote:
>On 05/03/20 03:48, linmiaohe wrote:
>> From: Miaohe Lin <linmiaohe@huawei.com>
>> 
>> We can get is_mtrr_mask by calculating (msr - 0x200) % 2 directly.
>>  		index = (msr - 0x200) / 2;
>> -		is_mtrr_mask = msr - 0x200 - 2 * index;
>> +		is_mtrr_mask = (msr - 0x200) % 2;
>>  		if (!is_mtrr_mask)
>>  			*pdata = vcpu->arch.mtrr_state.var_ranges[index].base;
>>  		else
>> 
>
>If you're going to do that, might as well use ">> 1" for index instead of "/ 2", and "msr & 1" for is_mtrr_mask.
>

Many thanks for suggestion. What do you mean is like this ?

	index = (msr - 0x200) >> 1;
	is_mtrr_mask = msr & 1;

Thanks again.

Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation

[hpa]

On March 5, 2020 6:05:40 PM PST, linmiaohe <linmiaohe@huawei.com> wrote:
>Hi,
>Paolo Bonzini <pbonzini@redhat.com> wrote:
>>On 05/03/20 03:48, linmiaohe wrote:
>>> From: Miaohe Lin <linmiaohe@huawei.com>
>>> 
>>> We can get is_mtrr_mask by calculating (msr - 0x200) % 2 directly.
>>>  		index = (msr - 0x200) / 2;
>>> -		is_mtrr_mask = msr - 0x200 - 2 * index;
>>> +		is_mtrr_mask = (msr - 0x200) % 2;
>>>  		if (!is_mtrr_mask)
>>>  			*pdata = vcpu->arch.mtrr_state.var_ranges[index].base;
>>>  		else
>>> 
>>
>>If you're going to do that, might as well use ">> 1" for index instead
>of "/ 2", and "msr & 1" for is_mtrr_mask.
>>
>
>Many thanks for suggestion. What do you mean is like this ?
>
>	index = (msr - 0x200) >> 1;
>	is_mtrr_mask = msr & 1;
>
>Thanks again.

You realize that the compiler will probably produce exactly the same code, right? As such, it is about making the code easy for the human reader.

Even if it didn't, this code is as far from performance critical as one can possibly get.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation

[linmiaohe]

hpa@zytor.com wrote:
>>On March 5, 2020 6:05:40 PM PST, linmiaohe <linmiaohe@huawei.com> wrote:
>>Hi,
>>Paolo Bonzini <pbonzini@redhat.com> wrote:
>>Many thanks for suggestion. What do you mean is like this ?
>>
>>	index = (msr - 0x200) >> 1;
>>	is_mtrr_mask = msr & 1;
>>
>>Thanks again.
>
>You realize that the compiler will probably produce exactly the same code, right? As such, it is about making the code easy for the human reader.
>
>Even if it didn't, this code is as far from performance critical as one can possibly get.

Yep, it looks gain little. Thanks.

Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation

[Wanpeng Li]

On Fri, 6 Mar 2020 at 10:23, linmiaohe <linmiaohe@huawei.com> wrote:
>
> hpa@zytor.com wrote:
> >>On March 5, 2020 6:05:40 PM PST, linmiaohe <linmiaohe@huawei.com> wrote:
> >>Hi,
> >>Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>Many thanks for suggestion. What do you mean is like this ?
> >>
> >>      index = (msr - 0x200) >> 1;
> >>      is_mtrr_mask = msr & 1;
> >>
> >>Thanks again.
> >
> >You realize that the compiler will probably produce exactly the same code, right? As such, it is about making the code easy for the human reader.
> >
> >Even if it didn't, this code is as far from performance critical as one can possibly get.
>
> Yep, it looks gain little. Thanks.

Please post the performance number when you mention optimize XXX later.

    Wanpeng

Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation

[linmiaohe]

Wanpeng Li <kernellwp@gmail.com> wrote:
>On Fri, 6 Mar 2020 at 10:23, linmiaohe <linmiaohe@huawei.com> wrote:
>>
>> hpa@zytor.com wrote:
>> >>On March 5, 2020 6:05:40 PM PST, linmiaohe <linmiaohe@huawei.com> wrote:
>> >>Hi,
>> >>Paolo Bonzini <pbonzini@redhat.com> wrote:
>> >>Many thanks for suggestion. What do you mean is like this ?
>> >>
>> >>      index = (msr - 0x200) >> 1;
>> >>      is_mtrr_mask = msr & 1;
>> >>
>> >>Thanks again.
>> >
>> >You realize that the compiler will probably produce exactly the same code, right? As such, it is about making the code easy for the human reader.
>> >
>> >Even if it didn't, this code is as far from performance critical as one can possibly get.
>>
>> Yep, it looks gain little. Thanks.
>
>Please post the performance number when you mention optimize XXX later.
>

Sure, I would take care of this. Thanks for your reminder!