reduce TSC deadline frequency errors

The v3 series can be found at

http://lkml.kernel.org/g/[email protected]

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 <[email protected]>, 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

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.

---
 arch/x86/kernel/apic/apic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 89a5bce..39517c0 100644
@@ -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

Thank you very much!

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

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.

---
 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(+)

index bc27611..971f446 100644
@@ -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

index 39517c0..c1ec995 100644
@@ -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

index 38ba6de..e0a6e20 100644
@@ -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