| Message ID | 8221cc7f-ad33-03da-5780-8a76fbdc404a@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: reduce errors in frequency calculations / unrelated cleanup | expand |
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. That said, what is wrong with keeping the original form? I realise this is only for a printk(), but the div instruction can't be shared between the two halves. ~Andrew
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
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@citrix.com>
--- a/xen/arch/x86/apic.c +++ b/xen/arch/x86/apic.c @@ -1204,14 +1204,14 @@ static void wait_tick_pvh(void) * APIC irq that way. */ -static int __init calibrate_APIC_clock(void) +static void __init calibrate_APIC_clock(void) { unsigned long long t1, t2; - unsigned long tt1, tt2, result; + unsigned long tt1, tt2; unsigned int i; unsigned long bus_freq; /* KAF: pointer-size avoids compile warns. */ unsigned int bus_cycle; /* length of one bus cycle in pico-seconds */ - const unsigned int LOOPS = HZ/10; +#define LOOPS_FRAC 10U /* measure for one tenth of a second */ apic_printk(APIC_VERBOSE, "calibrating APIC timer ...\n"); @@ -1238,9 +1238,9 @@ static int __init calibrate_APIC_clock(v tt1 = apic_read(APIC_TMCCT); /* - * Let's wait LOOPS ticks: + * Let's wait HZ / LOOPS_FRAC ticks: */ - for (i = 0; i < LOOPS; i++) + for (i = 0; i < HZ / LOOPS_FRAC; i++) if ( !xen_guest ) wait_8254_wraparound(); else @@ -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); apic_printk(APIC_VERBOSE, "..... host bus clock speed is %ld.%04ld MHz.\n", - result / (1000000 / HZ), result % (1000000 / HZ)); + bus_freq / 1000000, (bus_freq / 100) % 10000); /* 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; bus_scale = (1000*262144)/bus_cycle; @@ -1269,7 +1268,7 @@ static int __init calibrate_APIC_clock(v /* reset APIC to zero timeout value */ __setup_APIC_LVTT(0); - return result; +#undef LOOPS_FRAC } void __init setup_boot_APIC_clock(void)
Dividing by HZ/10 just to subsequently multiply by HZ again in all uses of the respective variable is pretty pointlessly introducing rounding (really: truncation) errors. While transforming the respective expressions it became apparent that "result" would be left unused except for its use as function return value. As the sole caller of the function doesn't look at the returned value, simply convert the function to have "void" return type. Signed-off-by: Jan Beulich <jbeulich@suse.com>