linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation
@ 2020-03-06  2:05 linmiaohe
  2020-03-06  2:09 ` hpa
  0 siblings, 1 reply; 9+ messages in thread
From: linmiaohe @ 2020-03-06  2:05 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: kvm, linux-kernel, x86, rkrcmar, sean.j.christopherson, vkuznets,
	jmattson, joro, tglx, mingo, bp, hpa

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation
  2020-03-06  2:05 [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation linmiaohe
@ 2020-03-06  2:09 ` hpa
  0 siblings, 0 replies; 9+ messages in thread
From: hpa @ 2020-03-06  2:09 UTC (permalink / raw)
  To: linmiaohe, Paolo Bonzini
  Cc: kvm, linux-kernel, x86, rkrcmar, sean.j.christopherson, vkuznets,
	jmattson, joro, tglx, mingo, bp

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation
@ 2020-03-06  2:30 linmiaohe
  0 siblings, 0 replies; 9+ messages in thread
From: linmiaohe @ 2020-03-06  2:30 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: hpa, Paolo Bonzini, kvm, linux-kernel, x86, rkrcmar,
	sean.j.christopherson, vkuznets, jmattson, joro, tglx, mingo, bp

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!


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation
  2020-03-06  2:22 linmiaohe
@ 2020-03-06  2:27 ` Wanpeng Li
  0 siblings, 0 replies; 9+ messages in thread
From: Wanpeng Li @ 2020-03-06  2:27 UTC (permalink / raw)
  To: linmiaohe
  Cc: hpa, Paolo Bonzini, kvm, linux-kernel, x86, rkrcmar,
	sean.j.christopherson, vkuznets, jmattson, joro, tglx, mingo, bp

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation
@ 2020-03-06  2:22 linmiaohe
  2020-03-06  2:27 ` Wanpeng Li
  0 siblings, 1 reply; 9+ messages in thread
From: linmiaohe @ 2020-03-06  2:22 UTC (permalink / raw)
  To: hpa
  Cc: Paolo Bonzini, kvm, linux-kernel, x86, rkrcmar,
	sean.j.christopherson, vkuznets, jmattson, joro, tglx, mingo, bp

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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation
  2020-03-05 15:10   ` David Laight
@ 2020-03-05 15:25     ` Paolo Bonzini
  0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2020-03-05 15:25 UTC (permalink / raw)
  To: David Laight, linmiaohe, rkrcmar, sean.j.christopherson,
	vkuznets, jmattson, joro, tglx, mingo, bp, hpa
  Cc: kvm, linux-kernel, x86

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation
  2020-03-05 14:35 ` Paolo Bonzini
@ 2020-03-05 15:10   ` David Laight
  2020-03-05 15:25     ` Paolo Bonzini
  0 siblings, 1 reply; 9+ messages in thread
From: David Laight @ 2020-03-05 15:10 UTC (permalink / raw)
  To: 'Paolo Bonzini',
	linmiaohe, rkrcmar, sean.j.christopherson, vkuznets, jmattson,
	joro, tglx, mingo, bp, hpa
  Cc: kvm, linux-kernel, x86

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)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation
  2020-03-05  2:48 linmiaohe
@ 2020-03-05 14:35 ` Paolo Bonzini
  2020-03-05 15:10   ` David Laight
  0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2020-03-05 14:35 UTC (permalink / raw)
  To: linmiaohe, rkrcmar, sean.j.christopherson, vkuznets, jmattson,
	joro, tglx, mingo, bp, hpa
  Cc: kvm, linux-kernel, x86

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation
@ 2020-03-05  2:48 linmiaohe
  2020-03-05 14:35 ` Paolo Bonzini
  0 siblings, 1 reply; 9+ messages in thread
From: linmiaohe @ 2020-03-05  2:48 UTC (permalink / raw)
  To: pbonzini, rkrcmar, sean.j.christopherson, vkuznets, jmattson,
	joro, tglx, mingo, bp, hpa
  Cc: linmiaohe, kvm, linux-kernel, x86

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-03-06  2:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-06  2:05 [PATCH] KVM: x86: small optimization for is_mtrr_mask calculation linmiaohe
2020-03-06  2:09 ` hpa
  -- strict thread matches above, loose matches on Subject: below --
2020-03-06  2:30 linmiaohe
2020-03-06  2:22 linmiaohe
2020-03-06  2:27 ` Wanpeng Li
2020-03-05  2:48 linmiaohe
2020-03-05 14:35 ` Paolo Bonzini
2020-03-05 15:10   ` David Laight
2020-03-05 15:25     ` Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).