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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.03.2020 16:50, Andrew Cooper wrote:
> On 13/03/2020 09:26, Jan Beulich wrote:
>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -1249,17 +1249,16 @@ static int __init calibrate_APIC_clock(v
>>      tt2 = apic_read(APIC_TMCCT);
>>      t2 = rdtsc_ordered();
>>  
>> -    result = (tt1-tt2)*APIC_DIVISOR/LOOPS;
>> +    bus_freq = (tt1 - tt2) * APIC_DIVISOR * LOOPS_FRAC;
>>  
>> -    apic_printk(APIC_VERBOSE, "..... CPU clock speed is %ld.%04ld MHz.\n",
>> -                ((long)(t2 - t1) / LOOPS) / (1000000 / HZ),
>> -                ((long)(t2 - t1) / LOOPS) % (1000000 / HZ));
>> +    apic_printk(APIC_VERBOSE, "..... CPU clock speed is %lu.%04lu MHz.\n",
>> +                ((unsigned long)(t2 - t1) * LOOPS_FRAC) / 1000000,
>> +                ((unsigned long)(t2 - t1) * LOOPS_FRAC / 100) % 10000);
> 
> If I'm not mistaken, this expression only works because of the L->R
> associativity (given the same precedence of * and /) grouping it as
> ((t2-t1) * 10)  / 100 as opposed to (t2-t1) * (10 / 100), where the
> latter would truncate to 0.  I think some extra brackets would help for
> clarity.

Well, yes, done. The alternative would have been to drop more of
them.

> That said, what is wrong with keeping the original form?

The same as elsewhere in this patch, and as said in the description -
there's been pointless rounding (really: truncation) errors here from
first dividing by HZ (to be precise: by HZ/10) and then effectively
multiplying by this value again. The original division-only argument
would not be affected afaict, but the remainder one would. Furthermore
I'd like to avoid having to retain the LOOPS constant.

>  I realise this
> is only for a printk(), but the div instruction can't be shared between
> the two halves.

This being an __init function, I don't think the number of divisions
is a concern here, the more that the compiler - with the divisor
being a constant - will convert them to multiplications anyway.

Jan

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