Re: [PATCH v2] Fix DEC truncation for greater than 0xffff_ffff/1000

[Alexander Graf <agraf@suse.de>]

On 10/31/2011 06:54 AM, Liu Yu-B13201 wrote:
>
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Monday, October 31, 2011 12:21 AM
>> To: Bhushan Bharat-R65777
>> Cc: Liu Yu-B13201; kvm-ppc@vger.kernel.org; bharatb.yadav@gmail.com
>> Subject: Re: [PATCH v2] Fix DEC truncation for greater than
>> 0xffff_ffff/1000
>>
>>
>> On 20.10.2011, at 09:18, Bhushan Bharat-R65777 wrote:
>>
>>>
>>>> -----Original Message-----
>>>> From: Liu Yu-B13201
>>>> Sent: Thursday, October 20, 2011 12:35 PM
>>>> To: Bhushan Bharat-R65777; agraf@suse.de
>>>> Cc: kvm-ppc@vger.kernel.org; bharatb.yadav@gmail.com
>>>> Subject: RE: [PATCH v2] Fix DEC truncation for greater than
>>>> 0xffff_ffff/1000
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Bhushan Bharat-R65777
>>>>> Sent: Thursday, October 20, 2011 2:41 PM
>>>>> To: Liu Yu-B13201; agraf@suse.de
>>>>> Cc: kvm-ppc@vger.kernel.org; bharatb.yadav@gmail.com
>>>>> Subject: RE: [PATCH v2] Fix DEC truncation for greater than
>>>>> 0xffff_ffff/1000
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Liu Yu-B13201
>>>>>> Sent: Wednesday, October 19, 2011 4:28 PM
>>>>>> To: Bhushan Bharat-R65777; agraf@suse.de
>>>>>> Cc: kvm-ppc@vger.kernel.org; bharatb.yadav@gmail.com;
>>>>> Bhushan Bharat-
>>>>>> R65777
>>>>>> Subject: RE: [PATCH v2] Fix DEC truncation for greater than
>>>>>> 0xffff_ffff/1000
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: kvm-ppc-owner@vger.kernel.org
>>>>>>> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of
>> Bharat Bhushan
>>>>>>> Sent: Wednesday, October 19, 2011 12:16 PM
>>>>>>> To: agraf@suse.de
>>>>>>> Cc: kvm-ppc@vger.kernel.org; bharatb.yadav@gmail.com; Bhushan
>>>>>>> Bharat-R65777
>>>>>>> Subject: [PATCH v2] Fix DEC truncation for greater than
>>>>>>> 0xffff_ffff/1000
>>>>>>>
>>>>>>> kvmppc_emulate_dec() uses dec_nsec of type unsigned
>> long and does
>>>>>>> below calculation:
>>>>>>>
>>>>>>>         dec_nsec = vcpu->arch.dec;
>>>>>>>         dec_nsec *= 1000;
>>>>>>> This will truncate if DEC value "vcpu->arch.dec" is greater than
>>>>>>> 0xffff_ffff/1000.
>>>>>>> For example : For tb_ticks_per_usec = 4a, we can not set
>>>>> decrementer
>>>>>>> more than ~58ms.
>>>>>>>
>>>>>>> Signed-off-by: Bharat Bhushan<bharat.bhushan@freescale.com>
>>>>>>> ---
>>>>>>> arch/powerpc/kvm/emulate.c |   12 +++++++-----
>>>>>>> 1 files changed, 7 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/powerpc/kvm/emulate.c
>>>>> b/arch/powerpc/kvm/emulate.c
>>>>>>> index 8af3bad..e7f3da4 100644
>>>>>>> --- a/arch/powerpc/kvm/emulate.c
>>>>>>> +++ b/arch/powerpc/kvm/emulate.c
>>>>>>> @@ -84,6 +84,7 @@ static bool kvmppc_dec_enabled(struct kvm_vcpu
>>>>>>> *vcpu)  void kvmppc_emulate_dec(struct kvm_vcpu *vcpu)  {
>>>>>>> 	unsigned long dec_nsec;
>>>>>>> +	unsigned long long dec_time;
>>>>>>>
>>>>>>> 	pr_debug("mtDEC: %x\n", vcpu->arch.dec);  #ifdef
>>>>>>> CONFIG_PPC_BOOK3S @@ -103,11 +104,12 @@ void
>>>>>>> kvmppc_emulate_dec(struct
>>>>> kvm_vcpu *vcpu)
>>>>>>> 		* host ticks. */
>>>>>>>
>>>>>>> 		hrtimer_try_to_cancel(&vcpu->arch.dec_timer);
>>>>>>> -		dec_nsec = vcpu->arch.dec;
>>>>>>> -		dec_nsec *= 1000;
>>>>>>> -		dec_nsec /= tb_ticks_per_usec;
>>>>>>> -		hrtimer_start(&vcpu->arch.dec_timer,
>>>>>>> ktime_set(0, dec_nsec),
>>>>>>> -			      HRTIMER_MODE_REL);
>>>>>>> +		dec_time = vcpu->arch.dec;
>>>>>>> +		dec_time *= 1000;
>>>>>>> +		do_div(dec_time, tb_ticks_per_usec);
>>>>>>> +		dec_nsec = do_div(dec_time, NSEC_PER_SEC);
>>>>>>> +		hrtimer_start(&vcpu->arch.dec_timer,
>>>>>>> +			ktime_set(dec_time, dec_nsec),
>>>>>>> HRTIMER_MODE_REL);
>>>>>>> 		vcpu->arch.dec_jiffies = get_tb();
>>>>>>> 	} else {
>>>>>>> 		hrtimer_try_to_cancel(&vcpu->arch.dec_timer);
>>>>>>> --
>>>>>>> 1.7.0.4
>>>>>>>
>>>>>> How does this impact performance?
>>>>>> 64bits multiplication and division looks slow.
>>>>>>
>>>>> I tried running below test as guest, with and without
>> this patch and
>>>>> tried to find latency added by this patch. Also I run
>> this for a list
>>>>> of timeouts (1, 2 , 4, 8, 16, 32ms) one by one.
>>>>>
>>>>> - get TB (say a).
>>>>> - set decrementer in auto reload mode.
>>>>> - wait for 1000 timebase interrupts.
>>>>> - Get timebase delta (b = get_tb - a).
>>>>>
>>>>> 		(b1	-   b2)<=>  b1 with this patch and b2
>>>>> without this patch. And roughly I found any impact. For example:
>>>>> For 1ms =  ( 48a19d8 -  48a1459)  = 0x57f  = .0018% For 32ms >>>>> (90fdfa23 - 90fdfe79)  = -(0x456)
>>>> Doesn't (b1 - b2) mean difference of the last one
>> interrupt between have
>>>> patch and havenot patch?
>>>> The time of previous 999 interrupts is hidden in the cpu idle time.
>>>>
>>>
>>> Probably I have not described properly. b1 and b2 are
>> delta, not timestamp. In this case I run this test with patch
>>> 	Print on console the total time (in TB tick) for which
>> this test runs. Which includes time of all 1000 interrupts.
>>> Then I exit and rerun the above test case without patch
>>> Then mannualy calculated difference/percentage etc.
>>>
>>> Also if you see timebase delta, it suggest that it is not
>> timebase difference of one decrementer.
>>
>> So Yu are you ok with this patch? If so, please ack.
>>
> Acked-by: Liu Yu<yu.liu@freescale.com>

Thanks, applied to kvm-ppc-next.


Alex