Re: tip.today - scheduler bam boom crash (cpu hotplug)

Wanpeng Li
2017-02-28T08:12:15Z

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;
> }
> }
>