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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/03/2020 09:07, Jan Beulich wrote:
> 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.

Good point.  Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

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