[RFC PATCH] x86: Kill notsc

[RFC PATCH] x86: Kill notsc

[Borislav Petkov]

Ok,

let's try this and see where it takes us. Patch has been only lightly
tested in kvm - I'll hammer on it for real once we agree about the
general form.

Aanyway, this patch is something Peter and I have been talking about on
IRC a couple of times already. I finally found some free time to poke at
it, here's the result.

Thoughts?

---

Kill "notsc" cmdline option and all the glue around it. The two boxes
worldwide which don't have a TSC should disable X86_TSC. Thus, make
native_sched_clock() use TSC unconditionally, even if the TSC is
unstable because that's fine there. This gets rid of the static key
too and makes the function even simpler and faster, which is a Good
Thing(tm).

The jiffies-fallback is for the !X86_TSC case.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 Documentation/kernel-parameters.txt       |  2 -
 Documentation/x86/x86_64/boot-options.txt |  5 ---
 arch/x86/include/asm/tsc.h                |  3 +-
 arch/x86/kernel/apic/apic.c               |  2 +-
 arch/x86/kernel/tsc.c                     | 74 ++++++++-----------------------
 5 files changed, 21 insertions(+), 65 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 50fc09b623f6..2589559b7520 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2549,8 +2549,6 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 	nosync		[HW,M68K] Disables sync negotiation for all devices.
 
-	notsc		[BUGS=X86-32] Disable Time Stamp Counter
-
 	nousb		[USB] Disable the USB subsystem
 
 	nowatchdog	[KNL] Disable both lockup detectors, i.e.
diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
index 68ed3114c363..0e43d94d9567 100644
--- a/Documentation/x86/x86_64/boot-options.txt
+++ b/Documentation/x86/x86_64/boot-options.txt
@@ -88,11 +88,6 @@ APICs
 
 Timing
 
-  notsc
-  Don't use the CPU time stamp counter to read the wall time.
-  This can be used to work around timing problems on multiprocessor systems
-  with not properly synchronized CPUs.
-
   nohpet
   Don't use the HPET timer.
 
diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 6d7c5479bcea..aa628d0f4bb0 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -33,7 +33,6 @@ extern void tsc_init(void);
 extern void mark_tsc_unstable(char *reason);
 extern int unsynchronized_tsc(void);
 extern int check_tsc_unstable(void);
-extern int check_tsc_disabled(void);
 extern unsigned long native_calibrate_tsc(void);
 extern unsigned long long native_sched_clock_from_tsc(u64 tsc);
 
@@ -46,7 +45,7 @@ extern int tsc_clocksource_reliable;
 extern void check_tsc_sync_source(int cpu);
 extern void check_tsc_sync_target(void);
 
-extern int notsc_setup(char *);
+extern int notsc_setup(void);
 extern void tsc_save_sched_clock_state(void);
 extern void tsc_restore_sched_clock_state(void);
 
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 2f69e3b184f6..09bf96f48227 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -152,7 +152,7 @@ static int apic_calibrate_pmtmr __initdata;
 static __init int setup_apicpmtimer(char *s)
 {
 	apic_calibrate_pmtmr = 1;
-	notsc_setup(NULL);
+	notsc_setup();
 	return 0;
 }
 __setup("apicpmtimer", setup_apicpmtimer);
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 69b84a26ea17..6c4bc8dc1a62 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -34,13 +34,6 @@ EXPORT_SYMBOL(tsc_khz);
  */
 static int __read_mostly tsc_unstable;
 
-/* native_sched_clock() is called before tsc_init(), so
-   we must start with the TSC soft disabled to prevent
-   erroneous rdtsc usage on !cpu_has_tsc processors */
-static int __read_mostly tsc_disabled = -1;
-
-static DEFINE_STATIC_KEY_FALSE(__use_tsc);
-
 int tsc_clocksource_reliable;
 
 /*
@@ -273,24 +266,20 @@ done:
  */
 u64 native_sched_clock(void)
 {
-	if (static_branch_likely(&__use_tsc)) {
-		u64 tsc_now = rdtsc();
-
-		/* return the value in ns */
-		return cycles_2_ns(tsc_now);
-	}
-
+#ifdef CONFIG_X86_TSC
+	/* return the value in ns */
+	return cycles_2_ns(rdtsc());
+#else
 	/*
-	 * Fall back to jiffies if there's no TSC available:
-	 * ( But note that we still use it if the TSC is marked
-	 *   unstable. We do this because unlike Time Of Day,
-	 *   the scheduler clock tolerates small errors and it's
-	 *   very important for it to be as fast as the platform
-	 *   can achieve it. )
+	 * Fall back to jiffies if there's no TSC available: ( But note that we
+	 * still use it if the TSC is marked unstable. We do this because unlike
+	 * Time Of Day, the scheduler clock tolerates small errors and it's very
+	 * important for it to be as fast as the platform can achieve it. )
+	 *
+	 * No locking - a rare wrong value is not a big deal:
 	 */
-
-	/* No locking but a rare wrong value is not a big deal: */
 	return (jiffies_64 - INITIAL_JIFFIES) * (1000000000 / HZ);
+#endif
 }
 
 /*
@@ -319,32 +308,15 @@ int check_tsc_unstable(void)
 }
 EXPORT_SYMBOL_GPL(check_tsc_unstable);
 
-int check_tsc_disabled(void)
-{
-	return tsc_disabled;
-}
-EXPORT_SYMBOL_GPL(check_tsc_disabled);
-
-#ifdef CONFIG_X86_TSC
-int __init notsc_setup(char *str)
-{
-	pr_warn("Kernel compiled with CONFIG_X86_TSC, cannot disable TSC completely\n");
-	tsc_disabled = 1;
-	return 1;
-}
-#else
-/*
- * disable flag for tsc. Takes effect by clearing the TSC cpu flag
- * in cpu/common.c
- */
-int __init notsc_setup(char *str)
+/* Disable the TSC feature flag to avoid further TSC use. */
+int __init notsc_setup(void)
 {
+#ifndef CONFIG_X86_TSC
 	setup_clear_cpu_cap(X86_FEATURE_TSC);
 	return 1;
-}
 #endif
