* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-01-19 13:36 ` Peter Zijlstra
@ 2017-01-19 16:54 ` Ingo Molnar
2017-01-19 17:20 ` Peter Zijlstra
2017-01-19 20:35 ` sched/clock: Fix hotplug issue kbuild test robot
` (3 subsequent siblings)
4 siblings, 1 reply; 29+ messages in thread
From: Ingo Molnar @ 2017-01-19 16:54 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner, Borislav Petkov
* Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Jan 19, 2017 at 11:19:24AM +0100, Peter Zijlstra wrote:
>
> > I'll go have a prod. Thanks!
>
> This seems to cure it for me.
>
> ---
> Subject: sched/clock: Fix hotplug issue
JFYI:
arch/x86/kernel/cpu/centaur.c: In function ‘early_init_centaur’:
arch/x86/kernel/cpu/centaur.c:108:2: error: implicit declaration of function ‘clear_sched_clock_stable’ [-Werror=implicit-function-declaration]
x86-64 defconfig-ish.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-01-19 16:54 ` Ingo Molnar
@ 2017-01-19 17:20 ` Peter Zijlstra
0 siblings, 0 replies; 29+ messages in thread
From: Peter Zijlstra @ 2017-01-19 17:20 UTC (permalink / raw)
To: Ingo Molnar
Cc: Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner, Borislav Petkov
On Thu, Jan 19, 2017 at 05:54:55PM +0100, Ingo Molnar wrote:
>
> * Peter Zijlstra <peterz@infradead.org> wrote:
>
> > On Thu, Jan 19, 2017 at 11:19:24AM +0100, Peter Zijlstra wrote:
> >
> > > I'll go have a prod. Thanks!
> >
> > This seems to cure it for me.
> >
> > ---
> > Subject: sched/clock: Fix hotplug issue
>
> JFYI:
>
> arch/x86/kernel/cpu/centaur.c: In function ‘early_init_centaur’:
> arch/x86/kernel/cpu/centaur.c:108:2: error: implicit declaration of function ‘clear_sched_clock_stable’ [-Werror=implicit-function-declaration]
>
> x86-64 defconfig-ish.
Duh.. that'd be a missing: #include <linux/sched.h>
Clearly that's not selected with the .config for the textbox ;-)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: sched/clock: Fix hotplug issue
2017-01-19 13:36 ` Peter Zijlstra
2017-01-19 16:54 ` Ingo Molnar
@ 2017-01-19 20:35 ` kbuild test robot
2017-01-19 20:37 ` kbuild test robot
` (2 subsequent siblings)
4 siblings, 0 replies; 29+ messages in thread
From: kbuild test robot @ 2017-01-19 20:35 UTC (permalink / raw)
To: Peter Zijlstra
Cc: kbuild-all, Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner,
Borislav Petkov
[-- Attachment #1: Type: text/plain, Size: 1533 bytes --]
Hi Peter,
[auto build test ERROR on tip/sched/core]
[also build test ERROR on next-20170119]
[cannot apply to tip/x86/core v4.10-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Peter-Zijlstra/sched-clock-Fix-hotplug-issue/20170120-034727
config: parisc-c3000_defconfig (attached as .config)
compiler: hppa-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=parisc
All errors (new ones prefixed by >>):
arch/parisc/kernel/setup.c: In function 'setup_arch':
>> arch/parisc/kernel/setup.c:180:2: error: implicit declaration of function 'clear_sched_clock_stable' [-Werror=implicit-function-declaration]
clear_sched_clock_stable();
^~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/clear_sched_clock_stable +180 arch/parisc/kernel/setup.c
174 #endif
175
176 #if defined(CONFIG_VT) && defined(CONFIG_DUMMY_CONSOLE)
177 conswitchp = &dummy_con; /* we use do_take_over_console() later ! */
178 #endif
179
> 180 clear_sched_clock_stable();
181 }
182
183 /*
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 14576 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: sched/clock: Fix hotplug issue
2017-01-19 13:36 ` Peter Zijlstra
2017-01-19 16:54 ` Ingo Molnar
2017-01-19 20:35 ` sched/clock: Fix hotplug issue kbuild test robot
@ 2017-01-19 20:37 ` kbuild test robot
2017-01-20 6:36 ` [tip:sched/core] sched/clock: Fix hotplug crash tip-bot for Peter Zijlstra
2017-02-27 12:30 ` tip.today - scheduler bam boom crash (cpu hotplug) Wanpeng Li
4 siblings, 0 replies; 29+ messages in thread
From: kbuild test robot @ 2017-01-19 20:37 UTC (permalink / raw)
To: Peter Zijlstra
Cc: kbuild-all, Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner,
Borislav Petkov
[-- Attachment #1: Type: text/plain, Size: 1367 bytes --]
Hi Peter,
[auto build test ERROR on tip/sched/core]
[also build test ERROR on next-20170119]
[cannot apply to tip/x86/core v4.10-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Peter-Zijlstra/sched-clock-Fix-hotplug-issue/20170120-034727
config: i386-randconfig-x003-201703 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
arch/x86/kernel/cpu/centaur.c: In function 'early_init_centaur':
>> arch/x86/kernel/cpu/centaur.c:108:2: error: implicit declaration of function 'clear_sched_clock_stable' [-Werror=implicit-function-declaration]
clear_sched_clock_stable();
^~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/clear_sched_clock_stable +108 arch/x86/kernel/cpu/centaur.c
102 break;
103 }
104 #ifdef CONFIG_X86_64
105 set_cpu_cap(c, X86_FEATURE_SYSENTER32);
106 #endif
107
> 108 clear_sched_clock_stable();
109 }
110
111 static void init_centaur(struct cpuinfo_x86 *c)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 22309 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* [tip:sched/core] sched/clock: Fix hotplug crash
2017-01-19 13:36 ` Peter Zijlstra
` (2 preceding siblings ...)
2017-01-19 20:37 ` kbuild test robot
@ 2017-01-20 6:36 ` tip-bot for Peter Zijlstra
2017-02-27 12:30 ` tip.today - scheduler bam boom crash (cpu hotplug) Wanpeng Li
4 siblings, 0 replies; 29+ messages in thread
From: tip-bot for Peter Zijlstra @ 2017-01-20 6:36 UTC (permalink / raw)
To: linux-tip-commits
Cc: tglx, hpa, torvalds, bp, mingo, linux-kernel, peterz, efault
Commit-ID: acb04058de49458010c44bb35b849d45113fd668
Gitweb: http://git.kernel.org/tip/acb04058de49458010c44bb35b849d45113fd668
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 19 Jan 2017 14:36:33 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 20 Jan 2017 02:38:46 +0100
sched/clock: Fix hotplug crash
Mike reported that he could trigger the WARN_ON_ONCE() in
set_sched_clock_stable() using hotplug.
This exposed a fundamental problem with the interface, we should never
mark the TSC stable if we ever find it to be unstable. Therefore
set_sched_clock_stable() is a broken interface.
The reason it existed is that not having it is a pain, it means all
relevant architecture code needs to call clear_sched_clock_stable()
where appropriate.
Of the three architectures that select HAVE_UNSTABLE_SCHED_CLOCK ia64
and parisc are trivial in that they never called
set_sched_clock_stable(), so add an unconditional call to
clear_sched_clock_stable() to them.
For x86 the story is a lot more involved, and what this patch tries to
do is ensure we preserve the status quo. So even is Cyrix or Transmeta
have usable TSC they never called set_sched_clock_stable() so they now
get an explicit mark unstable.
Reported-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: 9881b024b7d7 ("sched/clock: Delay switching sched_clock to stable")
Link: http://lkml.kernel.org/r/20170119133633.GB6536@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/ia64/kernel/setup.c | 2 ++
arch/parisc/kernel/setup.c | 2 ++
arch/x86/kernel/cpu/amd.c | 6 ++++--
arch/x86/kernel/cpu/centaur.c | 6 ++++--
arch/x86/kernel/cpu/common.c | 3 +++
arch/x86/kernel/cpu/cyrix.c | 2 ++
arch/x86/kernel/cpu/intel.c | 6 ++++--
arch/x86/kernel/cpu/transmeta.c | 3 +++
arch/x86/kernel/kvmclock.c | 2 +-
include/linux/sched.h | 1 -
kernel/sched/clock.c | 29 ++++++++---------------------
11 files changed, 33 insertions(+), 29 deletions(-)
diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
index 7ec7acc..c483ece 100644
--- a/arch/ia64/kernel/setup.c
+++ b/arch/ia64/kernel/setup.c
@@ -619,6 +619,8 @@ setup_arch (char **cmdline_p)
check_sal_cache_flush();
#endif
paging_init();
+
+ clear_sched_clock_stable();
}
/*
diff --git a/arch/parisc/kernel/setup.c b/arch/parisc/kernel/setup.c
index 2e66a88..068ed36 100644
--- a/arch/parisc/kernel/setup.c
+++ b/arch/parisc/kernel/setup.c
@@ -36,6 +36,7 @@
#undef PCI_DEBUG
#include <linux/proc_fs.h>
#include <linux/export.h>
+#include <linux/sched.h>
#include <asm/processor.h>
#include <asm/sections.h>
@@ -176,6 +177,7 @@ void __init setup_arch(char **cmdline_p)
conswitchp = &dummy_con; /* we use do_take_over_console() later ! */
#endif
+ clear_sched_clock_stable();
}
/*
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 71cae73..1bb253a 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -548,8 +548,10 @@ static void early_init_amd(struct cpuinfo_x86 *c)
if (c->x86_power & (1 << 8)) {
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
- if (!check_tsc_unstable())
- set_sched_clock_stable();
+ if (check_tsc_unstable())
+ clear_sched_clock_stable();
+ } else {
+ clear_sched_clock_stable();
}
/* Bit 12 of 8000_0007 edx is accumulated power mechanism. */
diff --git a/arch/x86/kernel/cpu/centaur.c b/arch/x86/kernel/cpu/centaur.c
index 1661d8e..2c234a6 100644
--- a/arch/x86/kernel/cpu/centaur.c
+++ b/arch/x86/kernel/cpu/centaur.c
@@ -1,5 +1,5 @@
-#include <linux/bitops.h>
-#include <linux/kernel.h>
+
+#include <linux/sched.h>
#include <asm/cpufeature.h>
#include <asm/e820.h>
@@ -104,6 +104,8 @@ static void early_init_centaur(struct cpuinfo_x86 *c)
#ifdef CONFIG_X86_64
set_cpu_cap(c, X86_FEATURE_SYSENTER32);
#endif
+
+ clear_sched_clock_stable();
}
static void init_centaur(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index dc1697c..3457186 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -83,6 +83,7 @@ static void default_init(struct cpuinfo_x86 *c)
strcpy(c->x86_model_id, "386");
}
#endif
+ clear_sched_clock_stable();
}
static const struct cpu_dev default_cpu = {
@@ -1055,6 +1056,8 @@ static void identify_cpu(struct cpuinfo_x86 *c)
*/
if (this_cpu->c_init)
this_cpu->c_init(c);
+ else
+ clear_sched_clock_stable();
/* Disable the PN if appropriate */
squash_the_stupid_serial_number(c);
diff --git a/arch/x86/kernel/cpu/cyrix.c b/arch/x86/kernel/cpu/cyrix.c
index bd9dcd6..47416f9 100644
--- a/arch/x86/kernel/cpu/cyrix.c
+++ b/arch/x86/kernel/cpu/cyrix.c
@@ -9,6 +9,7 @@
#include <asm/pci-direct.h>
#include <asm/tsc.h>
#include <asm/cpufeature.h>
+#include <linux/sched.h>
#include "cpu.h"
@@ -183,6 +184,7 @@ static void early_init_cyrix(struct cpuinfo_x86 *c)
set_cpu_cap(c, X86_FEATURE_CYRIX_ARR);
break;
}
+ clear_sched_clock_stable();
}
static void init_cyrix(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fcd484d..26eaff4 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -124,8 +124,10 @@ static void early_init_intel(struct cpuinfo_x86 *c)
if (c->x86_power & (1 << 8)) {
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
- if (!check_tsc_unstable())
- set_sched_clock_stable();
+ if (check_tsc_unstable())
+ clear_sched_clock_stable();
+ } else {
+ clear_sched_clock_stable();
}
/* Penwell and Cloverview have the TSC which doesn't sleep on S3 */
diff --git a/arch/x86/kernel/cpu/transmeta.c b/arch/x86/kernel/cpu/transmeta.c
index 3417856..c1ea5b9 100644
--- a/arch/x86/kernel/cpu/transmeta.c
+++ b/arch/x86/kernel/cpu/transmeta.c
@@ -1,4 +1,5 @@
#include <linux/kernel.h>
+#include <linux/sched.h>
#include <linux/mm.h>
#include <asm/cpufeature.h>
#include <asm/msr.h>
@@ -14,6 +15,8 @@ static void early_init_transmeta(struct cpuinfo_x86 *c)
if (xlvl >= 0x80860001)
c->x86_capability[CPUID_8086_0001_EDX] = cpuid_edx(0x80860001);
}
+
+ clear_sched_clock_stable();
}
static void init_transmeta(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index 2a5cafd..542710b 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -107,12 +107,12 @@ static inline void kvm_sched_clock_init(bool stable)
{
if (!stable) {
pv_time_ops.sched_clock = kvm_clock_read;
+ clear_sched_clock_stable();
return;
}
kvm_sched_clock_offset = kvm_clock_read();
pv_time_ops.sched_clock = kvm_sched_clock_read;
- set_sched_clock_stable();
printk(KERN_INFO "kvm-clock: using sched offset of %llu cycles\n",
kvm_sched_clock_offset);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index a8daed9..69e6852 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2547,7 +2547,6 @@ extern void sched_clock_init_late(void);
* is reliable after all:
*/
extern int sched_clock_stable(void);
-extern void set_sched_clock_stable(void);
extern void clear_sched_clock_stable(void);
extern void sched_clock_tick(void);
diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
index 7713b2b..ad64efe 100644
--- a/kernel/sched/clock.c
+++ b/kernel/sched/clock.c
@@ -83,8 +83,15 @@ void sched_clock_init(void)
}
#ifdef CONFIG_HAVE_UNSTABLE_SCHED_CLOCK
+/*
+ * We must start with !__sched_clock_stable because the unstable -> stable
+ * transition is accurate, while the stable -> unstable transition is not.
+ *
+ * Similarly we start with __sched_clock_stable_early, thereby assuming we
+ * will become stable, such that there's only a single 1 -> 0 transition.
+ */
static DEFINE_STATIC_KEY_FALSE(__sched_clock_stable);
-static int __sched_clock_stable_early;
+static int __sched_clock_stable_early = 1;
/*
* We want: ktime_get_ns() + gtod_offset == sched_clock() + raw_offset
@@ -132,24 +139,6 @@ static void __set_sched_clock_stable(void)
tick_dep_clear(TICK_DEP_BIT_CLOCK_UNSTABLE);
}
-void set_sched_clock_stable(void)
-{
- __sched_clock_stable_early = 1;
-
- smp_mb(); /* matches sched_clock_init_late() */
-
- /*
- * This really should only be called early (before
- * sched_clock_init_late()) when guestimating our sched_clock() is
- * solid.
- *
- * After that we test stability and we can negate our guess using
- * clear_sched_clock_stable, possibly from a watchdog.
- */
- if (WARN_ON_ONCE(sched_clock_running == 2))
- __set_sched_clock_stable();
-}
-
static void __clear_sched_clock_stable(struct work_struct *work)
{
struct sched_clock_data *scd = this_scd();
@@ -199,8 +188,6 @@ void sched_clock_init_late(void)
if (__sched_clock_stable_early)
__set_sched_clock_stable();
- else
- __clear_sched_clock_stable(NULL);
}
/*
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-01-19 13:36 ` Peter Zijlstra
` (3 preceding siblings ...)
2017-01-20 6:36 ` [tip:sched/core] sched/clock: Fix hotplug crash tip-bot for Peter Zijlstra
@ 2017-02-27 12:30 ` Wanpeng Li
2017-02-27 12:35 ` Paolo Bonzini
2017-02-27 12:43 ` Peter Zijlstra
4 siblings, 2 replies; 29+ messages in thread
From: Wanpeng Li @ 2017-02-27 12:30 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner,
Borislav Petkov, Paolo Bonzini
Cc Paolo,
2017-01-19 21:36 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Thu, Jan 19, 2017 at 11:19:24AM +0100, Peter Zijlstra wrote:
>
>> I'll go have a prod. Thanks!
>
> This seems to cure it for me.
>
> ---
> Subject: sched/clock: Fix hotplug issue
>
> Mike reported that he could trigger the WARN_ON_ONCE() in
> set_sched_clock_stable() using hotplug.
>
> This exposed a fundamental problem with the interface, we should never
> mark the TSC stable if we ever find it to be unstable. Therefore
> set_sched_clock_stable() is a broken interface.
>
> The reason it existed is that not having it is a pain, it means all
> relevant architecture code needs to call clear_sched_clock_stable()
> where appropriate.
>
> Of the three architectures that select HAVE_UNSTABLE_SCHED_CLOCK ia64
> and parisc are trivial in that they never called
> set_sched_clock_stable(), so add an unconditional call to
> clear_sched_clock_stable() to them.
>
> For x86 the story is a lot more involved, and what this patch tries to
> do is ensure we preserve the status quo. So even is Cyrix or Transmeta
> have usable TSC they never called set_sched_clock_stable() so they now
> get an explicit mark unstable.
>
> XXX: what about Xen ?
>
> Fixes: 9881b024b7d7 ("sched/clock: Delay switching sched_clock to stable")
> Reported-by: Mike Galbraith <efault@gmx.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> arch/ia64/kernel/setup.c | 2 ++
> arch/parisc/kernel/setup.c | 2 ++
> arch/x86/kernel/cpu/amd.c | 6 ++++--
> arch/x86/kernel/cpu/centaur.c | 2 ++
> arch/x86/kernel/cpu/common.c | 3 +++
> arch/x86/kernel/cpu/cyrix.c | 2 ++
> arch/x86/kernel/cpu/intel.c | 6 ++++--
> arch/x86/kernel/cpu/transmeta.c | 3 +++
> arch/x86/kernel/kvmclock.c | 2 +-
> include/linux/sched.h | 1 -
> kernel/sched/clock.c | 29 ++++++++---------------------
> 11 files changed, 31 insertions(+), 27 deletions(-)
>
> diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
> index 7ec7acc..c483ece 100644
> --- a/arch/ia64/kernel/setup.c
> +++ b/arch/ia64/kernel/setup.c
> @@ -619,6 +619,8 @@ setup_arch (char **cmdline_p)
> check_sal_cache_flush();
> #endif
> paging_init();
> +
> + clear_sched_clock_stable();
> }
>
> /*
> diff --git a/arch/parisc/kernel/setup.c b/arch/parisc/kernel/setup.c
> index 2e66a88..068ed36 100644
> --- a/arch/parisc/kernel/setup.c
> +++ b/arch/parisc/kernel/setup.c
> @@ -36,6 +36,7 @@
> #undef PCI_DEBUG
> #include <linux/proc_fs.h>
> #include <linux/export.h>
> +#include <linux/sched.h>
>
> #include <asm/processor.h>
> #include <asm/sections.h>
> @@ -176,6 +177,7 @@ void __init setup_arch(char **cmdline_p)
> conswitchp = &dummy_con; /* we use do_take_over_console() later ! */
> #endif
>
> + clear_sched_clock_stable();
> }
>
> /*
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 1d31672..80e657e 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -541,8 +541,10 @@ static void early_init_amd(struct cpuinfo_x86 *c)
> if (c->x86_power & (1 << 8)) {
> set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> - if (!check_tsc_unstable())
> - set_sched_clock_stable();
> + if (check_tsc_unstable())
> + clear_sched_clock_stable();
> + } else {
> + clear_sched_clock_stable();
> }
>
> /* Bit 12 of 8000_0007 edx is accumulated power mechanism. */
> diff --git a/arch/x86/kernel/cpu/centaur.c b/arch/x86/kernel/cpu/centaur.c
> index 1661d8e..c4bab8c 100644
> --- a/arch/x86/kernel/cpu/centaur.c
> +++ b/arch/x86/kernel/cpu/centaur.c
> @@ -104,6 +104,8 @@ static void early_init_centaur(struct cpuinfo_x86 *c)
> #ifdef CONFIG_X86_64
> set_cpu_cap(c, X86_FEATURE_SYSENTER32);
> #endif
> +
> + clear_sched_clock_stable();
> }
>
> static void init_centaur(struct cpuinfo_x86 *c)
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 9bab7a8..0bdb1ab 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -83,6 +83,7 @@ static void default_init(struct cpuinfo_x86 *c)
> strcpy(c->x86_model_id, "386");
> }
> #endif
> + clear_sched_clock_stable();
> }
>
> static const struct cpu_dev default_cpu = {
> @@ -1055,6 +1056,8 @@ static void identify_cpu(struct cpuinfo_x86 *c)
> */
> if (this_cpu->c_init)
> this_cpu->c_init(c);
> + else
> + clear_sched_clock_stable();
>
> /* Disable the PN if appropriate */
> squash_the_stupid_serial_number(c);
> diff --git a/arch/x86/kernel/cpu/cyrix.c b/arch/x86/kernel/cpu/cyrix.c
> index bd9dcd6..47416f9 100644
> --- a/arch/x86/kernel/cpu/cyrix.c
> +++ b/arch/x86/kernel/cpu/cyrix.c
> @@ -9,6 +9,7 @@
> #include <asm/pci-direct.h>
> #include <asm/tsc.h>
> #include <asm/cpufeature.h>
> +#include <linux/sched.h>
>
> #include "cpu.h"
>
> @@ -183,6 +184,7 @@ static void early_init_cyrix(struct cpuinfo_x86 *c)
> set_cpu_cap(c, X86_FEATURE_CYRIX_ARR);
> break;
> }
> + clear_sched_clock_stable();
> }
>
> static void init_cyrix(struct cpuinfo_x86 *c)
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 203f860..026c728 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -119,8 +119,10 @@ static void early_init_intel(struct cpuinfo_x86 *c)
> if (c->x86_power & (1 << 8)) {
> set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> - if (!check_tsc_unstable())
> - set_sched_clock_stable();
> + if (check_tsc_unstable())
> + clear_sched_clock_stable();
> + } else {
> + clear_sched_clock_stable();
> }
>
> /* Penwell and Cloverview have the TSC which doesn't sleep on S3 */
> diff --git a/arch/x86/kernel/cpu/transmeta.c b/arch/x86/kernel/cpu/transmeta.c
> index 3417856..c1ea5b9 100644
> --- a/arch/x86/kernel/cpu/transmeta.c
> +++ b/arch/x86/kernel/cpu/transmeta.c
> @@ -1,4 +1,5 @@
> #include <linux/kernel.h>
> +#include <linux/sched.h>
> #include <linux/mm.h>
> #include <asm/cpufeature.h>
> #include <asm/msr.h>
> @@ -14,6 +15,8 @@ static void early_init_transmeta(struct cpuinfo_x86 *c)
> if (xlvl >= 0x80860001)
> c->x86_capability[CPUID_8086_0001_EDX] = cpuid_edx(0x80860001);
> }
> +
> + clear_sched_clock_stable();
> }
>
> static void init_transmeta(struct cpuinfo_x86 *c)
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index 2a5cafd..542710b 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -107,12 +107,12 @@ static inline void kvm_sched_clock_init(bool stable)
> {
> if (!stable) {
> pv_time_ops.sched_clock = kvm_clock_read;
> + clear_sched_clock_stable();
> return;
> }
>
> kvm_sched_clock_offset = kvm_clock_read();
> pv_time_ops.sched_clock = kvm_sched_clock_read;
> - set_sched_clock_stable();
This results in sched clock always unstable for kvm guest since there
is no invariant tsc cpuid bit exposed for kvm guest currently. The
blockage happened for several reasons:
1) Migration: to host with different TSC frequency.
2) Savevm: It is not safe to use the TSC for wall clock timer services.
How about something like below(untested):
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index bae6ea6..a61c477 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -115,6 +115,7 @@ static inline void kvm_sched_clock_init(bool stable)
kvm_sched_clock_offset = kvm_clock_read();
pv_time_ops.sched_clock = kvm_sched_clock_read;
+ hypervisor_sched_clock_stable();
printk(KERN_INFO "kvm-clock: using sched offset of %llu cycles\n",
kvm_sched_clock_offset);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 451e241..38c6edb 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2499,6 +2499,10 @@ static inline void clear_sched_clock_stable(void)
{
}
+static inline void hypervisor_sched_clock_stable(void)
+{
+}
+
static inline void sched_clock_idle_sleep_event(void)
{
}
@@ -2526,6 +2530,7 @@ extern void sched_clock_init_late(void);
*/
extern int sched_clock_stable(void);
extern void clear_sched_clock_stable(void);
+extern void hypervisor_sched_clock_stable(void);
extern void sched_clock_tick(void);
extern void sched_clock_idle_sleep_event(void);
diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
index ad64efe..a46639e 100644
--- a/kernel/sched/clock.c
+++ b/kernel/sched/clock.c
@@ -77,9 +77,15 @@ EXPORT_SYMBOL_GPL(sched_clock);
__read_mostly int sched_clock_running;
+enum {
+ SCHED_CLOCK_INIT = 0,
+ HYPERVISOR_SCHED_CLOCK_STABLE,
+ SCHED_CLOCK_INIT_LATE
+};
+
void sched_clock_init(void)
{
- sched_clock_running = 1;
+ sched_clock_running = SCHED_CLOCK_INIT;
}
#ifdef CONFIG_HAVE_UNSTABLE_SCHED_CLOCK
@@ -170,13 +176,14 @@ void clear_sched_clock_stable(void)
smp_mb(); /* matches sched_clock_init_late() */
- if (sched_clock_running == 2)
+ if (sched_clock_running == SCHED_CLOCK_INIT_LATE)
schedule_work(&sched_clock_work);
}
void sched_clock_init_late(void)
{
- sched_clock_running = 2;
+ if (sched_clock_running == SCHED_CLOCK_INIT)
+ sched_clock_running = SCHED_CLOCK_INIT_LATE;
/*
* Ensure that it is impossible to not do a static_key update.
*
@@ -186,8 +193,15 @@ void sched_clock_init_late(void)
*/
smp_mb(); /* matches {set,clear}_sched_clock_stable() */
- if (__sched_clock_stable_early)
+ if (__sched_clock_stable_early ||
+ sched_clock_running == HYPERVISOR_SCHED_CLOCK_STABLE) {
__set_sched_clock_stable();
+ }
+}
+
+void hypervisor_sched_clock_stable()
+{
+ sched_clock_running = HYPERVISOR_SCHED_CLOCK_STABLE;
}
/*
Regards,
Wanpeng Li
>
> printk(KERN_INFO "kvm-clock: using sched offset of %llu cycles\n",
> kvm_sched_clock_offset);
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 68b35dd..8a9bbb6 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2522,7 +2522,6 @@ extern void sched_clock_init_late(void);
> * is reliable after all:
> */
> extern int sched_clock_stable(void);
> -extern void set_sched_clock_stable(void);
> extern void clear_sched_clock_stable(void);
>
> extern void sched_clock_tick(void);
> diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
> index 7713b2b..ad64efe 100644
> --- a/kernel/sched/clock.c
> +++ b/kernel/sched/clock.c
> @@ -83,8 +83,15 @@ void sched_clock_init(void)
> }
>
> #ifdef CONFIG_HAVE_UNSTABLE_SCHED_CLOCK
> +/*
> + * We must start with !__sched_clock_stable because the unstable -> stable
> + * transition is accurate, while the stable -> unstable transition is not.
> + *
> + * Similarly we start with __sched_clock_stable_early, thereby assuming we
> + * will become stable, such that there's only a single 1 -> 0 transition.
> + */
> static DEFINE_STATIC_KEY_FALSE(__sched_clock_stable);
> -static int __sched_clock_stable_early;
> +static int __sched_clock_stable_early = 1;
>
> /*
> * We want: ktime_get_ns() + gtod_offset == sched_clock() + raw_offset
> @@ -132,24 +139,6 @@ static void __set_sched_clock_stable(void)
> tick_dep_clear(TICK_DEP_BIT_CLOCK_UNSTABLE);
> }
>
> -void set_sched_clock_stable(void)
> -{
> - __sched_clock_stable_early = 1;
> -
> - smp_mb(); /* matches sched_clock_init_late() */
> -
> - /*
> - * This really should only be called early (before
> - * sched_clock_init_late()) when guestimating our sched_clock() is
> - * solid.
> - *
> - * After that we test stability and we can negate our guess using
> - * clear_sched_clock_stable, possibly from a watchdog.
> - */
> - if (WARN_ON_ONCE(sched_clock_running == 2))
> - __set_sched_clock_stable();
> -}
> -
> static void __clear_sched_clock_stable(struct work_struct *work)
> {
> struct sched_clock_data *scd = this_scd();
> @@ -199,8 +188,6 @@ void sched_clock_init_late(void)
>
> if (__sched_clock_stable_early)
> __set_sched_clock_stable();
> - else
> - __clear_sched_clock_stable(NULL);
> }
>
> /*
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 12:30 ` tip.today - scheduler bam boom crash (cpu hotplug) Wanpeng Li
@ 2017-02-27 12:35 ` Paolo Bonzini
2017-02-27 12:43 ` Peter Zijlstra
1 sibling, 0 replies; 29+ messages in thread
From: Paolo Bonzini @ 2017-02-27 12:35 UTC (permalink / raw)
To: Wanpeng Li, Peter Zijlstra
Cc: Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner, Borislav Petkov
On 27/02/2017 13:30, Wanpeng Li wrote:
> This results in sched clock always unstable for kvm guest since there
> is no invariant tsc cpuid bit exposed for kvm guest currently. The
> blockage happened for several reasons:
>
> 1) Migration: to host with different TSC frequency.
> 2) Savevm: It is not safe to use the TSC for wall clock timer services.
Right, the purpose of kvmclock is basically to ensure a stable clock
without using TSC. However, I know zilch about kernel/sched/clock.c so
I cannot really understand your patch.
Paolo
> How about something like below(untested):
>
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index bae6ea6..a61c477 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -115,6 +115,7 @@ static inline void kvm_sched_clock_init(bool stable)
>
> kvm_sched_clock_offset = kvm_clock_read();
> pv_time_ops.sched_clock = kvm_sched_clock_read;
> + hypervisor_sched_clock_stable();
>
> printk(KERN_INFO "kvm-clock: using sched offset of %llu cycles\n",
> kvm_sched_clock_offset);
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 451e241..38c6edb 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2499,6 +2499,10 @@ static inline void clear_sched_clock_stable(void)
> {
> }
>
> +static inline void hypervisor_sched_clock_stable(void)
> +{
> +}
> +
> static inline void sched_clock_idle_sleep_event(void)
> {
> }
> @@ -2526,6 +2530,7 @@ extern void sched_clock_init_late(void);
> */
> extern int sched_clock_stable(void);
> extern void clear_sched_clock_stable(void);
> +extern void hypervisor_sched_clock_stable(void);
>
> extern void sched_clock_tick(void);
> extern void sched_clock_idle_sleep_event(void);
> diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
> index ad64efe..a46639e 100644
> --- a/kernel/sched/clock.c
> +++ b/kernel/sched/clock.c
> @@ -77,9 +77,15 @@ EXPORT_SYMBOL_GPL(sched_clock);
>
> __read_mostly int sched_clock_running;
>
> +enum {
> + SCHED_CLOCK_INIT = 0,
> + HYPERVISOR_SCHED_CLOCK_STABLE,
> + SCHED_CLOCK_INIT_LATE
> +};
> +
> void sched_clock_init(void)
> {
> - sched_clock_running = 1;
> + sched_clock_running = SCHED_CLOCK_INIT;
> }
>
> #ifdef CONFIG_HAVE_UNSTABLE_SCHED_CLOCK
> @@ -170,13 +176,14 @@ void clear_sched_clock_stable(void)
>
> smp_mb(); /* matches sched_clock_init_late() */
>
> - if (sched_clock_running == 2)
> + if (sched_clock_running == SCHED_CLOCK_INIT_LATE)
> schedule_work(&sched_clock_work);
> }
>
> void sched_clock_init_late(void)
> {
> - sched_clock_running = 2;
> + if (sched_clock_running == SCHED_CLOCK_INIT)
> + sched_clock_running = SCHED_CLOCK_INIT_LATE;
> /*
> * Ensure that it is impossible to not do a static_key update.
> *
> @@ -186,8 +193,15 @@ void sched_clock_init_late(void)
> */
> smp_mb(); /* matches {set,clear}_sched_clock_stable() */
>
> - if (__sched_clock_stable_early)
> + if (__sched_clock_stable_early ||
> + sched_clock_running == HYPERVISOR_SCHED_CLOCK_STABLE) {
> __set_sched_clock_stable();
> + }
> +}
> +
> +void hypervisor_sched_clock_stable()
> +{
> + sched_clock_running = HYPERVISOR_SCHED_CLOCK_STABLE;
> }
>
> /*
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 12:30 ` tip.today - scheduler bam boom crash (cpu hotplug) Wanpeng Li
2017-02-27 12:35 ` Paolo Bonzini
@ 2017-02-27 12:43 ` Peter Zijlstra
2017-02-27 12:50 ` Paolo Bonzini
2017-02-27 13:48 ` Wanpeng Li
1 sibling, 2 replies; 29+ messages in thread
From: Peter Zijlstra @ 2017-02-27 12:43 UTC (permalink / raw)
To: Wanpeng Li
Cc: Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner,
Borislav Petkov, Paolo Bonzini
On Mon, Feb 27, 2017 at 08:30:11PM +0800, Wanpeng Li wrote:
> > diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> > index 2a5cafd..542710b 100644
> > --- a/arch/x86/kernel/kvmclock.c
> > +++ b/arch/x86/kernel/kvmclock.c
> > @@ -107,12 +107,12 @@ static inline void kvm_sched_clock_init(bool stable)
> > {
> > if (!stable) {
> > pv_time_ops.sched_clock = kvm_clock_read;
> > + clear_sched_clock_stable();
> > return;
> > }
> >
> > kvm_sched_clock_offset = kvm_clock_read();
> > pv_time_ops.sched_clock = kvm_sched_clock_read;
> > - set_sched_clock_stable();
>
> This results in sched clock always unstable for kvm guest since there
> is no invariant tsc cpuid bit exposed for kvm guest currently.
What the heck is KVM_FEATURE_CLOCKSOURCE_STABLE_BIT /
PVCLOCK_TSC_STABLE_BIT about then?
> How about something like below(untested):
I have no clue what that tries to do.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 12:43 ` Peter Zijlstra
@ 2017-02-27 12:50 ` Paolo Bonzini
2017-02-27 13:04 ` Peter Zijlstra
2017-02-27 13:48 ` Wanpeng Li
1 sibling, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2017-02-27 12:50 UTC (permalink / raw)
To: Peter Zijlstra, Wanpeng Li
Cc: Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner, Borislav Petkov
On 27/02/2017 13:43, Peter Zijlstra wrote:
> On Mon, Feb 27, 2017 at 08:30:11PM +0800, Wanpeng Li wrote:
>
>>> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
>>> index 2a5cafd..542710b 100644
>>> --- a/arch/x86/kernel/kvmclock.c
>>> +++ b/arch/x86/kernel/kvmclock.c
>>> @@ -107,12 +107,12 @@ static inline void kvm_sched_clock_init(bool stable)
>>> {
>>> if (!stable) {
>>> pv_time_ops.sched_clock = kvm_clock_read;
>>> + clear_sched_clock_stable();
>>> return;
>>> }
>>>
>>> kvm_sched_clock_offset = kvm_clock_read();
>>> pv_time_ops.sched_clock = kvm_sched_clock_read;
>>> - set_sched_clock_stable();
>>
>> This results in sched clock always unstable for kvm guest since there
>> is no invariant tsc cpuid bit exposed for kvm guest currently.
>
> What the heck is KVM_FEATURE_CLOCKSOURCE_STABLE_BIT /
> PVCLOCK_TSC_STABLE_BIT about then?
It checks that all the bugs in the host have been ironed out, and that
the host itself supports invtsc.
Paolo
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 12:50 ` Paolo Bonzini
@ 2017-02-27 13:04 ` Peter Zijlstra
2017-02-27 15:27 ` Paolo Bonzini
0 siblings, 1 reply; 29+ messages in thread
From: Peter Zijlstra @ 2017-02-27 13:04 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Wanpeng Li, Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner,
Borislav Petkov
On Mon, Feb 27, 2017 at 01:50:29PM +0100, Paolo Bonzini wrote:
>
>
> On 27/02/2017 13:43, Peter Zijlstra wrote:
> > On Mon, Feb 27, 2017 at 08:30:11PM +0800, Wanpeng Li wrote:
> >
> >>> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> >>> index 2a5cafd..542710b 100644
> >>> --- a/arch/x86/kernel/kvmclock.c
> >>> +++ b/arch/x86/kernel/kvmclock.c
> >>> @@ -107,12 +107,12 @@ static inline void kvm_sched_clock_init(bool stable)
> >>> {
> >>> if (!stable) {
> >>> pv_time_ops.sched_clock = kvm_clock_read;
> >>> + clear_sched_clock_stable();
> >>> return;
> >>> }
> >>>
> >>> kvm_sched_clock_offset = kvm_clock_read();
> >>> pv_time_ops.sched_clock = kvm_sched_clock_read;
> >>> - set_sched_clock_stable();
> >>
> >> This results in sched clock always unstable for kvm guest since there
> >> is no invariant tsc cpuid bit exposed for kvm guest currently.
> >
> > What the heck is KVM_FEATURE_CLOCKSOURCE_STABLE_BIT /
> > PVCLOCK_TSC_STABLE_BIT about then?
>
> It checks that all the bugs in the host have been ironed out, and that
> the host itself supports invtsc.
But what does it mean if that is not so? That is, will kvm_clock_read()
still be stable even if !stable?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 13:04 ` Peter Zijlstra
@ 2017-02-27 15:27 ` Paolo Bonzini
2017-02-27 15:59 ` Peter Zijlstra
0 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2017-02-27 15:27 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Wanpeng Li, Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner,
Borislav Petkov
On 27/02/2017 14:04, Peter Zijlstra wrote:
>>>> This results in sched clock always unstable for kvm guest since there
>>>> is no invariant tsc cpuid bit exposed for kvm guest currently.
>>> What the heck is KVM_FEATURE_CLOCKSOURCE_STABLE_BIT /
>>> PVCLOCK_TSC_STABLE_BIT about then?
>> It checks that all the bugs in the host have been ironed out, and that
>> the host itself supports invtsc.
> But what does it mean if that is not so? That is, will kvm_clock_read()
> still be stable even if !stable?
If kvmclock is !stable, nobody should have set that the sched clock to
stable, to begin with.
However, if kvmclock is stable, we know that the sched clock is stable.
Paolo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 15:27 ` Paolo Bonzini
@ 2017-02-27 15:59 ` Peter Zijlstra
2017-02-27 16:11 ` Paolo Bonzini
0 siblings, 1 reply; 29+ messages in thread
From: Peter Zijlstra @ 2017-02-27 15:59 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Wanpeng Li, Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner,
Borislav Petkov
On Mon, Feb 27, 2017 at 04:27:32PM +0100, Paolo Bonzini wrote:
>
>
> On 27/02/2017 14:04, Peter Zijlstra wrote:
> >>>> This results in sched clock always unstable for kvm guest since there
> >>>> is no invariant tsc cpuid bit exposed for kvm guest currently.
> >>> What the heck is KVM_FEATURE_CLOCKSOURCE_STABLE_BIT /
> >>> PVCLOCK_TSC_STABLE_BIT about then?
> >> It checks that all the bugs in the host have been ironed out, and that
> >> the host itself supports invtsc.
> > But what does it mean if that is not so? That is, will kvm_clock_read()
> > still be stable even if !stable?
>
> If kvmclock is !stable, nobody should have set that the sched clock to
> stable, to begin with.
OK, so if !KVM_FEATURE_CLOCKSOURCE_STABLE_BIT nothing is stable, but if
it is set, TSC might still not be stable, but kvm_clock_read() is.
> However, if kvmclock is stable, we know that the sched clock is stable.
Right, so the problem is that we only ever want to allow marking
unstable -- once its found unstable, for whatever reason, we should
never allow going stable. The corollary of this proposition is that we
must start out assuming it will become stable. And to avoid actually
using unstable TSC we do a 3 state bringup:
1) sched_clock_running = 0, __stable_early = 1, __stable = 0
2) sched_clock_running = 1 (__stable is effective, iow, we run unstable)
3) sched_clock_running = 2 (__stable <- __stable_early)
2) happens 'early' but is 'safe'.
3) happens 'late', after we've brought up SMP and probed TSC
Between there, we should have detected the most common TSC wreckage and
made sure to not then switch to 'stable' at 3.
Now the problem appears to be that we assume sched_clock will use RDTSC
(native_sched_clock) while sched_clock is a paravirt op.
Now, I've not yet figured out the ordering between when we set
pv_time_ops.sched_clock and when we do the 'normal' TSC init stuff.
But it appears to me, we should not be calling
clear_sched_clock_stable() on TSC bits when we don't end up using
native_sched_clock().
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 15:59 ` Peter Zijlstra
@ 2017-02-27 16:11 ` Paolo Bonzini
2017-02-27 16:36 ` Peter Zijlstra
2017-02-28 1:51 ` Wanpeng Li
0 siblings, 2 replies; 29+ messages in thread
From: Paolo Bonzini @ 2017-02-27 16:11 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Wanpeng Li, Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner,
Borislav Petkov
On 27/02/2017 16:59, Peter Zijlstra wrote:
> OK, so if !KVM_FEATURE_CLOCKSOURCE_STABLE_BIT nothing is stable, but if
> it is set, TSC might still not be stable, but kvm_clock_read() is.
>
>> However, if kvmclock is stable, we know that the sched clock is stable.
> Right, so the problem is that we only ever want to allow marking
> unstable -- once its found unstable, for whatever reason, we should
> never allow going stable. The corollary of this proposition is that we
> must start out assuming it will become stable. And to avoid actually
> using unstable TSC we do a 3 state bringup:
>
> 1) sched_clock_running = 0, __stable_early = 1, __stable = 0
> 2) sched_clock_running = 1 (__stable is effective, iow, we run unstable)
> 3) sched_clock_running = 2 (__stable <- __stable_early)
>
> 2) happens 'early' but is 'safe'.
> 3) happens 'late', after we've brought up SMP and probed TSC
>
> Between there, we should have detected the most common TSC wreckage and
> made sure to not then switch to 'stable' at 3.
>
> Now the problem appears to be that we assume sched_clock will use RDTSC
> (native_sched_clock) while sched_clock is a paravirt op.
>
> Now, I've not yet figured out the ordering between when we set
> pv_time_ops.sched_clock and when we do the 'normal' TSC init stuff.
I think the ordering is fine:
- pv_time_ops.sched_clock is set here:
start_kernel (init/main.c line 509)
setup_arch
kvmclock_init
kvm_sched_clock_init
- TSC can be declared unstable only after this:
start_kernel (init/main.c line 628)
late_time_init
tsc_init
So by the time the tsc_cs_mark_unstable or mark_tsc_unstable can call
clear_sched_clock_stable, pv_time_ops.sched_clock has been set.
> But it appears to me, we should not be calling
> clear_sched_clock_stable() on TSC bits when we don't end up using
> native_sched_clock().
Yes, this makes sense.
Paolo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 16:11 ` Paolo Bonzini
@ 2017-02-27 16:36 ` Peter Zijlstra
2017-02-27 17:27 ` Paolo Bonzini
2017-02-28 1:51 ` Wanpeng Li
1 sibling, 1 reply; 29+ messages in thread
From: Peter Zijlstra @ 2017-02-27 16:36 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Wanpeng Li, Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner,
Borislav Petkov
On Mon, Feb 27, 2017 at 05:11:34PM +0100, Paolo Bonzini wrote:
> On 27/02/2017 16:59, Peter Zijlstra wrote:
> > Now, I've not yet figured out the ordering between when we set
> > pv_time_ops.sched_clock and when we do the 'normal' TSC init stuff.
>
> I think the ordering is fine:
>
> - pv_time_ops.sched_clock is set here:
>
> start_kernel (init/main.c line 509)
> setup_arch
> kvmclock_init
> kvm_sched_clock_init
>
> - TSC can be declared unstable only after this:
>
> start_kernel (init/main.c line 628)
> late_time_init
> tsc_init
>
> So by the time the tsc_cs_mark_unstable or mark_tsc_unstable can call
> clear_sched_clock_stable, pv_time_ops.sched_clock has been set.
>
> > But it appears to me, we should not be calling
> > clear_sched_clock_stable() on TSC bits when we don't end up using
> > native_sched_clock().
>
> Yes, this makes sense.
Something like the below then (completely untested for now) has a chance
of working. It does however change behaviour a little.
I'm trying to debug something else; after that I'll give this a little
more consideration.
---
arch/x86/kernel/cpu/amd.c | 4 +---
arch/x86/kernel/cpu/centaur.c | 2 +-
arch/x86/kernel/cpu/common.c | 4 ++--
arch/x86/kernel/cpu/cyrix.c | 2 +-
arch/x86/kernel/cpu/intel.c | 4 +---
arch/x86/kernel/cpu/transmeta.c | 2 +-
arch/x86/kernel/tsc.c | 33 +++++++++++++++++++++------------
7 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 4e95b2e0d95f..bc3bbb6a8ab0 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -555,10 +555,8 @@ static void early_init_amd(struct cpuinfo_x86 *c)
if (c->x86_power & (1 << 8)) {
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
- if (check_tsc_unstable())
- clear_sched_clock_stable();
} else {
- clear_sched_clock_stable();
+ mark_tsc_unstable("not invariant");
}
/* Bit 12 of 8000_0007 edx is accumulated power mechanism. */
diff --git a/arch/x86/kernel/cpu/centaur.c b/arch/x86/kernel/cpu/centaur.c
index 2c234a6d94c4..0fdff183aa30 100644
--- a/arch/x86/kernel/cpu/centaur.c
+++ b/arch/x86/kernel/cpu/centaur.c
@@ -105,7 +105,7 @@ static void early_init_centaur(struct cpuinfo_x86 *c)
set_cpu_cap(c, X86_FEATURE_SYSENTER32);
#endif
- clear_sched_clock_stable();
+ mark_tsc_unstable("not invariant");
}
static void init_centaur(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index f07005e6f461..2b7ff648ea25 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -86,7 +86,7 @@ static void default_init(struct cpuinfo_x86 *c)
strcpy(c->x86_model_id, "386");
}
#endif
- clear_sched_clock_stable();
+ mark_tsc_unstable("not invariant");
}
static const struct cpu_dev default_cpu = {
@@ -1076,7 +1076,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
if (this_cpu->c_init)
this_cpu->c_init(c);
else
- clear_sched_clock_stable();
+ mark_tsc_unstable("not invariant");
/* Disable the PN if appropriate */
squash_the_stupid_serial_number(c);
diff --git a/arch/x86/kernel/cpu/cyrix.c b/arch/x86/kernel/cpu/cyrix.c
index 47416f959a48..35057d67e864 100644
--- a/arch/x86/kernel/cpu/cyrix.c
+++ b/arch/x86/kernel/cpu/cyrix.c
@@ -184,7 +184,7 @@ static void early_init_cyrix(struct cpuinfo_x86 *c)
set_cpu_cap(c, X86_FEATURE_CYRIX_ARR);
break;
}
- clear_sched_clock_stable();
+ mark_tsc_unstable("not invariant");
}
static void init_cyrix(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 017ecd3bb553..e0e192e43a4c 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -161,10 +161,8 @@ static void early_init_intel(struct cpuinfo_x86 *c)
if (c->x86_power & (1 << 8)) {
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
- if (check_tsc_unstable())
- clear_sched_clock_stable();
} else {
- clear_sched_clock_stable();
+ mark_tsc_unstable("not invariant");
}
/* Penwell and Cloverview have the TSC which doesn't sleep on S3 */
diff --git a/arch/x86/kernel/cpu/transmeta.c b/arch/x86/kernel/cpu/transmeta.c
index c1ea5b999839..fa243ffd1c84 100644
--- a/arch/x86/kernel/cpu/transmeta.c
+++ b/arch/x86/kernel/cpu/transmeta.c
@@ -16,7 +16,7 @@ static void early_init_transmeta(struct cpuinfo_x86 *c)
c->x86_capability[CPUID_8086_0001_EDX] = cpuid_edx(0x80860001);
}
- clear_sched_clock_stable();
+ mark_tsc_unstable("not invariant");
}
static void init_transmeta(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 2724dc82f992..bf6627aff54d 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -326,9 +326,16 @@ unsigned long long sched_clock(void)
{
return paravirt_sched_clock();
}
+
+static inline bool using_native_sched_clock(void)
+{
+ return pv_time_ops.sched_clock == native_sched_clock;
+}
#else
unsigned long long
sched_clock(void) __attribute__((alias("native_sched_clock")));
+
+static inline bool using_native_sched_clock(void) { return true; }
#endif
int check_tsc_unstable(void)
@@ -1112,7 +1119,8 @@ static void tsc_cs_mark_unstable(struct clocksource *cs)
if (tsc_unstable)
return;
tsc_unstable = 1;
- clear_sched_clock_stable();
+ if (using_native_sched_clock())
+ clear_sched_clock_stable();
disable_sched_clock_irqtime();
pr_info("Marking TSC unstable due to clocksource watchdog\n");
}
@@ -1134,18 +1142,19 @@ static struct clocksource clocksource_tsc = {
void mark_tsc_unstable(char *reason)
{
- if (!tsc_unstable) {
- tsc_unstable = 1;
+ if (tsc_unstable)
+ return;
+ tsc_unstable = 1;
+ if (using_native_sched_clock())
clear_sched_clock_stable();
- disable_sched_clock_irqtime();
- pr_info("Marking TSC unstable due to %s\n", reason);
- /* Change only the rating, when not registered */
- if (clocksource_tsc.mult)
- clocksource_mark_unstable(&clocksource_tsc);
- else {
- clocksource_tsc.flags |= CLOCK_SOURCE_UNSTABLE;
- clocksource_tsc.rating = 0;
- }
+ disable_sched_clock_irqtime();
+ pr_info("Marking TSC unstable due to %s\n", reason);
+ /* Change only the rating, when not registered */
+ if (clocksource_tsc.mult)
+ clocksource_mark_unstable(&clocksource_tsc);
+ else {
+ clocksource_tsc.flags |= CLOCK_SOURCE_UNSTABLE;
+ clocksource_tsc.rating = 0;
}
}
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 16:36 ` Peter Zijlstra
@ 2017-02-27 17:27 ` Paolo Bonzini
2017-02-27 17:40 ` Thomas Gleixner
0 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2017-02-27 17:27 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Wanpeng Li, Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner,
Borislav Petkov
On 27/02/2017 17:36, Peter Zijlstra wrote:
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 4e95b2e0d95f..bc3bbb6a8ab0 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -555,10 +555,8 @@ static void early_init_amd(struct cpuinfo_x86 *c)
> if (c->x86_power & (1 << 8)) {
> set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> - if (check_tsc_unstable())
> - clear_sched_clock_stable();
> } else {
> - clear_sched_clock_stable();
> + mark_tsc_unstable("not invariant");
> }
>
> /* Bit 12 of 8000_0007 edx is accumulated power mechanism. */
> diff --git a/arch/x86/kernel/cpu/centaur.c b/arch/x86/kernel/cpu/centaur.c
> index 2c234a6d94c4..0fdff183aa30 100644
> --- a/arch/x86/kernel/cpu/centaur.c
> +++ b/arch/x86/kernel/cpu/centaur.c
> @@ -105,7 +105,7 @@ static void early_init_centaur(struct cpuinfo_x86 *c)
> set_cpu_cap(c, X86_FEATURE_SYSENTER32);
> #endif
>
> - clear_sched_clock_stable();
> + mark_tsc_unstable("not invariant");
> }
>
> static void init_centaur(struct cpuinfo_x86 *c)
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index f07005e6f461..2b7ff648ea25 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -86,7 +86,7 @@ static void default_init(struct cpuinfo_x86 *c)
> strcpy(c->x86_model_id, "386");
> }
> #endif
> - clear_sched_clock_stable();
> + mark_tsc_unstable("not invariant");
> }
>
> static const struct cpu_dev default_cpu = {
> @@ -1076,7 +1076,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
> if (this_cpu->c_init)
> this_cpu->c_init(c);
> else
> - clear_sched_clock_stable();
> + mark_tsc_unstable("not invariant");
>
> /* Disable the PN if appropriate */
> squash_the_stupid_serial_number(c);
> diff --git a/arch/x86/kernel/cpu/cyrix.c b/arch/x86/kernel/cpu/cyrix.c
> index 47416f959a48..35057d67e864 100644
> --- a/arch/x86/kernel/cpu/cyrix.c
> +++ b/arch/x86/kernel/cpu/cyrix.c
> @@ -184,7 +184,7 @@ static void early_init_cyrix(struct cpuinfo_x86 *c)
> set_cpu_cap(c, X86_FEATURE_CYRIX_ARR);
> break;
> }
> - clear_sched_clock_stable();
> + mark_tsc_unstable("not invariant");
> }
>
> static void init_cyrix(struct cpuinfo_x86 *c)
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 017ecd3bb553..e0e192e43a4c 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -161,10 +161,8 @@ static void early_init_intel(struct cpuinfo_x86 *c)
> if (c->x86_power & (1 << 8)) {
> set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> - if (check_tsc_unstable())
> - clear_sched_clock_stable();
> } else {
> - clear_sched_clock_stable();
> + mark_tsc_unstable("not invariant");
> }
>
> /* Penwell and Cloverview have the TSC which doesn't sleep on S3 */
Doh, these are called _before_ kvmclock_init. But perhaps they can all
be replaced by something like this:
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 2724dc82f992..3080b6877190 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1398,6 +1398,9 @@ void __init tsc_init(void)
use_tsc_delay();
+ if (!boot_cpu_has(X86_FEATURE_NONSTOP_TSC))
+ mark_tsc_unstable("not invariant");
+
if (unsynchronized_tsc())
mark_tsc_unstable("TSCs unsynchronized");
The rest seems nice.
Paolo
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 17:27 ` Paolo Bonzini
@ 2017-02-27 17:40 ` Thomas Gleixner
2017-02-27 19:06 ` Paolo Bonzini
0 siblings, 1 reply; 29+ messages in thread
From: Thomas Gleixner @ 2017-02-27 17:40 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Zijlstra, Wanpeng Li, Mike Galbraith, LKML, Ingo Molnar,
Borislav Petkov
On Mon, 27 Feb 2017, Paolo Bonzini wrote:
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 2724dc82f992..3080b6877190 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -1398,6 +1398,9 @@ void __init tsc_init(void)
>
> use_tsc_delay();
>
> + if (!boot_cpu_has(X86_FEATURE_NONSTOP_TSC))
> + mark_tsc_unstable("not invariant");
Errm, no.
That makes TSC unusable for systems which do not go into C/P states in
which the TSC stops. There is a world outside KVM ....
Thanks,
tglx
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 17:40 ` Thomas Gleixner
@ 2017-02-27 19:06 ` Paolo Bonzini
2017-02-27 20:35 ` Peter Zijlstra
0 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2017-02-27 19:06 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Peter Zijlstra, Wanpeng Li, Mike Galbraith, LKML, Ingo Molnar,
Borislav Petkov
----- Original Message -----
> From: "Thomas Gleixner" <tglx@linutronix.de>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: "Peter Zijlstra" <peterz@infradead.org>, "Wanpeng Li" <kernellwp@gmail.com>, "Mike Galbraith" <efault@gmx.de>,
> "LKML" <linux-kernel@vger.kernel.org>, "Ingo Molnar" <mingo@elte.hu>, "Borislav Petkov" <bp@alien8.de>
> Sent: Monday, February 27, 2017 6:40:46 PM
> Subject: Re: tip.today - scheduler bam boom crash (cpu hotplug)
>
> On Mon, 27 Feb 2017, Paolo Bonzini wrote:
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > index 2724dc82f992..3080b6877190 100644
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -1398,6 +1398,9 @@ void __init tsc_init(void)
> >
> > use_tsc_delay();
> >
> > + if (!boot_cpu_has(X86_FEATURE_NONSTOP_TSC))
> > + mark_tsc_unstable("not invariant");
>
> Errm, no.
>
> That makes TSC unusable for systems which do not go into C/P states in
> which the TSC stops. There is a world outside KVM ....
Actually I was surprised too by Peter's patch, as it was adding
mark_tsc_unstable pretty much everywhere that didn't have nonstop TSC.
But hopefully it would still be okay to call clear_sched_clock_stable
in tsc_init, in the same way.
Paolo
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 19:06 ` Paolo Bonzini
@ 2017-02-27 20:35 ` Peter Zijlstra
0 siblings, 0 replies; 29+ messages in thread
From: Peter Zijlstra @ 2017-02-27 20:35 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Thomas Gleixner, Wanpeng Li, Mike Galbraith, LKML, Ingo Molnar,
Borislav Petkov
On Mon, Feb 27, 2017 at 02:06:27PM -0500, Paolo Bonzini wrote:
> > > + if (!boot_cpu_has(X86_FEATURE_NONSTOP_TSC))
> > > + mark_tsc_unstable("not invariant");
> >
> > Errm, no.
> >
> > That makes TSC unusable for systems which do not go into C/P states in
> > which the TSC stops. There is a world outside KVM ....
>
> Actually I was surprised too by Peter's patch, as it was adding
> mark_tsc_unstable pretty much everywhere that didn't have nonstop TSC.
> But hopefully it would still be okay to call clear_sched_clock_stable
> in tsc_init, in the same way.
I was preserving the sched_clock behaviour. That said, maybe we can
simply remove all those extra checks and only rely on the current
mark_tsc_unstable() calls.
I'll just have to sit down and consider the various cases..
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 16:11 ` Paolo Bonzini
2017-02-27 16:36 ` Peter Zijlstra
@ 2017-02-28 1:51 ` Wanpeng Li
2017-02-28 8:08 ` Peter Zijlstra
1 sibling, 1 reply; 29+ messages in thread
From: Wanpeng Li @ 2017-02-28 1:51 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Zijlstra, Mike Galbraith, LKML, Ingo Molnar,
Thomas Gleixner, Borislav Petkov
2017-02-28 0:11 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 27/02/2017 16:59, Peter Zijlstra wrote:
>> OK, so if !KVM_FEATURE_CLOCKSOURCE_STABLE_BIT nothing is stable, but if
>> it is set, TSC might still not be stable, but kvm_clock_read() is.
>>
>>> However, if kvmclock is stable, we know that the sched clock is stable.
>> Right, so the problem is that we only ever want to allow marking
>> unstable -- once its found unstable, for whatever reason, we should
>> never allow going stable. The corollary of this proposition is that we
>> must start out assuming it will become stable. And to avoid actually
>> using unstable TSC we do a 3 state bringup:
>>
>> 1) sched_clock_running = 0, __stable_early = 1, __stable = 0
>> 2) sched_clock_running = 1 (__stable is effective, iow, we run unstable)
>> 3) sched_clock_running = 2 (__stable <- __stable_early)
>>
>> 2) happens 'early' but is 'safe'.
>> 3) happens 'late', after we've brought up SMP and probed TSC
>>
>> Between there, we should have detected the most common TSC wreckage and
>> made sure to not then switch to 'stable' at 3.
>>
>> Now the problem appears to be that we assume sched_clock will use RDTSC
>> (native_sched_clock) while sched_clock is a paravirt op.
>>
>> Now, I've not yet figured out the ordering between when we set
>> pv_time_ops.sched_clock and when we do the 'normal' TSC init stuff.
>
> I think the ordering is fine:
>
> - pv_time_ops.sched_clock is set here:
>
> start_kernel (init/main.c line 509)
> setup_arch
> kvmclock_init
> kvm_sched_clock_init
>
> - TSC can be declared unstable only after this:
>
> start_kernel (init/main.c line 628)
> late_time_init
> tsc_init
>
> So by the time the tsc_cs_mark_unstable or mark_tsc_unstable can call
> clear_sched_clock_stable, pv_time_ops.sched_clock has been set.
>
>> But it appears to me, we should not be calling
>> clear_sched_clock_stable() on TSC bits when we don't end up using
>> native_sched_clock().
>
> Yes, this makes sense.
How about something like below, we delay sched clock stable check to
tsc_init() if we run in VM, it calls clear_sched_clock_stable() if
there is no invariant tsc bit when we end up using
native_sched_clock(), otherwise, kvmclock will determine if it is
stable depends on KVM_FEATURE_CLOCKSOURCE_STABLE_BIT /
PVCLOCK_TSC_STABLE_BIT :
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 4e95b2e..ed8eda4 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -557,7 +557,7 @@ static void early_init_amd(struct cpuinfo_x86 *c)
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
if (check_tsc_unstable())
clear_sched_clock_stable();
- } else {
+ } else if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
clear_sched_clock_stable();
}
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 017ecd3..1927f5f 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -163,7 +163,7 @@ static void early_init_intel(struct cpuinfo_x86 *c)
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
if (check_tsc_unstable())
clear_sched_clock_stable();
- } else {
+ } else if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
clear_sched_clock_stable();
}
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 2724dc8..68149f5 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -326,9 +326,16 @@ unsigned long long sched_clock(void)
{
return paravirt_sched_clock();
}
+
+static inline bool using_native_sched_clock(void)
+{
+ return pv_time_ops.sched_clock == native_sched_clock;
+}
#else
unsigned long long
sched_clock(void) __attribute__((alias("native_sched_clock")));
+
+static inline bool using_native_sched_clock(void) { return true; }
#endif
int check_tsc_unstable(void)
@@ -1398,6 +1405,11 @@ void __init tsc_init(void)
use_tsc_delay();
+ if (using_native_sched_clock() &&
+ boot_cpu_has(X86_FEATURE_HYPERVISOR) &&
+ !boot_cpu_has(X86_FEATURE_NONSTOP_TSC))
+ clear_sched_clock_stable();
+
if (unsynchronized_tsc())
mark_tsc_unstable("TSCs unsynchronized");
Regards,
Wanpeng Li
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-28 1:51 ` Wanpeng Li
@ 2017-02-28 8:08 ` Peter Zijlstra
2017-02-28 8:11 ` Wanpeng Li
0 siblings, 1 reply; 29+ messages in thread
From: Peter Zijlstra @ 2017-02-28 8:08 UTC (permalink / raw)
To: Wanpeng Li
Cc: Paolo Bonzini, Mike Galbraith, LKML, Ingo Molnar,
Thomas Gleixner, Borislav Petkov
On Tue, Feb 28, 2017 at 09:51:07AM +0800, Wanpeng Li wrote:
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 4e95b2e..ed8eda4 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -557,7 +557,7 @@ static void early_init_amd(struct cpuinfo_x86 *c)
> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> if (check_tsc_unstable())
> clear_sched_clock_stable();
> - } else {
> + } else if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
> clear_sched_clock_stable();
> }
That's wrong, you can have HYPERVISOR and still use
native_sched_clock() (lguest does that for one).
I suspect we can do something like the below. Since we fixed the
clocksource watchdog to mark TSC unstable, and we're already fairly
careful with using TSC for timekeeping anyway.
---
arch/x86/kernel/cpu/amd.c | 4 ----
arch/x86/kernel/cpu/centaur.c | 2 --
arch/x86/kernel/cpu/common.c | 3 ---
arch/x86/kernel/cpu/cyrix.c | 1 -
arch/x86/kernel/cpu/intel.c | 4 ----
arch/x86/kernel/cpu/transmeta.c | 2 --
arch/x86/kernel/tsc.c | 35 +++++++++++++++++++++++------------
7 files changed, 23 insertions(+), 28 deletions(-)
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 35a5d5dca2fa..c36140d788fe 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -556,10 +556,6 @@ static void early_init_amd(struct cpuinfo_x86 *c)
if (c->x86_power & (1 << 8)) {
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
- if (check_tsc_unstable())
- clear_sched_clock_stable();
- } else {
- clear_sched_clock_stable();
}
/* Bit 12 of 8000_0007 edx is accumulated power mechanism. */
diff --git a/arch/x86/kernel/cpu/centaur.c b/arch/x86/kernel/cpu/centaur.c
index 106bd3318121..44207b71fee1 100644
--- a/arch/x86/kernel/cpu/centaur.c
+++ b/arch/x86/kernel/cpu/centaur.c
@@ -105,8 +105,6 @@ static void early_init_centaur(struct cpuinfo_x86 *c)
#ifdef CONFIG_X86_64
set_cpu_cap(c, X86_FEATURE_SYSENTER32);
#endif
-
- clear_sched_clock_stable();
}
static void init_centaur(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index c188ae5a5d9f..0209907a63b1 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -88,7 +88,6 @@ static void default_init(struct cpuinfo_x86 *c)
strcpy(c->x86_model_id, "386");
}
#endif
- clear_sched_clock_stable();
}
static const struct cpu_dev default_cpu = {
@@ -1077,8 +1076,6 @@ static void identify_cpu(struct cpuinfo_x86 *c)
*/
if (this_cpu->c_init)
this_cpu->c_init(c);
- else
- clear_sched_clock_stable();
/* Disable the PN if appropriate */
squash_the_stupid_serial_number(c);
diff --git a/arch/x86/kernel/cpu/cyrix.c b/arch/x86/kernel/cpu/cyrix.c
index 0a3bc19de017..a70fd61095f8 100644
--- a/arch/x86/kernel/cpu/cyrix.c
+++ b/arch/x86/kernel/cpu/cyrix.c
@@ -185,7 +185,6 @@ static void early_init_cyrix(struct cpuinfo_x86 *c)
set_cpu_cap(c, X86_FEATURE_CYRIX_ARR);
break;
}
- clear_sched_clock_stable();
}
static void init_cyrix(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fe0a615a051b..063197771b8d 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -162,10 +162,6 @@ static void early_init_intel(struct cpuinfo_x86 *c)
if (c->x86_power & (1 << 8)) {
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
- if (check_tsc_unstable())
- clear_sched_clock_stable();
- } else {
- clear_sched_clock_stable();
}
/* Penwell and Cloverview have the TSC which doesn't sleep on S3 */
diff --git a/arch/x86/kernel/cpu/transmeta.c b/arch/x86/kernel/cpu/transmeta.c
index 8457b4978668..d77d07ab310b 100644
--- a/arch/x86/kernel/cpu/transmeta.c
+++ b/arch/x86/kernel/cpu/transmeta.c
@@ -16,8 +16,6 @@ static void early_init_transmeta(struct cpuinfo_x86 *c)
if (xlvl >= 0x80860001)
c->x86_capability[CPUID_8086_0001_EDX] = cpuid_edx(0x80860001);
}
-
- clear_sched_clock_stable();
}
static void init_transmeta(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 46bcda4cb1c2..5ca0f52e1ba1 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -327,9 +327,16 @@ unsigned long long sched_clock(void)
{
return paravirt_sched_clock();
}
+
+static inline bool using_native_sched_clock(void)
+{
+ return pv_time_ops.sched_clock == native_sched_clock;
+}
#else
unsigned long long
sched_clock(void) __attribute__((alias("native_sched_clock")));
+
+static inline bool using_native_sched_clock(void) { return true; }
#endif
int check_tsc_unstable(void)
@@ -1112,8 +1119,10 @@ static void tsc_cs_mark_unstable(struct clocksource *cs)
{
if (tsc_unstable)
return;
+
tsc_unstable = 1;
- clear_sched_clock_stable();
+ if (using_native_sched_clock())
+ clear_sched_clock_stable();
disable_sched_clock_irqtime();
pr_info("Marking TSC unstable due to clocksource watchdog\n");
}
@@ -1135,18 +1144,20 @@ static struct clocksource clocksource_tsc = {
void mark_tsc_unstable(char *reason)
{
- if (!tsc_unstable) {
- tsc_unstable = 1;
+ if (tsc_unstable)
+ return;
+
+ tsc_unstable = 1;
+ if (using_native_sched_clock())
clear_sched_clock_stable();
- disable_sched_clock_irqtime();
- pr_info("Marking TSC unstable due to %s\n", reason);
- /* Change only the rating, when not registered */
- if (clocksource_tsc.mult)
- clocksource_mark_unstable(&clocksource_tsc);
- else {
- clocksource_tsc.flags |= CLOCK_SOURCE_UNSTABLE;
- clocksource_tsc.rating = 0;
- }
+ disable_sched_clock_irqtime();
+ pr_info("Marking TSC unstable due to %s\n", reason);
+ /* Change only the rating, when not registered */
+ if (clocksource_tsc.mult)
+ clocksource_mark_unstable(&clocksource_tsc);
+ else {
+ clocksource_tsc.flags |= CLOCK_SOURCE_UNSTABLE;
+ clocksource_tsc.rating = 0;
}
}
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-28 8:08 ` Peter Zijlstra
@ 2017-02-28 8:11 ` Wanpeng Li
2017-03-01 13:39 ` Wanpeng Li
0 siblings, 1 reply; 29+ messages in thread
From: Wanpeng Li @ 2017-02-28 8:11 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Paolo Bonzini, Mike Galbraith, LKML, Ingo Molnar,
Thomas Gleixner, Borislav Petkov
2017-02-28 16:08 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Tue, Feb 28, 2017 at 09:51:07AM +0800, Wanpeng Li wrote:
>> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
>> index 4e95b2e..ed8eda4 100644
>> --- a/arch/x86/kernel/cpu/amd.c
>> +++ b/arch/x86/kernel/cpu/amd.c
>> @@ -557,7 +557,7 @@ static void early_init_amd(struct cpuinfo_x86 *c)
>> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
>> if (check_tsc_unstable())
>> clear_sched_clock_stable();
>> - } else {
>> + } else if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
>> clear_sched_clock_stable();
>> }
>
> That's wrong, you can have HYPERVISOR and still use
> native_sched_clock() (lguest does that for one).
My posting delay to check this in tsc_init().
Regards,
Wanpeng Li
>
>
> I suspect we can do something like the below. Since we fixed the
> clocksource watchdog to mark TSC unstable, and we're already fairly
> careful with using TSC for timekeeping anyway.
>
> ---
> arch/x86/kernel/cpu/amd.c | 4 ----
> arch/x86/kernel/cpu/centaur.c | 2 --
> arch/x86/kernel/cpu/common.c | 3 ---
> arch/x86/kernel/cpu/cyrix.c | 1 -
> arch/x86/kernel/cpu/intel.c | 4 ----
> arch/x86/kernel/cpu/transmeta.c | 2 --
> arch/x86/kernel/tsc.c | 35 +++++++++++++++++++++++------------
> 7 files changed, 23 insertions(+), 28 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 35a5d5dca2fa..c36140d788fe 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -556,10 +556,6 @@ static void early_init_amd(struct cpuinfo_x86 *c)
> if (c->x86_power & (1 << 8)) {
> set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> - if (check_tsc_unstable())
> - clear_sched_clock_stable();
> - } else {
> - clear_sched_clock_stable();
> }
>
> /* Bit 12 of 8000_0007 edx is accumulated power mechanism. */
> diff --git a/arch/x86/kernel/cpu/centaur.c b/arch/x86/kernel/cpu/centaur.c
> index 106bd3318121..44207b71fee1 100644
> --- a/arch/x86/kernel/cpu/centaur.c
> +++ b/arch/x86/kernel/cpu/centaur.c
> @@ -105,8 +105,6 @@ static void early_init_centaur(struct cpuinfo_x86 *c)
> #ifdef CONFIG_X86_64
> set_cpu_cap(c, X86_FEATURE_SYSENTER32);
> #endif
> -
> - clear_sched_clock_stable();
> }
>
> static void init_centaur(struct cpuinfo_x86 *c)
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index c188ae5a5d9f..0209907a63b1 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -88,7 +88,6 @@ static void default_init(struct cpuinfo_x86 *c)
> strcpy(c->x86_model_id, "386");
> }
> #endif
> - clear_sched_clock_stable();
> }
>
> static const struct cpu_dev default_cpu = {
> @@ -1077,8 +1076,6 @@ static void identify_cpu(struct cpuinfo_x86 *c)
> */
> if (this_cpu->c_init)
> this_cpu->c_init(c);
> - else
> - clear_sched_clock_stable();
>
> /* Disable the PN if appropriate */
> squash_the_stupid_serial_number(c);
> diff --git a/arch/x86/kernel/cpu/cyrix.c b/arch/x86/kernel/cpu/cyrix.c
> index 0a3bc19de017..a70fd61095f8 100644
> --- a/arch/x86/kernel/cpu/cyrix.c
> +++ b/arch/x86/kernel/cpu/cyrix.c
> @@ -185,7 +185,6 @@ static void early_init_cyrix(struct cpuinfo_x86 *c)
> set_cpu_cap(c, X86_FEATURE_CYRIX_ARR);
> break;
> }
> - clear_sched_clock_stable();
> }
>
> static void init_cyrix(struct cpuinfo_x86 *c)
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index fe0a615a051b..063197771b8d 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -162,10 +162,6 @@ static void early_init_intel(struct cpuinfo_x86 *c)
> if (c->x86_power & (1 << 8)) {
> set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> - if (check_tsc_unstable())
> - clear_sched_clock_stable();
> - } else {
> - clear_sched_clock_stable();
> }
>
> /* Penwell and Cloverview have the TSC which doesn't sleep on S3 */
> diff --git a/arch/x86/kernel/cpu/transmeta.c b/arch/x86/kernel/cpu/transmeta.c
> index 8457b4978668..d77d07ab310b 100644
> --- a/arch/x86/kernel/cpu/transmeta.c
> +++ b/arch/x86/kernel/cpu/transmeta.c
> @@ -16,8 +16,6 @@ static void early_init_transmeta(struct cpuinfo_x86 *c)
> if (xlvl >= 0x80860001)
> c->x86_capability[CPUID_8086_0001_EDX] = cpuid_edx(0x80860001);
> }
> -
> - clear_sched_clock_stable();
> }
>
> static void init_transmeta(struct cpuinfo_x86 *c)
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 46bcda4cb1c2..5ca0f52e1ba1 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -327,9 +327,16 @@ unsigned long long sched_clock(void)
> {
> return paravirt_sched_clock();
> }
> +
> +static inline bool using_native_sched_clock(void)
> +{
> + return pv_time_ops.sched_clock == native_sched_clock;
> +}
> #else
> unsigned long long
> sched_clock(void) __attribute__((alias("native_sched_clock")));
> +
> +static inline bool using_native_sched_clock(void) { return true; }
> #endif
>
> int check_tsc_unstable(void)
> @@ -1112,8 +1119,10 @@ static void tsc_cs_mark_unstable(struct clocksource *cs)
> {
> if (tsc_unstable)
> return;
> +
> tsc_unstable = 1;
> - clear_sched_clock_stable();
> + if (using_native_sched_clock())
> + clear_sched_clock_stable();
> disable_sched_clock_irqtime();
> pr_info("Marking TSC unstable due to clocksource watchdog\n");
> }
> @@ -1135,18 +1144,20 @@ static struct clocksource clocksource_tsc = {
>
> void mark_tsc_unstable(char *reason)
> {
> - if (!tsc_unstable) {
> - tsc_unstable = 1;
> + if (tsc_unstable)
> + return;
> +
> + tsc_unstable = 1;
> + if (using_native_sched_clock())
> clear_sched_clock_stable();
> - disable_sched_clock_irqtime();
> - pr_info("Marking TSC unstable due to %s\n", reason);
> - /* Change only the rating, when not registered */
> - if (clocksource_tsc.mult)
> - clocksource_mark_unstable(&clocksource_tsc);
> - else {
> - clocksource_tsc.flags |= CLOCK_SOURCE_UNSTABLE;
> - clocksource_tsc.rating = 0;
> - }
> + disable_sched_clock_irqtime();
> + pr_info("Marking TSC unstable due to %s\n", reason);
> + /* Change only the rating, when not registered */
> + if (clocksource_tsc.mult)
> + clocksource_mark_unstable(&clocksource_tsc);
> + else {
> + clocksource_tsc.flags |= CLOCK_SOURCE_UNSTABLE;
> + clocksource_tsc.rating = 0;
> }
> }
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-28 8:11 ` Wanpeng Li
@ 2017-03-01 13:39 ` Wanpeng Li
2017-03-01 14:17 ` Peter Zijlstra
0 siblings, 1 reply; 29+ messages in thread
From: Wanpeng Li @ 2017-03-01 13:39 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Paolo Bonzini, Mike Galbraith, LKML, Ingo Molnar,
Thomas Gleixner, Borislav Petkov
2017-02-28 16:11 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> 2017-02-28 16:08 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
>> On Tue, Feb 28, 2017 at 09:51:07AM +0800, Wanpeng Li wrote:
>>> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
>>> index 4e95b2e..ed8eda4 100644
>>> --- a/arch/x86/kernel/cpu/amd.c
>>> +++ b/arch/x86/kernel/cpu/amd.c
>>> @@ -557,7 +557,7 @@ static void early_init_amd(struct cpuinfo_x86 *c)
>>> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
>>> if (check_tsc_unstable())
>>> clear_sched_clock_stable();
>>> - } else {
>>> + } else if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
>>> clear_sched_clock_stable();
>>> }
>>
>> That's wrong, you can have HYPERVISOR and still use
>> native_sched_clock() (lguest does that for one).
>
> My posting delay to check this in tsc_init().
Does it make sense?
Regards,
Wanpeng Li
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-03-01 13:39 ` Wanpeng Li
@ 2017-03-01 14:17 ` Peter Zijlstra
0 siblings, 0 replies; 29+ messages in thread
From: Peter Zijlstra @ 2017-03-01 14:17 UTC (permalink / raw)
To: Wanpeng Li
Cc: Paolo Bonzini, Mike Galbraith, LKML, Ingo Molnar,
Thomas Gleixner, Borislav Petkov
On Wed, Mar 01, 2017 at 09:39:18PM +0800, Wanpeng Li wrote:
> 2017-02-28 16:11 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> > 2017-02-28 16:08 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> >> On Tue, Feb 28, 2017 at 09:51:07AM +0800, Wanpeng Li wrote:
> >>> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> >>> index 4e95b2e..ed8eda4 100644
> >>> --- a/arch/x86/kernel/cpu/amd.c
> >>> +++ b/arch/x86/kernel/cpu/amd.c
> >>> @@ -557,7 +557,7 @@ static void early_init_amd(struct cpuinfo_x86 *c)
> >>> set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> >>> if (check_tsc_unstable())
> >>> clear_sched_clock_stable();
> >>> - } else {
> >>> + } else if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
> >>> clear_sched_clock_stable();
> >>> }
> >>
> >> That's wrong, you can have HYPERVISOR and still use
> >> native_sched_clock() (lguest does that for one).
> >
> > My posting delay to check this in tsc_init().
>
> Does it make sense?
It makes confusion from where I'm sitting. Please look at that latest
patch I send. I think it should work, but in case you have doubts, try
and find the holes.
http://lkml.kernel.org/r/20170228080855.GV6515@twins.programming.kicks-ass.net
That marries the sched_clock stable to the existing TSC stable machinery
and removes a bunch of quirks.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 12:43 ` Peter Zijlstra
2017-02-27 12:50 ` Paolo Bonzini
@ 2017-02-27 13:48 ` Wanpeng Li
2017-02-27 14:05 ` Peter Zijlstra
1 sibling, 1 reply; 29+ messages in thread
From: Wanpeng Li @ 2017-02-27 13:48 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner,
Borislav Petkov, Paolo Bonzini
2017-02-27 20:43 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Mon, Feb 27, 2017 at 08:30:11PM +0800, Wanpeng Li wrote:
>
>> > diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
>> > index 2a5cafd..542710b 100644
>> > --- a/arch/x86/kernel/kvmclock.c
>> > +++ b/arch/x86/kernel/kvmclock.c
>> > @@ -107,12 +107,12 @@ static inline void kvm_sched_clock_init(bool stable)
>> > {
>> > if (!stable) {
>> > pv_time_ops.sched_clock = kvm_clock_read;
>> > + clear_sched_clock_stable();
>> > return;
>> > }
>> >
>> > kvm_sched_clock_offset = kvm_clock_read();
>> > pv_time_ops.sched_clock = kvm_sched_clock_read;
>> > - set_sched_clock_stable();
>>
>> This results in sched clock always unstable for kvm guest since there
>> is no invariant tsc cpuid bit exposed for kvm guest currently.
>
> What the heck is KVM_FEATURE_CLOCKSOURCE_STABLE_BIT /
> PVCLOCK_TSC_STABLE_BIT about then?
>
>> How about something like below(untested):
>
> I have no clue what that tries to do.
The patch tries to mark sched clock stable if
KVM_FEATURE_CLOCKSOURCE_STABLE_BIT / PVCLOCK_TSC_STABLE_BIT is set in
kvm guest even if there is no invariant tsc cpuid bit.
Regards,
Wanpeng Li
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: tip.today - scheduler bam boom crash (cpu hotplug)
2017-02-27 13:48 ` Wanpeng Li
@ 2017-02-27 14:05 ` Peter Zijlstra
0 siblings, 0 replies; 29+ messages in thread
From: Peter Zijlstra @ 2017-02-27 14:05 UTC (permalink / raw)
To: Wanpeng Li
Cc: Mike Galbraith, LKML, Ingo Molnar, Thomas Gleixner,
Borislav Petkov, Paolo Bonzini
On Mon, Feb 27, 2017 at 09:48:35PM +0800, Wanpeng Li wrote:
> 2017-02-27 20:43 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> > I have no clue what that tries to do.
>
> The patch tries to mark sched clock stable if
> KVM_FEATURE_CLOCKSOURCE_STABLE_BIT / PVCLOCK_TSC_STABLE_BIT is set in
> kvm guest even if there is no invariant tsc cpuid bit.
I just ripped out the interface to 'set stable' for good reasons. Also,
the branch you add it to doesn't in fact do clear_stable -- so who does?
Also, the sched/clock.c code shouldn't know about hypervisor anything.
So whatever it is you're doing, you're doing it wrong.
^ permalink raw reply [flat|nested] 29+ messages in thread