x86: Kill notsc

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?

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

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

arch/x86/kernel/tsc.c done: · line 272

Quoted 4 lines

arch/x86/kernel/tsc.c +4 -0

+#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).

u64 native_sched_clock(void)
{

arch/x86/kernel/tsc.c +4 -7

-	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
 	/*

Quoted 18 lines · Borislav Petkov

u64 native_sched_clock(void)
{

arch/x86/kernel/tsc.c +4 -11

-	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

arch/x86/kernel/tsc.c done: · line 292

Quoted 6 lines

arch/x86/kernel/tsc.c +4 -2

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

arch/x86/kernel/tsc.c int check_tsc_unstable(void) · line 337

Quoted 10 lines · Borislav Petkov

arch/x86/kernel/tsc.c +3 -5

-/*
- * 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.

Quoted 14 lines · Thomas Gleixner

arch/x86/kernel/tsc.c int check_tsc_unstable(void) · line 337

Quoted 10 lines · Borislav Petkov

arch/x86/kernel/tsc.c +3 -5

-/*
- * 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.

Quoted 3 lines

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.

Quoted 15 lines · H. Peter Anvin

Quoted 14 lines · Thomas Gleixner

arch/x86/kernel/tsc.c int check_tsc_unstable(void) · line 337

Quoted 10 lines · Borislav Petkov

arch/x86/kernel/tsc.c +3 -5

-/*
- * 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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 10 lines · Thomas Gleixner

There is an interesting problem:

tsc_init()

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

Quoted 5 lines

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:

arch/x86/kernel/tsc.c +11 -0

+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?