-
-__setup("notsc", notsc_setup);
+	return 0;
+}
 
 static int no_sched_irq_time;
 
@@ -1137,7 +1109,7 @@ out:
 
 static int __init init_tsc_clocksource(void)
 {
-	if (!cpu_has_tsc || tsc_disabled > 0 || !tsc_khz)
+	if (!cpu_has_tsc || !tsc_khz)
 		return 0;
 
 	if (tsc_clocksource_reliable)
@@ -1176,7 +1148,7 @@ void __init tsc_init(void)
 
 	x86_init.timers.tsc_pre_init();
 
-	if (!cpu_has_tsc) {
+	if (notsc_setup()) {
 		setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
 		return;
 	}
@@ -1205,14 +1177,6 @@ void __init tsc_init(void)
 		set_cyc2ns_scale(cpu_khz, cpu);
 	}
 
-	if (tsc_disabled > 0)
-		return;
-
-	/* now allow native_sched_clock() to use rdtsc */
-
-	tsc_disabled = 0;
-	static_branch_enable(&__use_tsc);
-
 	if (!no_sched_irq_time)
 		enable_sched_clock_irqtime();
 
@@ -1239,7 +1203,7 @@ unsigned long calibrate_delay_is_known(void)
 {
 	int i, cpu = smp_processor_id();
 
-	if (!tsc_disabled && !cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC))
+	if (!cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC))
 		return 0;
 
 	for_each_online_cpu(i)
-- 
2.3.5


-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [RFC PATCH] x86: Kill notsc

[Andy Lutomirski]

On Sun, Oct 18, 2015 at 7:20 AM, Borislav Petkov <bp@suse.de> wrote:
> Ok,
>
> let's try this and see where it takes us. Patch has been only lightly
> tested in kvm - I'll hammer on it for real once we agree about the
> general form.
>
> Aanyway, this patch is something Peter and I have been talking about on
> IRC a couple of times already. I finally found some free time to poke at
> it, here's the result.
>
> Thoughts?

I'm a bit confused.  Is it currently the case that, if you boot a
normal kernel on a TSC-less machine without notsc, it fails?

--Andy

Re: [RFC PATCH] x86: Kill notsc

[Borislav Petkov]

On Sun, Oct 18, 2015 at 09:47:21PM -0700, Andy Lutomirski wrote:
> I'm a bit confused.  Is it currently the case that, if you boot a
> normal kernel on a TSC-less machine without notsc, it fails?

No, it shouldn't fail because we're checking CPUID flags.

The point of this patch is purely cleaning up stuff and getting rid
of all the notsc/tsc_disabled logic. What is more tempting, however,
is getting rid of the static key in native_sched_clock. It is almost
pointless complexity and we don't really need it.

AFAIK, only some early 486s and 586s don't have TSC and they can disable
CONFIG_X86_TSC. Which they do, actually, if you choose M486 or M586 in
Kconfig.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [RFC PATCH] x86: Kill notsc

[Borislav Petkov]

On Sun, Oct 18, 2015 at 04:20:07PM +0200, Borislav Petkov wrote:
> Ok,
> 
> let's try this and see where it takes us. Patch has been only lightly
> tested in kvm - I'll hammer on it for real once we agree about the
> general form.
> 
> Aanyway, this patch is something Peter and I have been talking about on
> IRC a couple of times already. I finally found some free time to poke at
> it, here's the result.
> 
> Thoughts?
> 
> ---
> 
> Kill "notsc" cmdline option and all the glue around it. The two boxes
> worldwide which don't have a TSC should disable X86_TSC. Thus, make
> native_sched_clock() use TSC unconditionally, even if the TSC is
> unstable because that's fine there. This gets rid of the static key
> too and makes the function even simpler and faster, which is a Good
> Thing(tm).
> 
> The jiffies-fallback is for the !X86_TSC case.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  Documentation/kernel-parameters.txt       |  2 -
>  Documentation/x86/x86_64/boot-options.txt |  5 ---
>  arch/x86/include/asm/tsc.h                |  3 +-
>  arch/x86/kernel/apic/apic.c               |  2 +-
>  arch/x86/kernel/tsc.c                     | 74 ++++++++-----------------------
>  5 files changed, 21 insertions(+), 65 deletions(-)

...

> +#ifdef CONFIG_X86_TSC
> +	/* return the value in ns */
> +	return cycles_2_ns(rdtsc());
> +#else

Ok, we have a problem here:

See the splat below. On the init path

start_kernel
|-> sched_init
    |-> init_idle

we're calling sched_clock() which does the cycles_2_ns() thing.

However, that cycles_2_ns() thing gets called only in tsc_init() which
comes later in start_kernel().

Which means, data in this line

      ns += mul_u64_u32_shr(cyc, data->cyc2ns_mul, data->cyc2ns_shift);

is NULL.

Now, the question is, can I push only the cyc2ns_init() call up, before
sched_init()?

