Re: [PATCH v4 1/2] arch, x86, tsc deadline clockevent dev: eliminate frequency roundoff error

From: Paolo Bonzini
Date: Thu Jul 14 2016 - 11:44:10 EST




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 <nicstange@xxxxxxxxx>
> ---
> 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 <pbonzini@xxxxxxxxxx>

and I suggest even CCing this to stable as it's pretty safe and an
improvement.

Paolo