On 14/07/2016 17:22, Nicolai Stange wrote: > In setup_APIC_timer(), the registered clockevent device's frequency > is calculated by first dividing tsc_khz by TSC_DIVISOR and multiplying > it with 1000 afterwards: > > (tsc_khz / TSC_DIVISOR) * 1000 > > The multiplication with 1000 is done for converting from kHz to Hz and the > division by TSC_DIVISOR is carried out in order to make sure that the final > result fits into an u32. > > However, with the order given in this calculation, the roundoff error > introduced by the division gets magnified by a factor of 1000 by the > following multiplication. Thus, reversing the order of the division and > the multiplication a la > > (tsc_khz * 1000) / TSC_DIVISOR > > reduces the roundoff error already. > > Furthermore, if TSC_DIVISOR divides 1000, associativity holds > > (tsc_khz * 1000) / TSC_DIVISOR = tsc_khz * (1000 / TSC_DIVISOR) > > and thus, the roundoff error even vanishes and the whole operation can be > carried out within 32 bits. > > The powers of two that divide 1000 are 2, 4 and 8. A value of 8 for > TSC_DIVISOR still allows for TSC frequencies up to > 2^32 / 10^9ns * 8 = 34.4GHz which is way larger than anything to expect > in the next years. > > Thus, replace the current TSC_DIVISOR value of 32 by 8. Reverse the > order of the divison and the multiplication in the calculation of the > registered clockevent device's frequency. > > Signed-off-by: Nicolai Stange <[email protected]> > --- > arch/x86/kernel/apic/apic.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index 89a5bce..39517c0 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -311,7 +311,7 @@ int lapic_get_maxlvt(void) > > /* Clock divisor */ > #define APIC_DIVISOR 16 > -#define TSC_DIVISOR 32 > +#define TSC_DIVISOR 8 > > /* > * This function sets up the local APIC timer, with a timeout of > @@ -563,7 +563,7 @@ static void setup_APIC_timer(void) > CLOCK_EVT_FEAT_DUMMY); > levt->set_next_event = lapic_next_deadline; > clockevents_config_and_register(levt, > - (tsc_khz / TSC_DIVISOR) * 1000, > + tsc_khz * (1000 / TSC_DIVISOR), > 0xF, ~0UL); > } else > clockevents_register_device(levt); >
Reviewed-by: Paolo Bonzini <[email protected]> and I suggest even CCing this to stable as it's pretty safe and an improvement. Paolo
