Re: [Xen-devel] [PATCH 2/4] x86/time: reduce rounding errors in calculations

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/03/2020 08:58, Jan Beulich wrote:
> On 13.03.2020 16:14, Andrew Cooper wrote:
>> On 13/03/2020 09:25, Jan Beulich wrote:
>>> Plain (unsigned) integer division simply truncates the results. The
>>> overall errors are smaller though if we use proper rounding. (Extend
>>> this to the purely cosmetic aspect of time.c's freq_string(), which
>>> before this change I've frequently observed to report e.g. NN.999MHz
>>> HPET clock speeds.)
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> ---
>>> We could switch at least the first div/rem pair in calibrate_APIC_clock()
>>> to use do_div(), but this would imply switching bus_freq (and then also
>>> result) to unsigned int (as the divisor has to be 32-bit). While I think
>>> there's pretty little risk for bus frequencies to go beyond 4GHz in the
>>> next so many years, I still wasn't certain enough this would be a welcome
>>> change.
>>
>> Honestly, do_div() is very easy to get wrong (and in security relevant
>> ways in Linux).  I'd advocate for phasing its use out, irrespective of
>> this frequency concern.
>>
>> As for 4GHz, I think its unlikely, but better to be safe in the code.
>>
>>>
>>> --- a/xen/arch/x86/apic.c
>>> +++ b/xen/arch/x86/apic.c
>>> @@ -1261,7 +1261,9 @@ static int __init calibrate_APIC_clock(v
>>>      /* set up multipliers for accurate timer code */
>>>      bus_freq   = result*HZ;
>>>      bus_cycle  = (u32) (1000000000000LL/bus_freq); /* in pico seconds */
>>> +    bus_cycle += (1000000000000UL % bus_freq) * 2 > bus_freq;
>>
>> These two differ in signedness of the numerator.  GCC seems to cope with
>> combining the two into a single div instruction, but I think we should
>> be consistent with the constant nevertheless.
> 
> IOW you'd like me to change the other line too, to have a UL
> suffix? If so, at that point I'd drop the stray cast, too.

That would be fine yes.

~Andrew

> 
>> Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 
> Thanks, but please let me know if the above is a correct
> understanding of mine.
> 
> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel