Skip to contentSkip to conversation

Various tsc/clocksource fixes

PATCH In review Peter Zijlstra 4 participants 20 messages Apr 30, 2018 – May 2, 2018

Subsystems (2)
x86 Timekeeping, clocksource core, NTP, alarmtimer
lore
Share & tools
lore

Peter Zijlstra

author
#0 lore raw

v2 cover Various tsc/clocksource fixes

These patches are the result of the regression Diego reported against commit:

aa83c45762a2 ("x86/tsc: Introduce early tsc clocksource")

His Core2 triggers mark_tsc_unstable() while we're running with tsc-early, which then results in us failing to unregister the clocksource.

Furthermore, since mark_tsc_unstable() only looks at the regular tsc clocksource, we don't even mark tsc_early UNSTABLE or de-rate it, resulting in it actually staying a desirable clocksource.

Fixing that is a little more tricky; albeit far less critical after we guarantee to unregister tsc-early.

Please consider these for /urgent. I've used both Fixes and stable tags on a bunch of them because the above commit is already in v4.16 and would need stable help.

These patches have been tested on both a Core2 and an IVB. On IVB I've both changed intel_idle to always report the TSC unstable and mucked about with the TSC msr from userspace to tickle the watchdog paths.

---
Changes since v1

- fix __clocksource_change_rating() locking
- removed work->kthread->watchdog hoops
- fixed up comment

author
#1 lore raw

PATCH v2 1/7 x86,tsc: Always unregister clocksource_tsc_early

Don't leave the tsc-early clocksource registered if it errors out early.

This was reported by Diego, who on his Core2 era machine got TSC invalidated while it was running with tsc-early (due to C-states). This results in keeping tsc-early with very bad effects.

Reported-and-Tested-by Diego Viola <[email protected]>
Fixes aa83c45762a2 ("x86/tsc: Introduce early tsc clocksource")
Cc [email protected]
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 arch/x86/kernel/tsc.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
arch/x86/kernel/tsc.c +6 -4
@@ -1244,7 +1244,7 @@ static void tsc_refine_calibration_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 +1297,12 @@ static void tsc_refine_calibration_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 +1312,8 @@ static int __init init_tsc_clocksource(v
 	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 +1329,7 @@ static int __init init_tsc_clocksource(v
 		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;
 	}
merge bot
#2 lore raw

[tip:timers/urgent] x86/tsc: Always unregister clocksource_tsc_early

x86/tsc: Always unregister clocksource_tsc_early
Commit-ID:  e9088adda13cd23249d4b0abb97ff8a81bf5573a
Gitweb:     https://git.kernel.org/tip/e9088adda13cd23249d4b0abb97ff8a81bf5573a
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Mon, 30 Apr 2018 12:00:09 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Wed, 2 May 2018 16:10:40 +0200

x86/tsc: Always unregister clocksource_tsc_early

Don't leave the tsc-early clocksource registered if it errors out early.

This was reported by Diego, who on his Core2 era machine got TSC invalidated while it was running with tsc-early (due to C-states). This results in keeping tsc-early with very bad effects.

Reported-and-Tested-by Diego Viola <[email protected]>
Fixes aa83c45762a2 ("x86/tsc: Introduce early tsc clocksource")
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
Reviewed-by Rafael J. Wysocki <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Link https://lkml.kernel.org/r/[email protected] open here
---
 arch/x86/kernel/tsc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
arch/x86/kernel/tsc.c +6 -4
index 91e6da48cbb6..23f36bc236ba 100644
@@ -1244,7 +1244,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 +1297,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 +1312,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 +1329,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;
 	}
author
#3 lore raw

PATCH v2 2/7 clocksource: Allow clocksource_mark_unstable() on unregisered clocksources

In reply to Peter Zijlstra

Because of how the code flips between tsc-early and tsc clocksources it might need to mark one or both unstable. The current code in mark_tsc_unstable() only worked because previously it registered the tsc clocksource once and then never touched it.

Since it now unregisters the tsc-early clocksource, it needs to know if a clocksource got unregistered and the current cs->mult test doesn't work for that. Instead use list_empty(&cs->list) to test for registration.

Furthermore, since clocksource_mark_unstable() needs to place the cs on the wd_list, it links the cs->list and cs->wd_list serialization. It must not see a clocsource registered (!empty cs->list) but already past dequeue_watchdog(). So place {en,de}queue{,_watchdog}() under the same lock.

Provided cs->list is initialized to empty, this then allows us to unconditionally use clocksource_mark_unstable(), regardless of the registration state.

Cc [email protected]
Tested-by Diego Viola <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 kernel/time/clocksource.c |   47 ++++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 16 deletions(-)
kernel/time/clocksource.c +31 -16
@@ -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,9 @@ static void __clocksource_unstable(struc
 	cs->flags &= ~(CLOCK_SOURCE_VALID_FOR_HRES | CLOCK_SOURCE_WATCHDOG);
 	cs->flags |= CLOCK_SOURCE_UNSTABLE;
 
+	if (list_empty(&cs->list))
+		return;
+
 	if (cs->mark_unstable)
 		cs->mark_unstable(cs);
 

@@ -164,7 +177,7 @@ void clocksource_mark_unstable(struct cl
 
 	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 +332,6 @@ static void clocksource_resume_watchdog(
 
 static void clocksource_enqueue_watchdog(struct clocksource *cs)
 {
-	unsigned long flags;
-
-	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 +341,6 @@ static void clocksource_enqueue_watchdog
 		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 +382,6 @@ static void clocksource_select_watchdog(
 
 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,21 +390,19 @@ static void clocksource_dequeue_watchdog
 			clocksource_stop_watchdog();
 		}
 	}
-	spin_unlock_irqrestore(&watchdog_lock, flags);
 }
 
 static int __clocksource_watchdog_kthread(void)
 {
 	struct clocksource *cs, *tmp;
 	unsigned long flags;
-	LIST_HEAD(unstable);
 	int select = 0;
 
 	spin_lock_irqsave(&watchdog_lock, flags);
 	list_for_each_entry_safe(cs, tmp, &watchdog_list, wd_list) {
 		if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
 			list_del_init(&cs->wd_list);
-			list_add(&cs->wd_list, &unstable);
+			__clocksource_change_rating(cs, 0);
 			select = 1;
 		}
 		if (cs->flags & CLOCK_SOURCE_RESELECT) {

@@ -410,11 +414,6 @@ static int __clocksource_watchdog_kthrea
 	clocksource_stop_watchdog();
 	spin_unlock_irqrestore(&watchdog_lock, flags);
 
-	/* Needs to be done outside of watchdog lock */
-	list_for_each_entry_safe(cs, tmp, &unstable, wd_list) {
-		list_del_init(&cs->wd_list);
-		__clocksource_change_rating(cs, 0);
-	}
 	return select;
 }
 

@@ -779,14 +778,19 @@ EXPORT_SYMBOL_GPL(__clocksource_update_f
  */
 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 +812,13 @@ static void __clocksource_change_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 +830,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 +845,12 @@ static int clocksource_unbind(struct clo
 		if (curr_clocksource == cs)
 			return -EBUSY;
 	}
+
+	clocksource_watchdog_lock(&flags);
 	clocksource_dequeue_watchdog(cs);
 	list_del_init(&cs->list);
+	clocksource_watchdog_unlock(&flags);
+
 	return 0;
 }
#4 lore raw

Re: [PATCH v2 2/7] clocksource: Allow clocksource_mark_unstable() on unregisered clocksources

Quoted 5 lines · Peter Zijlstra
kernel/time/clocksource.c +0 -0
@@ -119,6 +119,16 @@ static DEFINE_SPINLOCK(watchdog_lock);
 static int watchdog_running;
 static atomic_t watchdog_reset_pending;

kernel/time/clocksource.c static DEFINE_SPINLOCK(watchdog_lock); · line 130

Quoted 10 lines
kernel/time/clocksource.c +9 -0
 
+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);
+}

I can see why you made that wrapper, but where are the stubs for !CONFIG_CLOCKSOURCE_WATCHDOG?

Thanks,

tglx

author Tested-by Diego Viola
#5 lore raw

Re: [PATCH v2 2/7] clocksource: Allow clocksource_mark_unstable() on unregisered clocksources

Quoted 7 lines · Thomas Gleixner
Quoted 6 lines · Peter Zijlstra
kernel/time/clocksource.c +0 -0
@@ -119,6 +119,16 @@ static DEFINE_SPINLOCK(watchdog_lock);
 static int watchdog_running;
 static atomic_t watchdog_reset_pending;
Quoted 12 lines

kernel/time/clocksource.c static DEFINE_SPINLOCK(watchdog_lock); · line 130

Quoted 9 lines
kernel/time/clocksource.c +9 -0
+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);
+}

I can see why you made that wrapper, but where are the stubs for !CONFIG_CLOCKSOURCE_WATCHDOG?

Whoops ;-)

---
Subject: clocksource: Allow clocksource_mark_unstable() on unregisered clocksources
From: Peter Zijlstra <[email protected]>
Date: Mon Apr 23 17:28:55 CEST 2018

Because of how the code flips between tsc-early and tsc clocksources it might need to mark one or both unstable. The current code in mark_tsc_unstable() only worked because previously it registered the tsc clocksource once and then never touched it.

Since it now unregisters the tsc-early clocksource, it needs to know if a clocksource got unregistered and the current cs->mult test doesn't work for that. Instead use list_empty(&cs->list) to test for registration.