I mean, it dummy initializes only the per-cpu variable and tsc_init()
will correct it anyway, as the comment says:

        /*
         * Secondary CPUs do not run through tsc_init(), so set up
         * all the scale factors for all CPUs, assuming the same
         * speed as the bootup CPU. (cpufreq notifiers will fix this
         * up if their speed diverges)
         */
        for_each_possible_cpu(cpu) {
                cyc2ns_init(cpu);
                set_cyc2ns_scale(cpu_khz, cpu);
        }


Thoughts?


[    0.000000] BUG: unable to handle kernel NULL pointer dereference at           (null)
[    0.000000] IP: [<ffffffff8100d19d>] native_sched_clock+0x2d/0x90
[    0.000000] PGD 0 
[    0.000000] Oops: 0000 [#1] PREEMPT SMP 
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.3.0-rc6+ #4
[    0.000000] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[    0.000000] task: ffffffff819f7500 ti: ffffffff819e8000 task.ti: ffffffff819e8000
[    0.000000] RIP: 0010:[<ffffffff8100d19d>]  [<ffffffff8100d19d>] native_sched_clock+0x2d/0x90
[    0.000000] RSP: 0000:ffffffff819ebee0  EFLAGS: 00010046
[    0.000000] RAX: 0000000129f79c5a RBX: ffff88007b7d5c00 RCX: 0000000000000000
[    0.000000] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff819f76e0
[    0.000000] RBP: ffffffff819ebee0 R08: 0000000000000001 R09: 0000000000000000
[    0.000000] R10: 0000000000000001 R11: ffffffff819f7500 R12: 0000000000000000
[    0.000000] R13: 0000000000000000 R14: ffffffff819f7b30 R15: ffffffff819f7500
[    0.000000] FS:  0000000000000000(0000) GS:ffff88007b600000(0000) knlGS:0000000000000000
[    0.000000] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[    0.000000] CR2: 0000000000000000 CR3: 00000000019f2000 CR4: 00000000000406b0
[    0.000000] Stack:
[    0.000000]  ffffffff819ebf28 ffffffff8108622f 0000000000000000 0000000000000086
[    0.000000]  ffffffff819f7500 0000000000000008 0000000000000008 00000000001d5c00
[    0.000000]  ffffffff0000ffff ffffffff819ebf58 ffffffff81cace4f ffffffffffffffff
[    0.000000] Call Trace:
[    0.000000]  [<ffffffff8108622f>] init_idle+0x13f/0x2d0
[    0.000000]  [<ffffffff81cace4f>] sched_init+0x304/0x345
[    0.000000]  [<ffffffff81c97d6d>] start_kernel+0x25e/0x417
[    0.000000]  [<ffffffff81c972fe>] x86_64_start_reservations+0x2a/0x2c
[    0.000000]  [<ffffffff81c97468>] x86_64_start_kernel+0x168/0x176
[    0.000000] Code: 89 e5 48 83 e4 f0 0f 31 48 c1 e2 20 48 09 d0 65 ff 05 88 dd ff 7e 65 48 8b 35 e0 5b 1c 7f 65 48 8b 15 e0 5b 1c 7f 48 39 d6 75 2c <8b> 16 8b 4e 04 48 f7 e2 48 0f ad d0 48 d3 ea f6 c1 40 48 0f 45 
[    0.000000] RIP  [<ffffffff8100d19d>] native_sched_clock+0x2d/0x90
[    0.000000]  RSP <ffffffff819ebee0>
[    0.000000] CR2: 0000000000000000
[    0.000000] ---[ end trace aff58b1304cfecf1 ]---
[    0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
[    0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [RFC PATCH] x86: Kill notsc

[Peter Zijlstra]

On Wed, Oct 21, 2015 at 07:58:03PM +0200, Borislav Petkov wrote:
> > +#ifdef CONFIG_X86_TSC
> > +	/* return the value in ns */
> > +	return cycles_2_ns(rdtsc());
> > +#else
> 
> Ok, we have a problem here:
> 
> See the splat below. On the init path
> 
> start_kernel
> |-> sched_init
>     |-> init_idle
> 
> we're calling sched_clock() which does the cycles_2_ns() thing.
> 
> However, that cycles_2_ns() thing gets called only in tsc_init() which
> comes later in start_kernel().
> 
> Which means, data in this line
> 
>       ns += mul_u64_u32_shr(cyc, data->cyc2ns_mul, data->cyc2ns_shift);
> 
> is NULL.
> 
> Now, the question is, can I push only the cyc2ns_init() call up, before
> sched_init()?

I _think_ so, but its late. It would result in sched_clock() being 0
until you hit that other bit, but that should be fine (maybe).

[PATCH -v2] x86: Kill notsc

[Borislav Petkov]

On Wed, Oct 21, 2015 at 09:01:09PM +0200, Peter Zijlstra wrote:
> I _think_ so, but its late. It would result in sched_clock() being 0
> until you hit that other bit, but that should be fine (maybe).

Ok, here's a v2, it boots fine here:

---
From: Borislav Petkov <bp@suse.de>
Date: Sun, 18 Oct 2015 16:05:32 +0200
Subject: [PATCH -v2] x86: Kill notsc

Kill "notsc" cmdline option and all the glue around it. The two boxes
worldwide which don't have a TSC should disable X86_TSC. Thus, make
native_sched_clock() use TSC unconditionally, even if the TSC is
unstable because that's fine there. This gets rid of the static key
too and makes the function even simpler and faster, which is a Good
Thing(tm).

The jiffies-fallback is for the !X86_TSC case.

Also, we need to initialize the cycles to nanoseconds machinery early,
before sched_init() -> init_idle() calls native_sched_clock() and we
explode, see link below.

Signed-off-by: Borislav Petkov <bp@suse.de>
Link: http://lkml.kernel.org/r/20151021175803.GF3575@pd.tnic
---
 Documentation/kernel-parameters.txt       |  2 -
 Documentation/x86/x86_64/boot-options.txt |  5 --
 arch/x86/include/asm/tsc.h                |  4 +-
 arch/x86/kernel/apic/apic.c               |  2 +-
 arch/x86/kernel/setup.c                   |  1 +
 arch/x86/kernel/tsc.c                     | 91 +++++++++++--------------------
 6 files changed, 37 insertions(+), 68 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 22a4b687ea5b..6d2504bd17ce 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2534,8 +2534,6 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 	nosync		[HW,M68K] Disables sync negotiation for all devices.
 
-	notsc		[BUGS=X86-32] Disable Time Stamp Counter
-
 	nousb		[USB] Disable the USB subsystem
 
 	nowatchdog	[KNL] Disable both lockup detectors, i.e.
diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
index 68ed3114c363..0e43d94d9567 100644
--- a/Documentation/x86/x86_64/boot-options.txt
+++ b/Documentation/x86/x86_64/boot-options.txt
@@ -88,11 +88,6 @@ APICs
 
 Timing
 
-  notsc
-  Don't use the CPU time stamp counter to read the wall time.
-  This can be used to work around timing problems on multiprocessor systems
-  with not properly synchronized CPUs.
-
   nohpet
   Don't use the HPET timer.
 
diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 6d7c5479bcea..41fe8a08b497 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -30,10 +30,10 @@ static inline cycles_t get_cycles(void)
 }
 
 extern void tsc_init(void);
+extern void __init early_tsc_init(void);
 extern void mark_tsc_unstable(char *reason);
 extern int unsynchronized_tsc(void);
 extern int check_tsc_unstable(void);
-extern int check_tsc_disabled(void);
 extern unsigned long native_calibrate_tsc(void);
 extern unsigned long long native_sched_clock_from_tsc(u64 tsc);
 
@@ -46,7 +46,7 @@ extern int tsc_clocksource_reliable;
 extern void check_tsc_sync_source(int cpu);
 extern void check_tsc_sync_target(void);
 
-extern int notsc_setup(char *);
+extern int notsc_setup(void);
 extern void tsc_save_sched_clock_state(void);
 extern void tsc_restore_sched_clock_state(void);
 
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 24e94ce454e2..19df3d640c15 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -152,7 +152,7 @@ static int apic_calibrate_pmtmr __initdata;
 static __init int setup_apicpmtimer(char *s)
 {
 	apic_calibrate_pmtmr = 1;
-	notsc_setup(NULL);
+	notsc_setup();
 	return 0;
 }
 __setup("apicpmtimer", setup_apicpmtimer);
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index fdb7f2a2d328..a8e2ab8cad5b 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1234,6 +1234,7 @@ void __init setup_arch(char **cmdline_p)
 	if (efi_enabled(EFI_BOOT))
 		efi_apply_memmap_quirks();
 #endif
+	early_tsc_init();
 }
 
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index c3f7602cd038..6211df475426 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -34,13 +34,6 @@ EXPORT_SYMBOL(tsc_khz);
  */
 static int __read_mostly tsc_unstable;
 
-/* native_sched_clock() is called before tsc_init(), so
-   we must start with the TSC soft disabled to prevent
-   erroneous rdtsc usage on !cpu_has_tsc processors */
-static int __read_mostly tsc_disabled = -1;
-
-static DEFINE_STATIC_KEY_FALSE(__use_tsc);
-
 int tsc_clocksource_reliable;
 
 /*
@@ -270,29 +263,38 @@ done:
 	sched_clock_idle_wakeup_event(0);
 	local_irq_restore(flags);
 }
+
+void __init early_tsc_init(void)
+{
+	int cpu;
+
+	/*
+	 * We need to init the cycles to ns conversion machinery because
+	 * init_idle() below will call sched_clock() which needs it.
+	 */
+	for_each_possible_cpu(cpu)
+		cyc2ns_init(cpu);
+}
+
 /*
  * Scheduler clock - returns current time in nanosec units.
  */
 u64 native_sched_clock(void)
 {
-	if (static_branch_likely(&__use_tsc)) {
-		u64 tsc_now = rdtsc();
-
-		/* return the value in ns */
-		return cycles_2_ns(tsc_now);
-	}
-
+#ifdef CONFIG_X86_TSC
+	/* return the value in ns */
+	return cycles_2_ns(rdtsc());
+#else
 	/*
-	 * Fall back to jiffies if there's no TSC available:
-	 * ( But note that we still use it if the TSC is marked
-	 *   unstable. We do this because unlike Time Of Day,
-	 *   the scheduler clock tolerates small errors and it's
-	 *   very important for it to be as fast as the platform
-	 *   can achieve it. )
+	 * Fall back to jiffies if there's no TSC available: ( But note that we
+	 * still use it if the TSC is marked unstable. We do this because unlike
+	 * Time Of Day, the scheduler clock tolerates small errors and it's very
+	 * important for it to be as fast as the platform can achieve it. )
+	 *
+	 * No locking - a rare wrong value is not a big deal:
 	 */
-
-	/* No locking but a rare wrong value is not a big deal: */
 	return (jiffies_64 - INITIAL_JIFFIES) * (1000000000 / HZ);
+#endif
 }
 
 /*
@@ -321,32 +323,15 @@ int check_tsc_unstable(void)
 }
 EXPORT_SYMBOL_GPL(check_tsc_unstable);
 
-int check_tsc_disabled(void)
-{
-	return tsc_disabled;
-}
-EXPORT_SYMBOL_GPL(check_tsc_disabled);
-
-#ifdef CONFIG_X86_TSC
-int __init notsc_setup(char *str)
-{
-	pr_warn("Kernel compiled with CONFIG_X86_TSC, cannot disable TSC completely\n");
-	tsc_disabled = 1;
-	return 1;
-}
-#else
-/*
- * disable flag for tsc. Takes effect by clearing the TSC cpu flag
- * in cpu/common.c
- */
-int __init notsc_setup(char *str)
+/* Disable the TSC feature flag to avoid further TSC use. */
+int __init notsc_setup(void)
 {
+#ifndef CONFIG_X86_TSC
 	setup_clear_cpu_cap(X86_FEATURE_TSC);
 	return 1;
-}
 #endif
