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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <14f0de3b-236d-20b3-ea8e-57e9cad03479@redhat.com>
Date:	Thu, 14 Jul 2016 17:43:53 +0200
From:	Paolo Bonzini <pbonzini@...hat.com>
To:	Nicolai Stange <nicstange@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>
Cc:	Ingo Molnar <mingo@...hat.com>, "H. Peter Anvin" <hpa@...or.com>,
	x86@...nel.org, Borislav Petkov <bp@...e.de>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>,
	"Peter Zijlstra (Intel)" <peterz@...radead.org>,
	"Christopher S. Hall" <christopher.s.hall@...el.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/2] arch, x86, tsc deadline clockevent dev: eliminate
 frequency roundoff error



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@...il.com>
> ---
>  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@...hat.com>

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

Paolo