| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 23 Apr 2018 09:49:43 -0300
From: Diego Viola <diego.viola@...il.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: linux-kernel@...r.kernel.org, tglx@...utronix.de,
len.brown@...el.com, rui.zhang@...el.com,
"Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: Experiencing freezes with kernel 4.16.3 on a desktop with E5500
CPU (bisect included)
On Mon, Apr 23, 2018 at 9:38 AM, Peter Zijlstra <peterz@...radead.org> wrote:
> On Mon, Apr 23, 2018 at 08:49:25AM -0300, Diego Viola wrote:
>> On Mon, Apr 23, 2018 at 8:48 AM, Peter Zijlstra <peterz@...radead.org> wrote:
>> > On Mon, Apr 23, 2018 at 08:23:24AM -0300, Diego Viola wrote:
>> >> > That's a Core2 era chip; does it actually have stable TSC ?
>> >>
>> >> I'm not sure.
>> >
>> > dmesg | grep -i tsc
>> >
>> > should be able to tell you.
>>
>> [diego@...lcore ~]$ dmesg | grep -i tsc
>> [ 0.000000] tsc: Fast TSC calibration using PIT
>> [ 0.016666] tsc: Fast TSC calibration using PIT
>> [ 0.019999] tsc: Detected 2793.087 MHz processor
>> [ 0.019999] clocksource: tsc-early: mask: 0xffffffffffffffff
>> max_cycles: 0x2842be30f1f, max_idle_ns: 440795236296 ns
>> [ 0.162058] clocksource: Switched to clocksource tsc-early
>> [ 0.300076] tsc: Marking TSC unstable due to TSC halts in idle
>> [diego@...lcore ~]$
>
> Much thanks.. I suspect there a bunch of fail when marking unstable
> before we register clocksource_tsc.
>
> The below patch is a bit ugly, but should cure a number of things; it
> compiles but hasn't otherwise been tested, can you give it a spin?
>
> ---
>
> - when TSC is unstable and we've already registered tsc-early, don't
> forget to unregister it; this then leaves us without a tsc
> clocksource entirely -- which is good.
>
> - when we call mark_tsc_unstable() before we've registered
> clocksource_tsc things go wobbly because it doesn't know about
> clocksource_tsc_early. Fix that by:
>
> - Make clocksource_mark_unstable() work for unregistered
> clocksources.
>
> - which means we have to be able to detect this; use cs.list
> for this; initialize it empty.
> - means we also have to place all cs.list manipulation under
> watchdog_lock -- bit ugly.
>
> - Make __clocksource_unstable() de-rate the clocksource.
>
> - Call clocksource_mark_unstable() on both tsc and tsc_early.
>
> This way we should either end up with a derated tsc clocksource marked
> UNSTABLE or no tsc clocksource at all, either should result in it not
> becoming the active clocksource.
>
> ---
> arch/x86/kernel/tsc.c | 22 +++++++++++-----------
> kernel/time/clocksource.c | 43 +++++++++++++++++++++++++++++++++++--------
> 2 files changed, 46 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 91e6da48cbb6..74392d9d51e0 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -1067,6 +1067,7 @@ static struct clocksource clocksource_tsc_early = {
> .resume = tsc_resume,
> .mark_unstable = tsc_cs_mark_unstable,
> .tick_stable = tsc_cs_tick_stable,
> + .list = LIST_HEAD_INIT(clocksource_tsc_early.list),
> };
>
> /*
> @@ -1086,6 +1087,7 @@ static struct clocksource clocksource_tsc = {
> .resume = tsc_resume,
> .mark_unstable = tsc_cs_mark_unstable,
> .tick_stable = tsc_cs_tick_stable,
> + .list = LIST_HEAD_INIT(clocksource_tsc.list),
> };
>
> void mark_tsc_unstable(char *reason)
> @@ -1098,13 +1100,9 @@ void mark_tsc_unstable(char *reason)
> 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;
> - }
> +
> + clocksource_mark_unstable(&clocksource_tsc_early);
> + clocksource_mark_unstable(&clocksource_tsc);
> }
>
> EXPORT_SYMBOL_GPL(mark_tsc_unstable);
> @@ -1244,7 +1242,7 @@ static void tsc_refine_calibration_work(struct work_struct *work)
>
> /* Don't bother refining TSC on unstable systems */
> if (tsc_unstable)
> - return;
> + goto unreg;
>
> /*
> * Since the work is started early in boot, we may be
> @@ -1297,11 +1295,12 @@ static void tsc_refine_calibration_work(struct work_struct *work)
>
> out:
> if (tsc_unstable)
> - return;
> + goto unreg;
>
> if (boot_cpu_has(X86_FEATURE_ART))
> art_related_clocksource = &clocksource_tsc;
> clocksource_register_khz(&clocksource_tsc, tsc_khz);
> +unreg:
> clocksource_unregister(&clocksource_tsc_early);
> }
>
> @@ -1311,8 +1310,8 @@ static int __init init_tsc_clocksource(void)
> if (!boot_cpu_has(X86_FEATURE_TSC) || tsc_disabled > 0 || !tsc_khz)
> return 0;
>
> - if (check_tsc_unstable())
> - return 0;
> + if (tsc_unstable)
> + goto unreg;
>
> if (tsc_clocksource_reliable)
> clocksource_tsc.flags &= ~CLOCK_SOURCE_MUST_VERIFY;
> @@ -1328,6 +1327,7 @@ static int __init init_tsc_clocksource(void)
> if (boot_cpu_has(X86_FEATURE_ART))
> art_related_clocksource = &clocksource_tsc;
> clocksource_register_khz(&clocksource_tsc, tsc_khz);
> +unreg:
> clocksource_unregister(&clocksource_tsc_early);
> return 0;
> }
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index 0e974cface0b..f689c9e3ff5e 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -119,6 +119,16 @@ static DEFINE_SPINLOCK(watchdog_lock);
> static int watchdog_running;
> static atomic_t watchdog_reset_pending;
>
> +static void inline clocksource_watchdog_lock(unsigned long *flags)
> +{
> + spin_lock_irqsave(&watchdog_lock, *flags);
> +}
> +
> +static void inline clocksource_watchdog_unlock(unsigned long *flags)
> +{
> + spin_unlock_irqrestore(&watchdog_lock, *flags);
> +}
> +
> static int clocksource_watchdog_kthread(void *data);
> static void __clocksource_change_rating(struct clocksource *cs, int rating);
>
> @@ -142,6 +152,13 @@ static void __clocksource_unstable(struct clocksource *cs)
> cs->flags &= ~(CLOCK_SOURCE_VALID_FOR_HRES | CLOCK_SOURCE_WATCHDOG);
> cs->flags |= CLOCK_SOURCE_UNSTABLE;
>
> + if (list_empty(&cs->list)) {
> + cs->rating = 0;
> + return;
> + }
> +
> + __clocksource_change_rating(cs, 0);
> +
> if (cs->mark_unstable)
> cs->mark_unstable(cs);
>
> @@ -164,7 +181,7 @@ void clocksource_mark_unstable(struct clocksource *cs)
>
> spin_lock_irqsave(&watchdog_lock, flags);
> if (!(cs->flags & CLOCK_SOURCE_UNSTABLE)) {
> - if (list_empty(&cs->wd_list))
> + if (!list_empty(&cs->list) && list_empty(&cs->wd_list))
> list_add(&cs->wd_list, &watchdog_list);
> __clocksource_unstable(cs);
> }
> @@ -319,9 +336,8 @@ static void clocksource_resume_watchdog(void)
>
> static void clocksource_enqueue_watchdog(struct clocksource *cs)
> {
> - unsigned long flags;
> + INIT_LIST_HEAD(&cs->wd_list);
>
> - spin_lock_irqsave(&watchdog_lock, flags);
> if (cs->flags & CLOCK_SOURCE_MUST_VERIFY) {
> /* cs is a clocksource to be watched. */
> list_add(&cs->wd_list, &watchdog_list);
> @@ -331,7 +347,6 @@ static void clocksource_enqueue_watchdog(struct clocksource *cs)
> if (cs->flags & CLOCK_SOURCE_IS_CONTINUOUS)
> cs->flags |= CLOCK_SOURCE_VALID_FOR_HRES;
> }
> - spin_unlock_irqrestore(&watchdog_lock, flags);
> }
>
> static void clocksource_select_watchdog(bool fallback)
> @@ -373,9 +388,6 @@ static void clocksource_select_watchdog(bool fallback)
>
> static void clocksource_dequeue_watchdog(struct clocksource *cs)
> {
> - unsigned long flags;
> -
> - spin_lock_irqsave(&watchdog_lock, flags);
> if (cs != watchdog) {
> if (cs->flags & CLOCK_SOURCE_MUST_VERIFY) {
> /* cs is a watched clocksource. */
> @@ -384,7 +396,6 @@ static void clocksource_dequeue_watchdog(struct clocksource *cs)
> clocksource_stop_watchdog();
> }
> }
> - spin_unlock_irqrestore(&watchdog_lock, flags);
> }
>
> static int __clocksource_watchdog_kthread(void)
> @@ -779,14 +790,19 @@ EXPORT_SYMBOL_GPL(__clocksource_update_freq_scale);
> */
> int __clocksource_register_scale(struct clocksource *cs, u32 scale, u32 freq)
> {
> + unsigned long flags;
>
> /* Initialize mult/shift and max_idle_ns */
> __clocksource_update_freq_scale(cs, scale, freq);
>
> /* Add clocksource to the clocksource list */
> mutex_lock(&clocksource_mutex);
> +
> + clocksource_watchdog_lock(&flags);
> clocksource_enqueue(cs);
> clocksource_enqueue_watchdog(cs);
> + clocksource_watchdog_unlock(&flags);
> +
> clocksource_select();
> clocksource_select_watchdog(false);
> mutex_unlock(&clocksource_mutex);
> @@ -808,8 +824,13 @@ static void __clocksource_change_rating(struct clocksource *cs, int rating)
> */
> void clocksource_change_rating(struct clocksource *cs, int rating)
> {
> + unsigned long flags;
> +
> mutex_lock(&clocksource_mutex);
> + clocksource_watchdog_lock(&flags);
> __clocksource_change_rating(cs, rating);
> + clocksource_watchdog_unlock(&flags);
> +
> clocksource_select();
> clocksource_select_watchdog(false);
> mutex_unlock(&clocksource_mutex);
> @@ -821,6 +842,8 @@ EXPORT_SYMBOL(clocksource_change_rating);
> */
> static int clocksource_unbind(struct clocksource *cs)
> {
> + unsigned long flags;
> +
> if (clocksource_is_watchdog(cs)) {
> /* Select and try to install a replacement watchdog. */
> clocksource_select_watchdog(true);
> @@ -834,8 +857,12 @@ static int clocksource_unbind(struct clocksource *cs)
> if (curr_clocksource == cs)
> return -EBUSY;
> }
> +
> + clocksource_watchdog_lock(&flags);
> clocksource_dequeue_watchdog(cs);
> list_del_init(&cs->list);
> + clocksource_watchdog_lock(&flags);
> +
> return 0;
> }
>
Yes, compiling the kernel (torvalds/linux.git HEAD) with your patch right now.
Thanks,
Diego
Powered by blists - more mailing lists