-
-__setup("notsc", notsc_setup);
+	return 0;
+}
 
 static int no_sched_irq_time;
 
@@ -1139,7 +1124,7 @@ out:
 
 static int __init init_tsc_clocksource(void)
 {
-	if (!cpu_has_tsc || tsc_disabled > 0 || !tsc_khz)
+	if (!cpu_has_tsc || !tsc_khz)
 		return 0;
 
 	if (tsc_clocksource_reliable)
@@ -1178,7 +1163,7 @@ void __init tsc_init(void)
 
 	x86_init.timers.tsc_pre_init();
 
-	if (!cpu_has_tsc) {
+	if (notsc_setup()) {
 		setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
 		return;
 	}
@@ -1202,18 +1187,8 @@ void __init tsc_init(void)
 	 * speed as the bootup CPU. (cpufreq notifiers will fix this
 	 * up if their speed diverges)
 	 */
-	for_each_possible_cpu(cpu) {
-		cyc2ns_init(cpu);
+	for_each_possible_cpu(cpu)
 		set_cyc2ns_scale(cpu_khz, cpu);
-	}
-
-	if (tsc_disabled > 0)
-		return;
-
-	/* now allow native_sched_clock() to use rdtsc */
-
-	tsc_disabled = 0;
-	static_branch_enable(&__use_tsc);
 
 	if (!no_sched_irq_time)
 		enable_sched_clock_irqtime();
@@ -1241,7 +1216,7 @@ unsigned long calibrate_delay_is_known(void)
 {
 	int i, cpu = smp_processor_id();
 
-	if (!tsc_disabled && !cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC))
+	if (!cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC))
 		return 0;
 
 	for_each_online_cpu(i)
-- 
2.3.5

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH -v2] x86: Kill notsc