Furthermore, since clocksource_mark_unstable() needs to place the cs on the wd_list, it links the cs->list and cs->wd_list serialization. It must not see a clocsource registered (!empty cs->list) but already past dequeue_watchdog(). So place {en,de}queue{,_watchdog}() under the same lock.

Provided cs->list is initialized to empty, this then allows us to unconditionally use clocksource_mark_unstable(), regardless of the registration state.

Cc [email protected]
Tested-by Diego Viola <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 kernel/time/clocksource.c |   50 +++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 16 deletions(-)
kernel/time/clocksource.c +34 -16
@@ -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,9 @@ static void __clocksource_unstable(struc
 	cs->flags &= ~(CLOCK_SOURCE_VALID_FOR_HRES | CLOCK_SOURCE_WATCHDOG);
 	cs->flags |= CLOCK_SOURCE_UNSTABLE;
 
+	if (list_empty(&cs->list))
+		return;
+
 	if (cs->mark_unstable)
 		cs->mark_unstable(cs);
 

@@ -164,7 +177,7 @@ void clocksource_mark_unstable(struct cl
 
 	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 +332,6 @@ static void clocksource_resume_watchdog(
 
 static void clocksource_enqueue_watchdog(struct clocksource *cs)
 {
-	unsigned long flags;
-
-	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 +341,6 @@ static void clocksource_enqueue_watchdog
 		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 +382,6 @@ static void clocksource_select_watchdog(
 
 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,21 +390,19 @@ static void clocksource_dequeue_watchdog
 			clocksource_stop_watchdog();
 		}
 	}
-	spin_unlock_irqrestore(&watchdog_lock, flags);
 }
 
 static int __clocksource_watchdog_kthread(void)
 {
 	struct clocksource *cs, *tmp;
 	unsigned long flags;
-	LIST_HEAD(unstable);
 	int select = 0;
 
 	spin_lock_irqsave(&watchdog_lock, flags);
 	list_for_each_entry_safe(cs, tmp, &watchdog_list, wd_list) {
 		if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
 			list_del_init(&cs->wd_list);
-			list_add(&cs->wd_list, &unstable);
+			__clocksource_change_rating(cs, 0);
 			select = 1;
 		}
 		if (cs->flags & CLOCK_SOURCE_RESELECT) {

@@ -410,11 +414,6 @@ static int __clocksource_watchdog_kthrea
 	clocksource_stop_watchdog();
 	spin_unlock_irqrestore(&watchdog_lock, flags);
 
-	/* Needs to be done outside of watchdog lock */
-	list_for_each_entry_safe(cs, tmp, &unstable, wd_list) {
-		list_del_init(&cs->wd_list);
-		__clocksource_change_rating(cs, 0);
-	}
 	return select;
 }
 

@@ -447,6 +446,9 @@ static inline int __clocksource_watchdog
 static bool clocksource_is_watchdog(struct clocksource *cs) { return false; }
 void clocksource_mark_unstable(struct clocksource *cs) { }
 
+static void inline clocksource_watchdog_lock(unsigned long *flags) { }
+static void inline clocksource_watchdog_unlock(unsigned long *flags) { }
+
 #endif /* CONFIG_CLOCKSOURCE_WATCHDOG */
 
 /**

@@ -779,14 +781,19 @@ EXPORT_SYMBOL_GPL(__clocksource_update_f
  */
 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 +815,13 @@ static void __clocksource_change_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 +833,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 +848,12 @@ static int clocksource_unbind(struct clo
 		if (curr_clocksource == cs)
 			return -EBUSY;
 	}
+
+	clocksource_watchdog_lock(&flags);
 	clocksource_dequeue_watchdog(cs);
 	list_del_init(&cs->list);
+	clocksource_watchdog_unlock(&flags);
+
 	return 0;
 }
merge bot
#6 lore raw

[tip:timers/urgent] clocksource: Allow clocksource_mark_unstable() on unregistered clocksources

clocksource: Allow clocksource_mark_unstable() on unregistered clocksources
Commit-ID:  2aae7bcfa4104b770e6f612356adb8d66c6144d6
Gitweb:     https://git.kernel.org/tip/2aae7bcfa4104b770e6f612356adb8d66c6144d6
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Mon, 23 Apr 2018 17:28:55 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Wed, 2 May 2018 16:10:40 +0200

clocksource: Allow clocksource_mark_unstable() on unregistered clocksources

Because of how the code flips between tsc-early and tsc clocksources it might need to mark one or both unstable. The current code in mark_tsc_unstable() only worked because previously it registered the tsc clocksource once and then never touched it.

Since it now unregisters the tsc-early clocksource, it needs to know if a clocksource got unregistered and the current cs->mult test doesn't work for that. Instead use list_empty(&cs->list) to test for registration.

Furthermore, since clocksource_mark_unstable() needs to place the cs on the wd_list, it links the cs->list and cs->wd_list serialization. It must not see a clocsource registered (!empty cs->list) but already past dequeue_watchdog(). So place {en,de}queue{,_watchdog}() under the same lock.

Provided cs->list is initialized to empty, this then allows us to unconditionally use clocksource_mark_unstable(), regardless of the registration state.

Fixes aa83c45762a2 ("x86/tsc: Introduce early tsc clocksource")
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
Reviewed-by Rafael J. Wysocki <[email protected]>
Tested-by Diego Viola <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Link https://lkml.kernel.org/r/[email protected] open here
---
 kernel/time/clocksource.c | 50 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 16 deletions(-)
kernel/time/clocksource.c +34 -16
index 0e974cface0b..c3d2b94723dc 100644
@@ -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,9 @@ 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))
+		return;
+
 	if (cs->mark_unstable)
 		cs->mark_unstable(cs);
 

