[PATCH v3 0/3] reduce TSC deadline frequency errors

[PATCH v3 0/3] reduce TSC deadline frequency errors

[Nicolai Stange]

This series is a split-off of the x86/TSC related patches from the
"avoid double timer interrupt with nohz and Intel TSC" v2 series
(http://lkml.kernel.org/g/20160710193047.18320-1-nicstange@gmail.com).

It turned out that the non-x86 related parts of the aforementioned series
are more than arguable and I don't want to bother you with any further
discussion of those. I apologize for any confusion this split might cause.

Applicable to linux-next-20160712. The individual patches don't depend on
each other.

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 (3):
  arch, x86, tsc deadline clockevent dev: reduce frequency roundoff
    error
  arch, x86, tsc deadline clockevent dev: reduce TSC_DIVISOR to 2
  arch, x86, tsc: inform TSC deadline clockevent device about
    recalibration

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

-- 
2.9.0

[PATCH v3 1/3] arch, x86, tsc deadline clockevent dev: reduce frequency roundoff error

[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.

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.

Increase the accuracy by reversing the order of the division and
multiplication. In order not to overflow during this calculation, cast
temporarily to u64.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 arch/x86/kernel/apic/apic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 89a5bce..dce654c 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -563,7 +563,8 @@ 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,
+						(u32)(((u64)tsc_khz * 1000) /
+							TSC_DIVISOR),
 						0xF, ~0UL);
 	} else
 		clockevents_register_device(levt);
-- 
2.9.0

Re: [PATCH v3 1/3] arch, x86, tsc deadline clockevent dev: reduce frequency roundoff error

[Peter Zijlstra]

On Wed, Jul 13, 2016 at 03:03:42PM +0200, Nicolai Stange wrote:

>  		clockevents_config_and_register(levt,
> +						(u32)(((u64)tsc_khz * 1000) /
> +							TSC_DIVISOR),
>  						0xF, ~0UL);

div_u64() perhaps ?

Re: [PATCH v3 1/3] arch, x86, tsc deadline clockevent dev: reduce frequency roundoff error

[Paolo Bonzini]

On 13/07/2016 15:49, Peter Zijlstra wrote:
> On Wed, Jul 13, 2016 at 03:03:42PM +0200, Nicolai Stange wrote:
> 
>>  		clockevents_config_and_register(levt,
>> +						(u32)(((u64)tsc_khz * 1000) /
>> +							TSC_DIVISOR),
>>  						0xF, ~0UL);
> 
> div_u64() perhaps ?

Or just squash together the two patches and do

	tsc_khz * (1000 / TSC_DIVISOR)

because with TSC_DIVISOR equal to 2/4/8 there is no error from
reassociating the operation.

Paolo

Re: [PATCH v3 1/3] arch, x86, tsc deadline clockevent dev: reduce frequency roundoff error

[Nicolai Stange]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 13/07/2016 15:49, Peter Zijlstra wrote:
>> On Wed, Jul 13, 2016 at 03:03:42PM +0200, Nicolai Stange wrote:
>> 
>>>  		clockevents_config_and_register(levt,
>>> +						(u32)(((u64)tsc_khz * 1000) /
>>> +							TSC_DIVISOR),
>>>  						0xF, ~0UL);
>> 
>> div_u64() perhaps ?
>
> Or just squash together the two patches and do
>
> 	tsc_khz * (1000 / TSC_DIVISOR)
>
> because with TSC_DIVISOR equal to 2/4/8 there is no error from
> reassociating the operation.

Oh great, I didn't see this. Will resend.

Thanks,

Nicolai

[PATCH v3 2/3] arch, x86, tsc deadline clockevent dev: reduce TSC_DIVISOR to 2

[Nicolai Stange]

In order to avoid overflowing an u32, the TSC deadline clockevent device's
frequency is divided by TSC_DIVISOR at registration.

The TSC_DIVISOR is currently defined as equaling 32 which allows for a
TSC frequency as high as 2^32 / 10^9ns * 32 = 137 GHz.

OTOH, larger values of TSC_DIVISOR introduce bigger roundoff errors into
the device's frequency.

A value of 2 for TSC_DIVISOR allows for a TSC frequency of
2^32 / 10^9ns * 2 = 8.5 GHz which is still way larger than anything to
expect in the next years.

Reduce the TSC deadline clockevent device's TSC_DIVISOR from 32 down to 2.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 arch/x86/kernel/apic/apic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index dce654c..1d22c72 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  2
 
 /*
  * This function sets up the local APIC timer, with a timeout of
-- 
2.9.0

[PATCH v3 3/3] arch, x86, tsc: inform TSC deadline clockevent device about recalibration

[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 | 25 +++++++++++++++++++++++++
 arch/x86/kernel/tsc.c       |  6 ++++++
 3 files changed, 32 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 1d22c72..cb133ac 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -571,6 +571,31 @@ 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,
+				(u32)(((u64)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 2a952fc..48455f0 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);
@@ -1255,6 +1256,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