[Thomas Gleixner]

On Thu, 22 Oct 2015, Borislav Petkov wrote:
>  u64 native_sched_clock(void)
>  {
> -	if (static_branch_likely(&__use_tsc)) {
> -		u64 tsc_now = rdtsc();
> -
> -		/* return the value in ns */
> -		return cycles_2_ns(tsc_now);
> -	}
> -
> +#ifdef CONFIG_X86_TSC
> +	/* return the value in ns */
> +	return cycles_2_ns(rdtsc());
> +#else
>  	/*
> -	 * Fall back to jiffies if there's no TSC available:
> -	 * ( But note that we still use it if the TSC is marked
> -	 *   unstable. We do this because unlike Time Of Day,
> -	 *   the scheduler clock tolerates small errors and it's
> -	 *   very important for it to be as fast as the platform
> -	 *   can achieve it. )
> +	 * Fall back to jiffies if there's no TSC available: ( But note that we
> +	 * still use it if the TSC is marked unstable. We do this because unlike
> +	 * Time Of Day, the scheduler clock tolerates small errors and it's very
> +	 * important for it to be as fast as the platform can achieve it. )

This comment does not make any sense with this modification.

Thanks,

	tglx

Re: [PATCH -v2] x86: Kill notsc

[Borislav Petkov]

On Wed, Nov 04, 2015 at 11:21:45AM +0100, Thomas Gleixner wrote:
> On Thu, 22 Oct 2015, Borislav Petkov wrote:
> >  u64 native_sched_clock(void)
> >  {
> > -	if (static_branch_likely(&__use_tsc)) {
> > -		u64 tsc_now = rdtsc();
> > -
> > -		/* return the value in ns */
> > -		return cycles_2_ns(tsc_now);
> > -	}
> > -
> > +#ifdef CONFIG_X86_TSC
> > +	/* return the value in ns */
> > +	return cycles_2_ns(rdtsc());
> > +#else
> >  	/*
> > -	 * Fall back to jiffies if there's no TSC available:
> > -	 * ( But note that we still use it if the TSC is marked
> > -	 *   unstable. We do this because unlike Time Of Day,
> > -	 *   the scheduler clock tolerates small errors and it's
> > -	 *   very important for it to be as fast as the platform
> > -	 *   can achieve it. )
> > +	 * Fall back to jiffies if there's no TSC available: ( But note that we
> > +	 * still use it if the TSC is marked unstable. We do this because unlike
> > +	 * Time Of Day, the scheduler clock tolerates small errors and it's very
> > +	 * important for it to be as fast as the platform can achieve it. )
> 
> This comment does not make any sense with this modification.

Ok.

My intention was to keep the aspect that we still can use the TSC here,
even if it is marked unstable. I'll move it over rdtsc().

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[RFC PATCH -v2.1] x86: Kill notsc

[Borislav Petkov]

Here's v2.1 with comments fixed. I still need to run it through the
boxes to see what breaks, thus the RFC.

---
From: Borislav Petkov <bp@suse.de>
Date: Sun, 18 Oct 2015 16:05:32 +0200
Subject: [RFC PATCH -v2.1] x86: Kill notsc

Kill "notsc" cmdline option and all the glue around it. The two boxes
worldwide which don't have a TSC should disable X86_TSC. Thus, make
native_sched_clock() use TSC unconditionally, even if the TSC is
unstable because that's fine there. This gets rid of the static key
too and makes the function even simpler and faster, which is a Good
Thing(tm).

The jiffies-fallback is for the !X86_TSC case.

Also, we need to initialize the cycles to nanoseconds machinery early,
before sched_init() -> init_idle() calls native_sched_clock() and we
explode, see link below.

Signed-off-by: Borislav Petkov <bp@suse.de>
Link: http://lkml.kernel.org/r/20151021175803.GF3575@pd.tnic
---

 Documentation/kernel-parameters.txt       |  2 -
 Documentation/x86/x86_64/boot-options.txt |  5 --
 arch/x86/include/asm/tsc.h                |  4 +-
 arch/x86/kernel/apic/apic.c               |  2 +-
 arch/x86/kernel/setup.c                   |  2 +
 arch/x86/kernel/tsc.c                     | 89 +++++++++++--------------------
 6 files changed, 36 insertions(+), 68 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index f8aae632f02f..b91e6c8cccab 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2576,8 +2576,6 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 	nosync		[HW,M68K] Disables sync negotiation for all devices.
 
-	notsc		[BUGS=X86-32] Disable Time Stamp Counter
-
 	nousb		[USB] Disable the USB subsystem
 
 	nowatchdog	[KNL] Disable both lockup detectors, i.e.
diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
index 68ed3114c363..0e43d94d9567 100644
--- a/Documentation/x86/x86_64/boot-options.txt
+++ b/Documentation/x86/x86_64/boot-options.txt
@@ -88,11 +88,6 @@ APICs
 
 Timing
 
-  notsc
-  Don't use the CPU time stamp counter to read the wall time.
-  This can be used to work around timing problems on multiprocessor systems
-  with not properly synchronized CPUs.
-
   nohpet
   Don't use the HPET timer.
 
diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 6d7c5479bcea..41fe8a08b497 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -30,10 +30,10 @@ static inline cycles_t get_cycles(void)
 }
 
 extern void tsc_init(void);
+extern void __init early_tsc_init(void);
 extern void mark_tsc_unstable(char *reason);
 extern int unsynchronized_tsc(void);
 extern int check_tsc_unstable(void);
-extern int check_tsc_disabled(void);
 extern unsigned long native_calibrate_tsc(void);
 extern unsigned long long native_sched_clock_from_tsc(u64 tsc);
 
@@ -46,7 +46,7 @@ extern int tsc_clocksource_reliable;
 extern void check_tsc_sync_source(int cpu);
 extern void check_tsc_sync_target(void);
 
-extern int notsc_setup(char *);
+extern int notsc_setup(void);
 extern void tsc_save_sched_clock_state(void);
 extern void tsc_restore_sched_clock_state(void);
 
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 2f69e3b184f6..09bf96f48227 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -152,7 +152,7 @@ static int apic_calibrate_pmtmr __initdata;
 static __init int setup_apicpmtimer(char *s)
 {
 	apic_calibrate_pmtmr = 1;
-	notsc_setup(NULL);
+	notsc_setup();
 	return 0;
 }
 __setup("apicpmtimer", setup_apicpmtimer);
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 29db25f9a745..3dc9b6a07618 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1252,6 +1252,8 @@ void __init setup_arch(char **cmdline_p)
 #endif
 
 	microcode_init();
+
+	early_tsc_init();
 }
 
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index c7c4d9c51e99..744b4b59656f 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -34,13 +34,6 @@ EXPORT_SYMBOL(tsc_khz);
  */
 static int __read_mostly tsc_unstable;
 
