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.coop...@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel