From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751530AbcGNPoD (ORCPT ); Thu, 14 Jul 2016 11:44:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36510 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751119AbcGNPoA (ORCPT ); Thu, 14 Jul 2016 11:44:00 -0400 Subject: Re: [PATCH v4 1/2] arch, x86, tsc deadline clockevent dev: eliminate frequency roundoff error To: Nicolai Stange , Thomas Gleixner References: <20160714152255.18295-1-nicstange@gmail.com> <20160714152255.18295-2-nicstange@gmail.com> Cc: Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Borislav Petkov , Viresh Kumar , Hidehiro Kawai , "Peter Zijlstra (Intel)" , "Christopher S. Hall" , Adrian Hunter , linux-kernel@vger.kernel.org From: Paolo Bonzini Message-ID: <14f0de3b-236d-20b3-ea8e-57e9cad03479@redhat.com> Date: Thu, 14 Jul 2016 17:43:53 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20160714152255.18295-2-nicstange@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 14 Jul 2016 15:43:59 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 and I suggest even CCing this to stable as it's pretty safe and an improvement. Paolo