-/* native_sched_clock() is called before tsc_init(), so
-   we must start with the TSC soft disabled to prevent
-   erroneous rdtsc usage on !cpu_has_tsc processors */
-static int __read_mostly tsc_disabled = -1;
-
-static DEFINE_STATIC_KEY_FALSE(__use_tsc);
-
 int tsc_clocksource_reliable;
 
 /*
@@ -279,29 +272,36 @@ done:
 	sched_clock_idle_wakeup_event(0);
 	local_irq_restore(flags);
 }
+
+void __init early_tsc_init(void)
+{
+	int cpu;
+
+	/*
+	 * We need to init the cycles to ns conversion machinery because
+	 * init_idle() below will call sched_clock() which needs it.
+	 */
+	for_each_possible_cpu(cpu)
+		cyc2ns_init(cpu);
+}
+
 /*
  * Scheduler clock - returns current time in nanosec units.
  */
 u64 native_sched_clock(void)
 {
-	if (static_branch_likely(&__use_tsc)) {
-		u64 tsc_now = rdtsc();
-
-		/* return the value in ns */
-		return cycles_2_ns(tsc_now);
-	}
-
+#ifdef CONFIG_X86_TSC
 	/*
-	 * Fall back to jiffies if there's no TSC available:
-	 * ( But note that we still use it if the TSC is marked
-	 *   unstable. We do this because unlike Time Of Day,
-	 *   the scheduler clock tolerates small errors and it's
-	 *   very important for it to be as fast as the platform
-	 *   can achieve it. )
+	 * Return the value in ns. Note that we still use the TSC even if it is
+	 * marked unstable. We do this because unlike Time Of Day, the scheduler
+	 * clock tolerates small errors and it's very important for it to be as
+	 * fast as the platform can achieve it.
 	 */
-
-	/* No locking but a rare wrong value is not a big deal: */
+	return cycles_2_ns(rdtsc());
+#else
+	/* No locking - a rare wrong value is not a big deal: */
 	return (jiffies_64 - INITIAL_JIFFIES) * (1000000000 / HZ);
+#endif
 }
 
 /*
@@ -330,32 +330,15 @@ int check_tsc_unstable(void)
 }
 EXPORT_SYMBOL_GPL(check_tsc_unstable);
 
-int check_tsc_disabled(void)
-{
-	return tsc_disabled;
-}
-EXPORT_SYMBOL_GPL(check_tsc_disabled);
-
-#ifdef CONFIG_X86_TSC
-int __init notsc_setup(char *str)
-{
-	pr_warn("Kernel compiled with CONFIG_X86_TSC, cannot disable TSC completely\n");
-	tsc_disabled = 1;
-	return 1;
-}
-#else
-/*
- * disable flag for tsc. Takes effect by clearing the TSC cpu flag
- * in cpu/common.c
- */
-int __init notsc_setup(char *str)
+/* Disable the TSC feature flag to avoid further TSC use. */
+int __init notsc_setup(void)
 {
+#ifndef CONFIG_X86_TSC
 	setup_clear_cpu_cap(X86_FEATURE_TSC);
 	return 1;
-}
 #endif
