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

[Paolo Bonzini]

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