linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/2] reduce TSC deadline frequency errors
@ 2016-07-14 15:22 Nicolai Stange
  2016-07-14 15:22 ` [PATCH v4 1/2] arch, x86, tsc deadline clockevent dev: eliminate frequency roundoff error Nicolai Stange
  2016-07-14 15:22 ` [PATCH v4 2/2] arch, x86, tsc: inform TSC deadline clockevent device about recalibration Nicolai Stange
  0 siblings, 2 replies; 7+ messages in thread
From: Nicolai Stange @ 2016-07-14 15:22 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, H. Peter Anvin, x86, Borislav Petkov, Paolo Bonzini,
	Viresh Kumar, Hidehiro Kawai, Peter Zijlstra (Intel),
	Christopher S. Hall, Adrian Hunter, linux-kernel, Nicolai Stange

The v3 series can be found at

  http://lkml.kernel.org/g/20160713130344.8319-1-nicstange@gmail.com

Applicable to linux-next-20160708 (in case you wonder why I turned back
from the 20160712 given in v3 to 20160708 again: mysteriously, 20160712
doesn't boot neither w/ nor w/o this series anymore).

The individual patches don't depend on each other.

Changes to v3:
  As Paolo Bonzini pointed out in reply to v3, for the values of 2, 4 and
  8 of TSC_DIVISOR, the 64 bit division
    (u32)(((u64)tsc_khz * 1000) / TSC_DIVISOR)
  can be safely turned into a 32 bit division by changing associativity:
    tsc_khz * (1000 / TSC_DIVISOR)

  In doing so, it suggests itself to squash former [1/3] ("arch, x86, tsc
  deadline clockevent dev: reduce frequency roundoff error") and
  [2/3] ("arch, x86, tsc deadline clockevent dev: reduce TSC_DIVISOR to 2")
  into a single patch. Furthermore, reducing TSC_DIVISOR down to 2 becomes
  unnessecary -- setting it to 8 suffices already.

  Thus,
  - [1/2] ("arch, x86, tsc deadline clockevent dev: eliminate frequency
            roundoff error")
      Former [1/3] and [2/3] squashed together. Don't reduce TSC_DIVISOR
      to 2 but to 8 only. Change associativity in order to get rid of the
      64 bit division. Adapt the commit message accordingly.
  - [2/2] ("arch, x86, tsc: inform TSC deadline clockevent device about
            recalibration")
      Former [3/3]. Likewise change associativity in order to get rid of
      the 64 bit division here, too.

Changes to v2:
  - [3/3] ("arch, x86, tsc: inform TSC deadline clockevent device about
            recalibration")
      Use clockevents_update_freq() rather than clockevents_config().

  - Former [4/4] ("kernel/time/clockevents: compensate for monotonic
                   clock's dynamic frequency")
      Split off, not a member of this series anymore.

Changes to v1:
  - [1/3] ("arch, x86, tsc deadline clockevent dev: reduce frequency
            roundoff error")
      No changes to the patch. Note that the v1 mail could not be delivered
      to the author of the TSC_DIVISOR introducing commit 279f1461432c
      ("x86: apic: Use tsc deadline for oneshot when available"),
      Suresh Siddha <suresh.b.siddha@intel.com>, so I had to remove him
      from the CC list.

  - [2/3] ("arch, x86, tsc deadline clockevent dev: reduce TSC_DIVISOR
            to 2")
      Likewise.

  - [3/3] ("arch, x86, tsc: inform TSC deadline clockevent device about
            recalibration")
      Silence the kbuild test robot on ARCH=i386 by wrapping the new call
      to lapic_update_tsc_freq() from arch/x86/kernel/tsc.c in an
      #ifdef CONFIG_X86_LOCAL_APIC.

Nicolai Stange (2):
  arch, x86, tsc deadline clockevent dev: eliminate frequency roundoff
    error
  arch, x86, tsc: inform TSC deadline clockevent device about
    recalibration

 arch/x86/include/asm/apic.h |  1 +
 arch/x86/kernel/apic/apic.c | 28 ++++++++++++++++++++++++++--
 arch/x86/kernel/tsc.c       |  6 ++++++
 3 files changed, 33 insertions(+), 2 deletions(-)

-- 
2.9.0

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v4 1/2] arch, x86, tsc deadline clockevent dev: eliminate frequency roundoff error
  2016-07-14 15:22 [PATCH v4 0/2] reduce TSC deadline frequency errors Nicolai Stange
@ 2016-07-14 15:22 ` Nicolai Stange
  2016-07-14 15:43   ` Paolo Bonzini
  2016-08-10 17:55   ` [tip:timers/urgent] x86/timers/apic: Fix imprecise timer interrupts by eliminating TSC clockevents " tip-bot for Nicolai Stange
  2016-07-14 15:22 ` [PATCH v4 2/2] arch, x86, tsc: inform TSC deadline clockevent device about recalibration Nicolai Stange
  1 sibling, 2 replies; 7+ messages in thread
From: Nicolai Stange @ 2016-07-14 15:22 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, H. Peter Anvin, x86, Borislav Petkov, Paolo Bonzini,
	Viresh Kumar, Hidehiro Kawai, Peter Zijlstra (Intel),
	Christopher S. Hall, Adrian Hunter, linux-kernel, Nicolai Stange

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@gmail.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);
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v4 2/2] arch, x86, tsc: inform TSC deadline clockevent device about recalibration
  2016-07-14 15:22 [PATCH v4 0/2] reduce TSC deadline frequency errors Nicolai Stange
  2016-07-14 15:22 ` [PATCH v4 1/2] arch, x86, tsc deadline clockevent dev: eliminate frequency roundoff error Nicolai Stange
@ 2016-07-14 15:22 ` Nicolai Stange
  2016-08-10 17:55   ` [tip:timers/urgent] x86/timers/apic: Inform " tip-bot for Nicolai Stange
  1 sibling, 1 reply; 7+ messages in thread
From: Nicolai Stange @ 2016-07-14 15:22 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, H. Peter Anvin, x86, Borislav Petkov, Paolo Bonzini,
	Viresh Kumar, Hidehiro Kawai, Peter Zijlstra (Intel),
	Christopher S. Hall, Adrian Hunter, linux-kernel, Nicolai Stange

The TSC deadline clockevent devices' configuration and registration
happens before the TSC frequency calibration is refined in
tsc_refine_calibration_work().

This results in the TSC clocksource and the TSC deadline clockevent
devices being configured with slightly different frequencies: the former
gets the refined one and the latter are configured with the inaccurate
frequency detected earlier by means of the
"Fast TSC calibration using PIT".

Within the APIC code, introduce the notifier function
lapic_update_tsc_freq() which reconfigures all per-CPU TSC deadline
clockevent devices with the current tsc_khz.

Call it from the TSC code after TSC calibration refinement has happened.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 arch/x86/include/asm/apic.h |  1 +
 arch/x86/kernel/apic/apic.c | 24 ++++++++++++++++++++++++
 arch/x86/kernel/tsc.c       |  6 ++++++
 3 files changed, 31 insertions(+)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index bc27611..971f446 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -135,6 +135,7 @@ extern void init_apic_mappings(void);
 void register_lapic_address(unsigned long address);
 extern void setup_boot_APIC_clock(void);
 extern void setup_secondary_APIC_clock(void);
+extern void lapic_update_tsc_freq(void);
 extern int APIC_init_uniprocessor(void);
 
 #ifdef CONFIG_X86_64
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 39517c0..c1ec995 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -570,6 +570,30 @@ static void setup_APIC_timer(void)
 }
 
 /*
+ * Install the updated TSC frequency from recalibration at the TSC
+ * deadline clockevent devices.
+ */
+static void __lapic_update_tsc_freq(void *info)
+{
+	struct clock_event_device *levt = this_cpu_ptr(&lapic_events);
+
+	if (!this_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER))
+		return;
+
+	clockevents_update_freq(levt, tsc_khz * (1000 / TSC_DIVISOR));
+}
+
+void lapic_update_tsc_freq(void)
+{
+	/*
+	 * The clockevent device's ->mult and ->shift can both be
+	 * changed. In order to avoid races, schedule the frequency
+	 * update code on each CPU.
+	 */
+	on_each_cpu(__lapic_update_tsc_freq, NULL, 0);
+}
+
+/*
  * In this functions we calibrate APIC bus clocks to the external timer.
  *
  * We want to do the calibration only once since we want to have local timer
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 38ba6de..e0a6e20 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -22,6 +22,7 @@
 #include <asm/nmi.h>
 #include <asm/x86_init.h>
 #include <asm/geode.h>
+#include <asm/apic.h>
 
 unsigned int __read_mostly cpu_khz;	/* TSC clocks / usec, not used here */
 EXPORT_SYMBOL(cpu_khz);
@@ -1193,6 +1194,11 @@ static void tsc_refine_calibration_work(struct work_struct *work)
 		(unsigned long)tsc_khz / 1000,
 		(unsigned long)tsc_khz % 1000);
 
+#ifdef CONFIG_X86_LOCAL_APIC
+	/* Inform the TSC deadline clockevent devices about the recalibration */
+	lapic_update_tsc_freq();
+#endif
+
 out:
 	if (boot_cpu_has(X86_FEATURE_ART))
 		art_related_clocksource = &clocksource_tsc;
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/2] arch, x86, tsc deadline clockevent dev: eliminate frequency roundoff error
  2016-07-14 15:22 ` [PATCH v4 1/2] arch, x86, tsc deadline clockevent dev: eliminate frequency roundoff error Nicolai Stange
@ 2016-07-14 15:43   ` Paolo Bonzini
  2016-07-16 21:08     ` Nicolai Stange
  2016-08-10 17:55   ` [tip:timers/urgent] x86/timers/apic: Fix imprecise timer interrupts by eliminating TSC clockevents " tip-bot for Nicolai Stange
  1 sibling, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2016-07-14 15:43 UTC (permalink / raw)
  To: Nicolai Stange, Thomas Gleixner
  Cc: Ingo Molnar, H. Peter Anvin, x86, Borislav Petkov, Viresh Kumar,
	Hidehiro Kawai, Peter Zijlstra (Intel),
	Christopher S. Hall, Adrian Hunter, linux-kernel



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@gmail.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@redhat.com>

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

Paolo

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/2] arch, x86, tsc deadline clockevent dev: eliminate frequency roundoff error
  2016-07-14 15:43   ` Paolo Bonzini
@ 2016-07-16 21:08     ` Nicolai Stange
  0 siblings, 0 replies; 7+ messages in thread
From: Nicolai Stange @ 2016-07-16 21:08 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Nicolai Stange, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Borislav Petkov, Viresh Kumar, Hidehiro Kawai,
	Peter Zijlstra (Intel),
	Christopher S. Hall, Adrian Hunter, linux-kernel

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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@gmail.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@redhat.com>

Thank you very much!


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

I've just collected both, the "coarse" TSC frequencies obtained through
the PIT calibration as well as the "refined" TSC frequencies across 1000
boots (CPU: Intel i7-4800MQ, Haswell).

My interpretation of the outcome is that the application of this patch
alone (i.e. w/o [2/2] "arch, x86, tsc: inform TSC deadline clockevent
device about recalibration") is not only worthless but has the potential
for making things worse.

1. Worthlessness
The "coarse" TSC frequency's mean is 2693.801 MHz with a standard
deviation of 122 kHz. Given that this patch changes the outcome by at
most 31kHz (15.5 kHz on average), the error corrected is relatively
small when compared to the standard deviation.

2. Potential for making things worse
Note that this patch never decreases frequency values. The roundoff
error fixed by this patch turns the "coarse" frequency mean of 2693.801
MHz into 2693.785 MHz. This errorneous value is significantly closer to
the "refined" frequency's mean of 2693.773 MHz (sdev: 5.8 kHz). Thus,
the two errors addressed by this series' two patches seem to be of
opposite direction on average and cancel each other to some extent
-> picking only one of the two makes the frequency bias worse on
average.


So, since it is unlikely that [2/2] ("arch, x86, tsc: inform TSC
deadline clockevent device about recalibration") is considered safe
enough for inclusion into -stable, I'd recommend not to send this
one (i.e. [1/2]) to stable either.


Thanks,

Nicolai

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [tip:timers/urgent] x86/timers/apic: Fix imprecise timer interrupts by eliminating TSC clockevents frequency roundoff error
  2016-07-14 15:22 ` [PATCH v4 1/2] arch, x86, tsc deadline clockevent dev: eliminate frequency roundoff error Nicolai Stange
  2016-07-14 15:43   ` Paolo Bonzini
@ 2016-08-10 17:55   ` tip-bot for Nicolai Stange
  1 sibling, 0 replies; 7+ messages in thread
From: tip-bot for Nicolai Stange @ 2016-08-10 17:55 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: bp, len.brown, torvalds, hidehiro.kawai.ez, christopher.s.hall,
	pbonzini, peterz, hpa, tglx, mingo, nicstange, linux-kernel,
	viresh.kumar, adrian.hunter

Commit-ID:  1a9e4c564ab174e53ed86def922804a5ddc63e7d
Gitweb:     http://git.kernel.org/tip/1a9e4c564ab174e53ed86def922804a5ddc63e7d
Author:     Nicolai Stange <nicstange@gmail.com>
AuthorDate: Thu, 14 Jul 2016 17:22:54 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 10 Aug 2016 12:37:38 +0200

x86/timers/apic: Fix imprecise timer interrupts by eliminating TSC clockevents frequency roundoff error

I noticed the following bug/misbehavior on certain Intel systems: with a
single task running on a NOHZ CPU on an Intel Haswell, I recognized
that I did not only get the one expected local_timer APIC interrupt, but
two per second at minimum. (!)

Further tracing showed that the first one precedes the programmed deadline
by up to ~50us and hence, it did nothing except for reprogramming the TSC
deadline clockevent device to trigger shortly thereafter again.

The reason for this is imprecise calibration, the timeout we program into
the APIC results in 'too short' timer interrupts. The core (hr)timer code
notices this (because it has a precise ktime source and sees the short
interrupt) and fixes it up by programming an additional very short
interrupt period.

This is obviously suboptimal.

The reason for the imprecise calibration is twofold, and this patch
fixes the first reason:

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.

To fix it, 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 we also 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@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Christopher S. Hall <christopher.s.hall@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Link: http://lkml.kernel.org/r/20160714152255.18295-2-nicstange@gmail.com
[ Improved changelog. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 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 ac8d8ad..a315dc4 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -313,7 +313,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
@@ -565,7 +565,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);

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [tip:timers/urgent] x86/timers/apic: Inform TSC deadline clockevent device about recalibration
  2016-07-14 15:22 ` [PATCH v4 2/2] arch, x86, tsc: inform TSC deadline clockevent device about recalibration Nicolai Stange
@ 2016-08-10 17:55   ` tip-bot for Nicolai Stange
  0 siblings, 0 replies; 7+ messages in thread
From: tip-bot for Nicolai Stange @ 2016-08-10 17:55 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: peterz, pbonzini, bp, christopher.s.hall, len.brown,
	adrian.hunter, hpa, hidehiro.kawai.ez, linux-kernel, tglx,
	nicstange, viresh.kumar, mingo, torvalds

Commit-ID:  6731b0d611a1274f9e785fa0189ac2aeeabd0591
Gitweb:     http://git.kernel.org/tip/6731b0d611a1274f9e785fa0189ac2aeeabd0591
Author:     Nicolai Stange <nicstange@gmail.com>
AuthorDate: Thu, 14 Jul 2016 17:22:55 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 10 Aug 2016 12:38:12 +0200

x86/timers/apic: Inform TSC deadline clockevent device about recalibration

This patch eliminates a source of imprecise APIC timer interrupts,
which imprecision may result in double interrupts or even late
interrupts.

The TSC deadline clockevent devices' configuration and registration
happens before the TSC frequency calibration is refined in
tsc_refine_calibration_work().

This results in the TSC clocksource and the TSC deadline clockevent
devices being configured with slightly different frequencies: the former
gets the refined one and the latter are configured with the inaccurate
frequency detected earlier by means of the "Fast TSC calibration using PIT".

Within the APIC code, introduce the notifier function
lapic_update_tsc_freq() which reconfigures all per-CPU TSC deadline
clockevent devices with the current tsc_khz.

Call it from the TSC code after TSC calibration refinement has happened.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Christopher S. Hall <christopher.s.hall@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Link: http://lkml.kernel.org/r/20160714152255.18295-3-nicstange@gmail.com
[ Pushed #ifdef CONFIG_X86_LOCAL_APIC into header, improved changelog. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/apic.h |  2 ++
 arch/x86/kernel/apic/apic.c | 24 ++++++++++++++++++++++++
 arch/x86/kernel/tsc.c       |  4 ++++
 3 files changed, 30 insertions(+)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index f5befd4..1243577 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -135,6 +135,7 @@ extern void init_apic_mappings(void);
 void register_lapic_address(unsigned long address);
 extern void setup_boot_APIC_clock(void);
 extern void setup_secondary_APIC_clock(void);
+extern void lapic_update_tsc_freq(void);
 extern int APIC_init_uniprocessor(void);
 
 #ifdef CONFIG_X86_64
@@ -170,6 +171,7 @@ static inline void init_apic_mappings(void) { }
 static inline void disable_local_APIC(void) { }
 # define setup_boot_APIC_clock x86_init_noop
 # define setup_secondary_APIC_clock x86_init_noop
+static inline void lapic_update_tsc_freq(void) { }
 #endif /* !CONFIG_X86_LOCAL_APIC */
 
 #ifdef CONFIG_X86_X2APIC
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index a315dc4..0fd3d65 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -572,6 +572,30 @@ static void setup_APIC_timer(void)
 }
 
 /*
+ * Install the updated TSC frequency from recalibration at the TSC
+ * deadline clockevent devices.
+ */
+static void __lapic_update_tsc_freq(void *info)
+{
+	struct clock_event_device *levt = this_cpu_ptr(&lapic_events);
+
+	if (!this_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER))
+		return;
+
+	clockevents_update_freq(levt, tsc_khz * (1000 / TSC_DIVISOR));
+}
+
+void lapic_update_tsc_freq(void)
+{
+	/*
+	 * The clockevent device's ->mult and ->shift can both be
+	 * changed. In order to avoid races, schedule the frequency
+	 * update code on each CPU.
+	 */
+	on_each_cpu(__lapic_update_tsc_freq, NULL, 0);
+}
+
+/*
  * In this functions we calibrate APIC bus clocks to the external timer.
  *
  * We want to do the calibration only once since we want to have local timer
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index a804b5a..8fb4b6a 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -22,6 +22,7 @@
 #include <asm/nmi.h>
 #include <asm/x86_init.h>
 #include <asm/geode.h>
+#include <asm/apic.h>
 
 unsigned int __read_mostly cpu_khz;	/* TSC clocks / usec, not used here */
 EXPORT_SYMBOL(cpu_khz);
@@ -1249,6 +1250,9 @@ static void tsc_refine_calibration_work(struct work_struct *work)
 		(unsigned long)tsc_khz / 1000,
 		(unsigned long)tsc_khz % 1000);
 
+	/* Inform the TSC deadline clockevent devices about the recalibration */
+	lapic_update_tsc_freq();
+
 out:
 	if (boot_cpu_has(X86_FEATURE_ART))
 		art_related_clocksource = &clocksource_tsc;

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-08-10 19:48 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-14 15:22 [PATCH v4 0/2] reduce TSC deadline frequency errors Nicolai Stange
2016-07-14 15:22 ` [PATCH v4 1/2] arch, x86, tsc deadline clockevent dev: eliminate frequency roundoff error Nicolai Stange
2016-07-14 15:43   ` Paolo Bonzini
2016-07-16 21:08     ` Nicolai Stange
2016-08-10 17:55   ` [tip:timers/urgent] x86/timers/apic: Fix imprecise timer interrupts by eliminating TSC clockevents " tip-bot for Nicolai Stange
2016-07-14 15:22 ` [PATCH v4 2/2] arch, x86, tsc: inform TSC deadline clockevent device about recalibration Nicolai Stange
2016-08-10 17:55   ` [tip:timers/urgent] x86/timers/apic: Inform " tip-bot for Nicolai Stange

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).