Various tsc/clocksource fixes

PATCH Superseded Peter Zijlstra 1 participant 8 messages Apr 23, 2018

Subsystems (2)

x86 Timekeeping, clocksource core, NTP, alarmtimer

lore

Share & tools

lore

Peter Zijlstra

author

2018-04-23 16:30 UTC #0 lore raw

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.

Peter Zijlstra

author

2018-04-23 16:29 UTC #1 lore raw

PATCH 1/5 x86,tsc: Always unregister clocksource_tsc_early

Don't leave the tsc-early clocksource registered if we error out early.

This was reported by Diego, who on his Core2 era machine got TSC invalidated while we were 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;
 	}

Peter Zijlstra

author

2018-04-23 16:29 UTC #2 lore raw

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

In reply to Peter Zijlstra

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

Since we now unregister the tsc-early clocksource, we need 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. We must not see a clocsource registered (!empty cs->list) but already past dequeue_watchdog(). So place {en,de}queue{,_watchdog}() under the same lock.

This then allows us to unconditionally use clocksource_mark_unstable(), regardless of the registration state.

Tested-by Diego Viola <[email protected]>

Cc [email protected]

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

---
 kernel/time/clocksource.c |   39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

kernel/time/clocksource.c +30 -9

@@ -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,7 +390,6 @@ static void clocksource_dequeue_watchdog
 			clocksource_stop_watchdog();
 		}
 	}
-	spin_unlock_irqrestore(&watchdog_lock, flags);
 }
 
 static int __clocksource_watchdog_kthread(void)

@@ -779,14 +784,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 +818,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 +836,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 +851,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;
 }

Peter Zijlstra

author

2018-04-23 16:41 UTC #3 lore raw

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

Quoted 10 lines · Peter Zijlstra

Because of how we flip between tsc-early and tsc clocksources we might need to mark one or both unstable. The current code in mark_tsc_unstable() only worked because we registered the tsc clocksource once and then never touched it.
Since we now unregister the tsc-early clocksource, we need 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.

Quoted 9 lines

Furthermore, since clocksource_mark_unstable() needs to place the cs on the wd_list, it links the cs->list and cs->wd_list serialization. We must not see a clocsource registered (!empty cs->list) but already past dequeue_watchdog(). So place {en,de}queue{,_watchdog}() under the same lock.
This then allows us to unconditionally use clocksource_mark_unstable(), regardless of the registration state.

I think we also want the below patch, either merged in or on top. Because __clocksource_change_rating() actually requires holding that lock now.

I've no idea why it says it cannot do that with the lock held. But there's more stale comments in there.

I'll actually test it later, need to head out to the Dojo soon.

kernel/time/clocksource.c +1 -7

@@ -405,14 +405,13 @@ static int __clocksource_watchdog_kthrea
 {
 	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) {

@@ -424,11 +423,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;
 }

Peter Zijlstra

author

2018-04-23 16:43 UTC #4 lore raw

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

Quoted 2 lines · Peter Zijlstra

I've no idea why it says it cannot do that with the lock held. But there's more stale comments in there.

Like that one, the x86 tsc code is the only caller of this. I could not find anything hotplug.

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

Peter Zijlstra

author

2018-04-23 16:29 UTC #5 lore raw

PATCH 3/5 clocksource: Initialize cs->wd_list

In reply to Peter Zijlstra

In a number of places we rely on list_empty(&cs->wd_list), however we do not initialize this list_head. Do so upon registration, such that thereafter we can 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);

Peter Zijlstra

author

2018-04-23 16:30 UTC #6 lore raw

PATCH 4/5 x86,tsc: Fix mark_tsc_unstable()

In reply to Peter Zijlstra

mark_tsc_unstable() also needs to affect tsc_early, now that we can use clocksource_mark_unstable() on a clocksource irrespective of its registation state, use it on both tsc_early and tsc.

This does however require we initialize cs->list to empty.

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

Peter Zijlstra

author

2018-04-23 16:30 UTC #7 lore raw

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

In reply to Peter Zijlstra

When we mark a registered clocksource unstable the watchdog_kthread will de-rate and re-select the clocksource. Ensure we also de-rate 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);
 }