@@ -164,7 +177,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 +332,6 @@ static void clocksource_resume_watchdog(void)
 
 static void clocksource_enqueue_watchdog(struct clocksource *cs)
 {
-	unsigned long flags;
-
-	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 +341,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 +382,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,21 +390,19 @@ static void clocksource_dequeue_watchdog(struct clocksource *cs)
 			clocksource_stop_watchdog();
 		}
 	}
-	spin_unlock_irqrestore(&watchdog_lock, flags);
 }
 
 static int __clocksource_watchdog_kthread(void)
 {
 	struct clocksource *cs, *tmp;
 	unsigned long flags;
-	LIST_HEAD(unstable);
 	int select = 0;
 
 	spin_lock_irqsave(&watchdog_lock, flags);
 	list_for_each_entry_safe(cs, tmp, &watchdog_list, wd_list) {
 		if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
 			list_del_init(&cs->wd_list);
-			list_add(&cs->wd_list, &unstable);
+			__clocksource_change_rating(cs, 0);
 			select = 1;
 		}
 		if (cs->flags & CLOCK_SOURCE_RESELECT) {

@@ -410,11 +414,6 @@ static int __clocksource_watchdog_kthread(void)
 	clocksource_stop_watchdog();
 	spin_unlock_irqrestore(&watchdog_lock, flags);
 
-	/* Needs to be done outside of watchdog lock */
-	list_for_each_entry_safe(cs, tmp, &unstable, wd_list) {
-		list_del_init(&cs->wd_list);
-		__clocksource_change_rating(cs, 0);
-	}
 	return select;
 }
 

@@ -447,6 +446,9 @@ static inline int __clocksource_watchdog_kthread(void) { return 0; }
 static bool clocksource_is_watchdog(struct clocksource *cs) { return false; }
 void clocksource_mark_unstable(struct clocksource *cs) { }
 
+static void inline clocksource_watchdog_lock(unsigned long *flags) { }
+static void inline clocksource_watchdog_unlock(unsigned long *flags) { }
+
 #endif /* CONFIG_CLOCKSOURCE_WATCHDOG */
 
 /**

@@ -779,14 +781,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 +815,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 +833,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 +848,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_unlock(&flags);
+
 	return 0;
 }
author
#7 lore raw

PATCH v2 3/7 clocksource: Initialize cs->wd_list

In reply to Peter Zijlstra

A number of places relies on list_empty(&cs->wd_list), however the list_head does not get initialized. Do so upon registration, such that thereafter it is possible to rely on list_empty() correctly reflecting the list membership status.

Tested-by Diego Viola <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 kernel/time/clocksource.c |    2 ++
 1 file changed, 2 insertions(+)
kernel/time/clocksource.c +2 -0
@@ -332,6 +332,8 @@ static void clocksource_resume_watchdog(
 
 static void clocksource_enqueue_watchdog(struct clocksource *cs)
 {
+	INIT_LIST_HEAD(&cs->wd_list);
+
 	if (cs->flags & CLOCK_SOURCE_MUST_VERIFY) {
 		/* cs is a clocksource to be watched. */
 		list_add(&cs->wd_list, &watchdog_list);
#8 lore raw

Re: [PATCH v2 3/7] clocksource: Initialize cs->wd_list

Quoted 7 lines · Peter Zijlstra

A number of places relies on list_empty(&cs->wd_list), however the list_head does not get initialized. Do so upon registration, such that thereafter it is possible to rely on list_empty() correctly reflecting the list membership status.

Tested-by Diego Viola <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>

The next patch is marked for stable, but this one not. Confused

Thanks,

tglx

author
#9 lore raw

Re: [PATCH v2 3/7] clocksource: Initialize cs->wd_list

Quoted 11 lines · Thomas Gleixner
Quoted 7 lines · Peter Zijlstra

A number of places relies on list_empty(&cs->wd_list), however the list_head does not get initialized. Do so upon registration, such that thereafter it is possible to rely on list_empty() correctly reflecting the list membership status.

Tested-by Diego Viola <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>

The next patch is marked for stable, but this one not. Confused

Patch ordering issue mostly. I only marked stable those that fix the immediate/reported problem.

I noticed this while working on the code, this fixes, afaict, a pre-existing issue, where the current code relies on list_empty(&->wd_list), but we don't actually initialize it at all.

Marking it stable is probably fine, but I didn't go to great lengths to test the individual code paths affected.

merge bot
#10 lore raw

[tip:timers/urgent] clocksource: Initialize cs->wd_list

In reply to Peter Zijlstra
clocksource: Initialize cs->wd_list
Commit-ID:  5b9e886a4af97574ca3ce1147f35545da0e7afc7
Gitweb:     https://git.kernel.org/tip/5b9e886a4af97574ca3ce1147f35545da0e7afc7
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Mon, 30 Apr 2018 12:00:11 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Wed, 2 May 2018 16:10:40 +0200

clocksource: Initialize cs->wd_list

A number of places relies on list_empty(&cs->wd_list), however the list_head does not get initialized. Do so upon registration, such that thereafter it is possible to rely on list_empty() correctly reflecting the list membership status.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
Tested-by Diego Viola <[email protected]>
Reviewed-by Rafael J. Wysocki <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Link https://lkml.kernel.org/r/[email protected] open here
---
 kernel/time/clocksource.c | 2 ++
 1 file changed, 2 insertions(+)
kernel/time/clocksource.c +2 -0
index c3d2b94723dc..935f39eb8aac 100644
@@ -332,6 +332,8 @@ static void clocksource_resume_watchdog(void)
 
 static void clocksource_enqueue_watchdog(struct clocksource *cs)
 {
+	INIT_LIST_HEAD(&cs->wd_list);
+
 	if (cs->flags & CLOCK_SOURCE_MUST_VERIFY) {
 		/* cs is a clocksource to be watched. */
 		list_add(&cs->wd_list, &watchdog_list);
author
#11 lore raw

PATCH v2 4/7 x86,tsc: Fix mark_tsc_unstable()

In reply to Peter Zijlstra

mark_tsc_unstable() also needs to affect tsc_early, Now that clocksource_mark_unstable() can be used on a clocksource irrespective of its registration state, use it on both tsc_early and tsc.

This does however require cs->list to be initialized empty, otherwise it cannot tell the registation state before registation.

Tested-by Diego Viola <[email protected]>
Fixes aa83c45762a2 ("x86/tsc: Introduce early tsc clocksource")
Cc [email protected]
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 arch/x86/kernel/tsc.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
arch/x86/kernel/tsc.c +5 -7
@@ -1067,6 +1067,7 @@ static struct clocksource clocksource_ts
 	.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_ts
 	.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);
merge bot
#12 lore raw

[tip:timers/urgent] x86/tsc: Fix mark_tsc_unstable()

x86/tsc: Fix mark_tsc_unstable()
Commit-ID:  e3b4f79025e0a4eb7e2a2c7d24dadfa1e38893b0
Gitweb:     https://git.kernel.org/tip/e3b4f79025e0a4eb7e2a2c7d24dadfa1e38893b0
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Mon, 30 Apr 2018 12:00:12 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Wed, 2 May 2018 16:10:40 +0200

x86/tsc: Fix mark_tsc_unstable()

mark_tsc_unstable() also needs to affect tsc_early, Now that clocksource_mark_unstable() can be used on a clocksource irrespective of its registration state, use it on both tsc_early and tsc.

This does however require cs->list to be initialized empty, otherwise it cannot tell the registation state before registation.

Fixes aa83c45762a2 ("x86/tsc: Introduce early tsc clocksource")
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
Tested-by Diego Viola <[email protected]>
Reviewed-by Rafael J. Wysocki <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Link https://lkml.kernel.org/r/[email protected] open here
---
 arch/x86/kernel/tsc.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
arch/x86/kernel/tsc.c +5 -7
index 23f36bc236ba..74392d9d51e0 100644
@@ -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);
author
#13 lore raw

PATCH v2 5/7 clocksource: Consistent de-rate when marking unstable

In reply to Peter Zijlstra

When a registered clocksource gets marked unstable the watchdog_kthread will de-rate and re-select the clocksource. Ensure it also de-rates when getting called on an unregistered clocksource.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 kernel/time/clocksource.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
kernel/time/clocksource.c +8 -1
@@ -152,12 +152,19 @@ static void __clocksource_unstable(struc
 	cs->flags &= ~(CLOCK_SOURCE_VALID_FOR_HRES | CLOCK_SOURCE_WATCHDOG);
 	cs->flags |= CLOCK_SOURCE_UNSTABLE;
 
-	if (list_empty(&cs->list))
+	/*
+	 * If the clocksource is registered clocksource_watchdog_kthread() will
+	 * re-rate and re-select.
+	 */
+	if (list_empty(&cs->list)) {
+		cs->rating = 0;
 		return;
+	}
 
 	if (cs->mark_unstable)
 		cs->mark_unstable(cs);
 
+	/* kick clocksource_watchdog_kthread() */
 	if (finished_booting)
 		schedule_work(&watchdog_work);
 }
merge bot
#14 lore raw

[tip:timers/urgent] clocksource: Consistent de-rate when marking unstable

clocksource: Consistent de-rate when marking unstable
Commit-ID:  cd2af07d823e5287cd6c91d54337348c2a873462
Gitweb:     https://git.kernel.org/tip/cd2af07d823e5287cd6c91d54337348c2a873462
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Mon, 30 Apr 2018 12:00:13 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Wed, 2 May 2018 16:10:41 +0200

clocksource: Consistent de-rate when marking unstable

When a registered clocksource gets marked unstable the watchdog_kthread will de-rate and re-select the clocksource. Ensure it also de-rates when getting called on an unregistered clocksource.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
Reviewed-by Rafael J. Wysocki <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Link https://lkml.kernel.org/r/[email protected] open here
---
 kernel/time/clocksource.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
kernel/time/clocksource.c +8 -1
index 935f39eb8aac..605656df16c4 100644
@@ -152,12 +152,19 @@ 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))
+	/*
+	 * If the clocksource is registered clocksource_watchdog_kthread() will
+	 * re-rate and re-select.
+	 */
+	if (list_empty(&cs->list)) {
+		cs->rating = 0;
 		return;
+	}
 
 	if (cs->mark_unstable)
 		cs->mark_unstable(cs);
 