-
-__setup("notsc", notsc_setup);
+	return 0;
+}
 
 static int no_sched_irq_time;
 
@@ -1148,7 +1131,7 @@ out:
 
 static int __init init_tsc_clocksource(void)
 {
-	if (!cpu_has_tsc || tsc_disabled > 0 || !tsc_khz)
+	if (!cpu_has_tsc || !tsc_khz)
 		return 0;
 
 	if (tsc_clocksource_reliable)
@@ -1187,7 +1170,7 @@ void __init tsc_init(void)
 
 	x86_init.timers.tsc_pre_init();
 
-	if (!cpu_has_tsc) {
+	if (notsc_setup()) {
 		setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
 		return;
 	}
@@ -1211,18 +1194,8 @@ void __init tsc_init(void)
 	 * speed as the bootup CPU. (cpufreq notifiers will fix this
 	 * up if their speed diverges)
 	 */
-	for_each_possible_cpu(cpu) {
-		cyc2ns_init(cpu);
+	for_each_possible_cpu(cpu)
 		set_cyc2ns_scale(cpu_khz, cpu);
-	}
-
-	if (tsc_disabled > 0)
-		return;
-
-	/* now allow native_sched_clock() to use rdtsc */
-
-	tsc_disabled = 0;
-	static_branch_enable(&__use_tsc);
 
 	if (!no_sched_irq_time)
 		enable_sched_clock_irqtime();
@@ -1250,7 +1223,7 @@ unsigned long calibrate_delay_is_known(void)
 {
 	int i, cpu = smp_processor_id();
 
-	if (!tsc_disabled && !cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC))
+	if (!cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC))
 		return 0;
 
 	for_each_online_cpu(i)
-- 
2.3.5


-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [RFC PATCH -v2.1] x86: Kill notsc

[Thomas Gleixner]

On Mon, 16 Nov 2015, Borislav Petkov wrote:
> -/*
> - * disable flag for tsc. Takes effect by clearing the TSC cpu flag
> - * in cpu/common.c
> - */
> -int __init notsc_setup(char *str)
> +/* Disable the TSC feature flag to avoid further TSC use. */
> +int __init notsc_setup(void)
>  {
> +#ifndef CONFIG_X86_TSC
>  	setup_clear_cpu_cap(X86_FEATURE_TSC);

This is silly, really.

If CONFIG_X86_TSC is disabled then we should just not compile tsc.c at
all and map cpu_has_tsc and stuff depending on it to false.

Thanks,

	tglx

Re: [RFC PATCH -v2.1] x86: Kill notsc

[H. Peter Anvin]

On 11/16/15 13:25, Thomas Gleixner wrote:
> On Mon, 16 Nov 2015, Borislav Petkov wrote:
>> -/*
>> - * disable flag for tsc. Takes effect by clearing the TSC cpu flag
>> - * in cpu/common.c
>> - */
>> -int __init notsc_setup(char *str)
>> +/* Disable the TSC feature flag to avoid further TSC use. */
>> +int __init notsc_setup(void)
>>  {
>> +#ifndef CONFIG_X86_TSC
>>  	setup_clear_cpu_cap(X86_FEATURE_TSC);
> 
> This is silly, really.
> 
> If CONFIG_X86_TSC is disabled then we should just not compile tsc.c at
> all and map cpu_has_tsc and stuff depending on it to false.
> 

CONFIG_X86_TSC means TSC is obligatory, not that it is supported.

	-hpa

Re: [RFC PATCH -v2.1] x86: Kill notsc

[Borislav Petkov]

On Mon, Nov 16, 2015 at 09:02:15PM -0800, H. Peter Anvin wrote:
> On 11/16/15 13:25, Thomas Gleixner wrote:
> > On Mon, 16 Nov 2015, Borislav Petkov wrote:
> >> -/*
> >> - * disable flag for tsc. Takes effect by clearing the TSC cpu flag
> >> - * in cpu/common.c
> >> - */
> >> -int __init notsc_setup(char *str)
> >> +/* Disable the TSC feature flag to avoid further TSC use. */
> >> +int __init notsc_setup(void)
> >>  {
> >> +#ifndef CONFIG_X86_TSC
> >>  	setup_clear_cpu_cap(X86_FEATURE_TSC);
> > 
> > This is silly, really.
> > 
> > If CONFIG_X86_TSC is disabled then we should just not compile tsc.c at
> > all and map cpu_has_tsc and stuff depending on it to false.
> > 
> 
> CONFIG_X86_TSC means TSC is obligatory, not that it is supported.

Hmm, I'd still need to untangle tsc.c though for people who want to
disable it for whatever reason.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [RFC PATCH -v2.1] x86: Kill notsc

[Thomas Gleixner]

On Tue, 17 Nov 2015, Borislav Petkov wrote:
> On Mon, Nov 16, 2015 at 09:02:15PM -0800, H. Peter Anvin wrote:
> > On 11/16/15 13:25, Thomas Gleixner wrote:
> > > On Mon, 16 Nov 2015, Borislav Petkov wrote:
> > >> -/*
> > >> - * disable flag for tsc. Takes effect by clearing the TSC cpu flag
> > >> - * in cpu/common.c
> > >> - */
> > >> -int __init notsc_setup(char *str)
> > >> +/* Disable the TSC feature flag to avoid further TSC use. */
> > >> +int __init notsc_setup(void)
> > >>  {
> > >> +#ifndef CONFIG_X86_TSC
> > >>  	setup_clear_cpu_cap(X86_FEATURE_TSC);
> > > 
> > > This is silly, really.
> > > 
> > > If CONFIG_X86_TSC is disabled then we should just not compile tsc.c at
> > > all and map cpu_has_tsc and stuff depending on it to false.
> > > 
> > 
> > CONFIG_X86_TSC means TSC is obligatory, not that it is supported.
> 
> Hmm, I'd still need to untangle tsc.c though for people who want to
> disable it for whatever reason.

There is an interesting problem:

tsc_init()
{
       tsc_khz = x86_platform.calibrate_tsc();
       if (!tsc_khz) {
       	  mark_tsc_unstable("could not calculate TSC khz");
	  ...
       }

In the current code we do NOT use TSC for sched_clock() and that's
correct as we have no idea what the TSC frequency is.

With your changes that is not longer the case, so you end up with a
completely wreckaged sched clock.

Thanks,

	tglx

Re: [RFC PATCH -v2.1] x86: Kill notsc

[Borislav Petkov]

On Tue, Nov 17, 2015 at 10:11:03AM +0100, Thomas Gleixner wrote:
> There is an interesting problem:
> 
> tsc_init()
> {
>        tsc_khz = x86_platform.calibrate_tsc();
>        if (!tsc_khz) {
>        	  mark_tsc_unstable("could not calculate TSC khz");
> 	  ...
>        }
> 
> In the current code we do NOT use TSC for sched_clock() and that's
> correct as we have no idea what the TSC frequency is.
> 
> With your changes that is not longer the case, so you end up with a
> completely wreckaged sched clock.

Hmm, I see it.

Well, I had a related splat which bombed because cyc2ns_init() had to
run before we do sched_init()->idle_init() which called sched_clock()
and there we did the cycles_2_ns() thing and the percpu vars weren't
initialized yet.

That's why I did this:

+void __init early_tsc_init(void)
+{
+       int cpu;
+
+       /*
+        * We need to init the cycles to ns conversion machinery because
+        * init_idle() below will call sched_clock() which needs it.
+        */
+       for_each_possible_cpu(cpu)
+               cyc2ns_init(cpu);
+}

Maybe I should move tsc_init() before sched_init() so that the
calibration happens before we use the TSC in sched_clock() for the first
time...?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [RFC PATCH -v2.1] x86: Kill notsc

[Thomas Gleixner]

On Tue, 17 Nov 2015, Borislav Petkov wrote:
> On Tue, Nov 17, 2015 at 10:11:03AM +0100, Thomas Gleixner wrote:
> > There is an interesting problem:
> > 
> > tsc_init()
> > {
> >        tsc_khz = x86_platform.calibrate_tsc();
> >        if (!tsc_khz) {
> >        	  mark_tsc_unstable("could not calculate TSC khz");
> > 	  ...
> >        }
> > 
> > In the current code we do NOT use TSC for sched_clock() and that's
> > correct as we have no idea what the TSC frequency is.
> > 
> > With your changes that is not longer the case, so you end up with a
> > completely wreckaged sched clock.
> 
> Hmm, I see it.
> 
> Well, I had a related splat which bombed because cyc2ns_init() had to
> run before we do sched_init()->idle_init() which called sched_clock()
> and there we did the cycles_2_ns() thing and the percpu vars weren't
> initialized yet.
> 
> That's why I did this:
> 
> +void __init early_tsc_init(void)
> +{
> +       int cpu;
> +
> +       /*
> +        * We need to init the cycles to ns conversion machinery because
> +        * init_idle() below will call sched_clock() which needs it.
> +        */
> +       for_each_possible_cpu(cpu)
> +               cyc2ns_init(cpu);
> +}
> 
> Maybe I should move tsc_init() before sched_init() so that the
> calibration happens before we use the TSC in sched_clock() for the first
> time...?

How does that help if your TSC frequency is 0 after the calibration?

Thanks,

	tglx

Re: [RFC PATCH -v2.1] x86: Kill notsc

[Borislav Petkov]

On Tue, Nov 17, 2015 at 11:08:19AM +0100, Thomas Gleixner wrote:
> How does that help if your TSC frequency is 0 after the calibration?

Bah, and this brings us back to the need to for the static key...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.