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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> 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