+	/* kick clocksource_watchdog_kthread() */
 	if (finished_booting)
 		schedule_work(&watchdog_work);
 }
author
#15 lore raw

PATCH v2 6/7 clocksource: Rework stale comment

In reply to Peter Zijlstra

AFAICS the hotplug code no longer uses this function.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 kernel/time/clocksource.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
kernel/time/clocksource.c +2 -4
@@ -173,10 +173,8 @@ static void __clocksource_unstable(struc
  * clocksource_mark_unstable - mark clocksource unstable via watchdog
  * @cs:		clocksource to be marked unstable
  *
- * This function is called instead of clocksource_change_rating from
- * cpu hotplug code to avoid a deadlock between the clocksource mutex
- * and the cpu hotplug mutex. It defers the update of the clocksource
- * to the watchdog thread.
+ * This function is called by the x86 TSC code to mark clocksources as unstable;
+ * it defers demotion and re-selection to a kthread.
  */
 void clocksource_mark_unstable(struct clocksource *cs)
 {
merge bot
#16 lore raw

[tip:timers/urgent] clocksource: Rework stale comment

clocksource: Rework stale comment
Commit-ID:  7dba33c6346c337aac3f7cd188137d4a6d3d1f3a
Gitweb:     https://git.kernel.org/tip/7dba33c6346c337aac3f7cd188137d4a6d3d1f3a
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Mon, 30 Apr 2018 12:00:14 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Wed, 2 May 2018 16:10:41 +0200

clocksource: Rework stale comment

AFAICS the hotplug code no longer uses this function.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
Reviewed-by Rafael J. Wysocki <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Link https://lkml.kernel.org/r/[email protected] open here
---
 kernel/time/clocksource.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
kernel/time/clocksource.c +2 -4
index 605656df16c4..84f37420fcf5 100644
@@ -173,10 +173,8 @@ static void __clocksource_unstable(struct clocksource *cs)
  * clocksource_mark_unstable - mark clocksource unstable via watchdog
  * @cs:		clocksource to be marked unstable
  *
- * This function is called instead of clocksource_change_rating from
- * cpu hotplug code to avoid a deadlock between the clocksource mutex
- * and the cpu hotplug mutex. It defers the update of the clocksource
- * to the watchdog thread.
+ * This function is called by the x86 TSC code to mark clocksources as unstable;
+ * it defers demotion and re-selection to a kthread.
  */
 void clocksource_mark_unstable(struct clocksource *cs)
 {
author
#17 lore raw

PATCH v2 7/7 clocksource: Remove kthread

In reply to Peter Zijlstra

The clocksource watchdog uses a work to spawn a kthread to run the watchdog. That is about as silly as it sounds, run the watchdog directly from the work.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 kernel/time/clocksource.c |   31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)
kernel/time/clocksource.c +10 -21
@@ -129,31 +129,19 @@ static void inline clocksource_watchdog_
 	spin_unlock_irqrestore(&watchdog_lock, *flags);
 }
 
-static int clocksource_watchdog_kthread(void *data);
-static void __clocksource_change_rating(struct clocksource *cs, int rating);
-
 /*
  * Interval: 0.5sec Threshold: 0.0625s
  */
 #define WATCHDOG_INTERVAL (HZ >> 1)
 #define WATCHDOG_THRESHOLD (NSEC_PER_SEC >> 4)
 
-static void clocksource_watchdog_work(struct work_struct *work)
-{
-	/*
-	 * If kthread_run fails the next watchdog scan over the
-	 * watchdog_list will find the unstable clock again.
-	 */
-	kthread_run(clocksource_watchdog_kthread, NULL, "kwatchdog");
-}
-
 static void __clocksource_unstable(struct clocksource *cs)
 {
 	cs->flags &= ~(CLOCK_SOURCE_VALID_FOR_HRES | CLOCK_SOURCE_WATCHDOG);
 	cs->flags |= CLOCK_SOURCE_UNSTABLE;
 
 	/*
-	 * If the clocksource is registered clocksource_watchdog_kthread() will
+	 * If the clocksource is registered clocksource_watchdog_work() will
 	 * re-rate and re-select.
 	 */
 	if (list_empty(&cs->list)) {

@@ -164,7 +152,7 @@ static void __clocksource_unstable(struc
 	if (cs->mark_unstable)
 		cs->mark_unstable(cs);
 
-	/* kick clocksource_watchdog_kthread() */
+	/* kick clocksource_watchdog_work() */
 	if (finished_booting)
 		schedule_work(&watchdog_work);
 }

@@ -174,7 +162,7 @@ static void __clocksource_unstable(struc
  * @cs:		clocksource to be marked unstable
  *
  * This function is called by the x86 TSC code to mark clocksources as unstable;
- * it defers demotion and re-selection to a kthread.
+ * it defers demotion and re-selection to a work.
  */
 void clocksource_mark_unstable(struct clocksource *cs)
 {

@@ -399,7 +387,9 @@ static void clocksource_dequeue_watchdog
 	}
 }
 
-static int __clocksource_watchdog_kthread(void)
+static void __clocksource_change_rating(struct clocksource *cs, int rating);
+
+static int __clocksource_watchdog_work(void)
 {
 	struct clocksource *cs, *tmp;
 	unsigned long flags;

@@ -424,13 +414,12 @@ static int __clocksource_watchdog_kthrea
 	return select;
 }
 
-static int clocksource_watchdog_kthread(void *data)
+static void clocksource_watchdog_work(struct work_struct *work)
 {
 	mutex_lock(&clocksource_mutex);
-	if (__clocksource_watchdog_kthread())
+	if (__clocksource_watchdog_work())
 		clocksource_select();
 	mutex_unlock(&clocksource_mutex);
-	return 0;
 }
 
 static bool clocksource_is_watchdog(struct clocksource *cs)

@@ -449,7 +438,7 @@ static void clocksource_enqueue_watchdog
 static void clocksource_select_watchdog(bool fallback) { }
 static inline void clocksource_dequeue_watchdog(struct clocksource *cs) { }
 static inline void clocksource_resume_watchdog(void) { }
-static inline int __clocksource_watchdog_kthread(void) { return 0; }
+static inline int __clocksource_watchdog_work(void) { return 0; }
 static bool clocksource_is_watchdog(struct clocksource *cs) { return false; }
 void clocksource_mark_unstable(struct clocksource *cs) { }
 

@@ -680,7 +669,7 @@ static int __init clocksource_done_booti
 	/*
 	 * Run the watchdog first to eliminate unstable clock sources
 	 */
-	__clocksource_watchdog_kthread();
+	__clocksource_watchdog_work();
 	clocksource_select();
 	mutex_unlock(&clocksource_mutex);
 	return 0;
merge bot
#18 lore raw

[tip:timers/core] clocksource: Remove kthread

clocksource: Remove kthread
Commit-ID:  7197e77abcb65a71d0b21d67beb24f153a96055e
Gitweb:     https://git.kernel.org/tip/7197e77abcb65a71d0b21d67beb24f153a96055e
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Mon, 30 Apr 2018 12:00:15 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Wed, 2 May 2018 16:11:46 +0200

clocksource: Remove kthread

The clocksource watchdog uses a work to spawn a kthread to run the watchdog. That is about as silly as it sounds, run the watchdog directly from the work.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
Reviewed-by Rafael J. Wysocki <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Link https://lkml.kernel.org/r/[email protected] open here
---
 kernel/time/clocksource.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)
kernel/time/clocksource.c +10 -21
index 84f37420fcf5..56fd2239674f 100644
@@ -129,31 +129,19 @@ 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);
-
 /*
  * Interval: 0.5sec Threshold: 0.0625s
  */
 #define WATCHDOG_INTERVAL (HZ >> 1)
 #define WATCHDOG_THRESHOLD (NSEC_PER_SEC >> 4)
 
-static void clocksource_watchdog_work(struct work_struct *work)
-{
-	/*
-	 * If kthread_run fails the next watchdog scan over the
-	 * watchdog_list will find the unstable clock again.
-	 */
-	kthread_run(clocksource_watchdog_kthread, NULL, "kwatchdog");
-}
-
 static void __clocksource_unstable(struct clocksource *cs)
 {
 	cs->flags &= ~(CLOCK_SOURCE_VALID_FOR_HRES | CLOCK_SOURCE_WATCHDOG);
 	cs->flags |= CLOCK_SOURCE_UNSTABLE;
 
 	/*
-	 * If the clocksource is registered clocksource_watchdog_kthread() will
+	 * If the clocksource is registered clocksource_watchdog_work() will
 	 * re-rate and re-select.
 	 */
 	if (list_empty(&cs->list)) {

@@ -164,7 +152,7 @@ static void __clocksource_unstable(struct clocksource *cs)
 	if (cs->mark_unstable)
 		cs->mark_unstable(cs);
 
-	/* kick clocksource_watchdog_kthread() */
+	/* kick clocksource_watchdog_work() */
 	if (finished_booting)
 		schedule_work(&watchdog_work);
 }

@@ -174,7 +162,7 @@ static void __clocksource_unstable(struct clocksource *cs)
  * @cs:		clocksource to be marked unstable
  *
  * This function is called by the x86 TSC code to mark clocksources as unstable;
- * it defers demotion and re-selection to a kthread.
+ * it defers demotion and re-selection to a work.
  */
 void clocksource_mark_unstable(struct clocksource *cs)
 {

@@ -399,7 +387,9 @@ static void clocksource_dequeue_watchdog(struct clocksource *cs)
 	}
 }
 
-static int __clocksource_watchdog_kthread(void)
+static void __clocksource_change_rating(struct clocksource *cs, int rating);
+
+static int __clocksource_watchdog_work(void)
 {
 	struct clocksource *cs, *tmp;
 	unsigned long flags;

@@ -424,13 +414,12 @@ static int __clocksource_watchdog_kthread(void)
 	return select;
 }
 
-static int clocksource_watchdog_kthread(void *data)
+static void clocksource_watchdog_work(struct work_struct *work)
 {
 	mutex_lock(&clocksource_mutex);
-	if (__clocksource_watchdog_kthread())
+	if (__clocksource_watchdog_work())
 		clocksource_select();
 	mutex_unlock(&clocksource_mutex);
-	return 0;
 }
 
 static bool clocksource_is_watchdog(struct clocksource *cs)

@@ -449,7 +438,7 @@ static void clocksource_enqueue_watchdog(struct clocksource *cs)
 static void clocksource_select_watchdog(bool fallback) { }
 static inline void clocksource_dequeue_watchdog(struct clocksource *cs) { }
 static inline void clocksource_resume_watchdog(void) { }
-static inline int __clocksource_watchdog_kthread(void) { return 0; }
+static inline int __clocksource_watchdog_work(void) { return 0; }
 static bool clocksource_is_watchdog(struct clocksource *cs) { return false; }
 void clocksource_mark_unstable(struct clocksource *cs) { }
 

@@ -683,7 +672,7 @@ static int __init clocksource_done_booting(void)
 	/*
 	 * Run the watchdog first to eliminate unstable clock sources
 	 */
-	__clocksource_watchdog_kthread();
+	__clocksource_watchdog_work();
 	clocksource_select();
 	mutex_unlock(&clocksource_mutex);
 	return 0;
Reviewed-by on the v2 series
#19 lore raw
In reply to Peter Zijlstra
Quoted 22 lines · CEST Peter Zijlstra

These patches are the result of the regression Diego reported against commit:

aa83c45762a2 ("x86/tsc: Introduce early tsc clocksource")

His Core2 triggers mark_tsc_unstable() while we're running with tsc-early, which then results in us failing to unregister the clocksource.

Furthermore, since mark_tsc_unstable() only looks at the regular tsc clocksource, we don't even mark tsc_early UNSTABLE or de-rate it, resulting in it actually staying a desirable clocksource.

Fixing that is a little more tricky; albeit far less critical after we guarantee to unregister tsc-early.

Please consider these for /urgent. I've used both Fixes and stable tags on a bunch of them because the above commit is already in v4.16 and would need stable help.

These patches have been tested on both a Core2 and an IVB. On IVB I've both changed intel_idle to always report the TSC unstable and mucked about with the TSC msr from userspace to tickle the watchdog paths.

Quoted 8 lines

---
Changes since v1

- fix __clocksource_change_rating() locking
- removed work->kthread->watchdog hoops
- fixed up comment

I've had a deeper look at the patches and have not found anything of concern in them, so for the whole series:

Reviewed-by Rafael J. Wysocki <[email protected]>