REGRESSION: boot stalls on several old dual core Intel CPUs

REGRESSION: boot stalls on several old dual core Intel CPUs

[Siegfried Metz]

Dear kernel developers,

since mainline kernel 4.18 (up to the latest mainline kernel 4.18.5)
Intel Core 2 Duo processors are affected by boot stalling early in the
boot process. As it is so early there is no dmesg output (or any log).

A few users in the Arch Linux community used git bisect and tracked the
issue down to this the bad commit:
7197e77abcb65a71d0b21d67beb24f153a96055e clocksource: Remove kthread

Either reverting the bad commit or as a workaround using an additional
kernel boot parameter "clocksource=hpet" has been successfully used to
boot with Intel Core 2 processors and avoid the stalling.

clocksource= options parameters are one of "tsc", "hpet", "acpi_pm",
most of the time hpet did the trick.


Additional information:
Kernel 4.17.19 is unaffected by this issues, kernel 4.18 series and
4.19-rc1 are still affected.

Also there is the bug-report:
https://bugzilla.kernel.org/show_bug.cgi?id=200957
and a duplicate one:
https://bugzilla.kernel.org/show_bug.cgi?id=200959


Thanks for fixing this issue.

Siegfried

Re: REGRESSION: boot stalls on several old dual core Intel CPUs

[Peter Zijlstra]

On Thu, Aug 30, 2018 at 12:55:30PM +0200, Siegfried Metz wrote:
> Dear kernel developers,
> 
> since mainline kernel 4.18 (up to the latest mainline kernel 4.18.5)
> Intel Core 2 Duo processors are affected by boot stalling early in the
> boot process. As it is so early there is no dmesg output (or any log).
> 
> A few users in the Arch Linux community used git bisect and tracked the
> issue down to this the bad commit:
> 7197e77abcb65a71d0b21d67beb24f153a96055e clocksource: Remove kthread

I just dug out my core2duo laptop (Lenovo T500) and build a tip/master
kernel for it (x86_64 debian distro .config).

Seems to boot just fine.. 3/3 so far.

Any other clues?

Re: REGRESSION: boot stalls on several old dual core Intel CPUs

[Peter Zijlstra]

On Thu, Aug 30, 2018 at 03:04:39PM +0200, Peter Zijlstra wrote:
> On Thu, Aug 30, 2018 at 12:55:30PM +0200, Siegfried Metz wrote:
> > Dear kernel developers,
> > 
> > since mainline kernel 4.18 (up to the latest mainline kernel 4.18.5)
> > Intel Core 2 Duo processors are affected by boot stalling early in the
> > boot process. As it is so early there is no dmesg output (or any log).
> > 
> > A few users in the Arch Linux community used git bisect and tracked the
> > issue down to this the bad commit:
> > 7197e77abcb65a71d0b21d67beb24f153a96055e clocksource: Remove kthread
> 
> I just dug out my core2duo laptop (Lenovo T500) and build a tip/master
> kernel for it (x86_64 debian distro .config).
> 
> Seems to boot just fine.. 3/3 so far.
> 
> Any other clues?

The .config from https://bugzilla.kernel.org/attachment.cgi?id=278183
seems to boot far enough to drop me into a busybox -- it doesn't seem to
contain enough bits to actually boot my system and I can't be arsed to
figure out what's missing.

Re: REGRESSION: boot stalls on several old dual core Intel CPUs

[Kevin Shanahan]

On Thu, Aug 30, 2018 at 03:04:39PM +0200, Peter Zijlstra wrote:
> On Thu, Aug 30, 2018 at 12:55:30PM +0200, Siegfried Metz wrote:
> > Dear kernel developers,
> > 
> > since mainline kernel 4.18 (up to the latest mainline kernel 4.18.5)
> > Intel Core 2 Duo processors are affected by boot stalling early in the
> > boot process. As it is so early there is no dmesg output (or any log).
> > 
> > A few users in the Arch Linux community used git bisect and tracked the
> > issue down to this the bad commit:
> > 7197e77abcb65a71d0b21d67beb24f153a96055e clocksource: Remove kthread
> 
> I just dug out my core2duo laptop (Lenovo T500) and build a tip/master
> kernel for it (x86_64 debian distro .config).
> 
> Seems to boot just fine.. 3/3 so far.
> 
> Any other clues?

One additional data point, my affected system is a Dell Latitude E6400
laptop which has a P8400 CPU:

  vendor_id     : GenuineIntel
  cpu family    : 6
  model         : 23
  model name    : Intel(R) Core(TM)2 Duo CPU     P8400  @ 2.26GHz
  stepping      : 6
  microcode     : 0x610

Judging from what is being discussed in the Arch forums, it does seem
to related to the CPU having unstable TSC and transitioning to another
clock source.  Workarounds that seem to be reliable are either booting
with clocksource=<something_not_tsc> or with nosmp.

One person did point out that the commit that introduced the kthread
did so to remove a deadlock - is the circular locking dependency
mentioned in that commit still relevant?

commit 01548f4d3e8e94caf323a4f664eb347fd34a34ab
Author: Martin Schwidefsky <schwidefsky@de.ibm.com>
Date:   Tue Aug 18 17:09:42 2009 +0200

    clocksource: Avoid clocksource watchdog circular locking dependency

    stop_machine from a multithreaded workqueue is not allowed because
    of a circular locking dependency between cpu_down and the workqueue
    execution. Use a kernel thread to do the clocksource downgrade.

    Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
    Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
    Cc: john stultz <johnstul@us.ibm.com>
    LKML-Reference: <20090818170942.3ab80c91@skybase>
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Thanks,
Kevin.

Re: REGRESSION: boot stalls on several old dual core Intel CPUs

[Peter Zijlstra]

On Sat, Sep 01, 2018 at 11:51:26AM +0930, Kevin Shanahan wrote:
> On Thu, Aug 30, 2018 at 03:04:39PM +0200, Peter Zijlstra wrote:
> > On Thu, Aug 30, 2018 at 12:55:30PM +0200, Siegfried Metz wrote:
> > > Dear kernel developers,
> > > 
> > > since mainline kernel 4.18 (up to the latest mainline kernel 4.18.5)
> > > Intel Core 2 Duo processors are affected by boot stalling early in the
> > > boot process. As it is so early there is no dmesg output (or any log).
> > > 
> > > A few users in the Arch Linux community used git bisect and tracked the
> > > issue down to this the bad commit:
> > > 7197e77abcb65a71d0b21d67beb24f153a96055e clocksource: Remove kthread
> > 
> > I just dug out my core2duo laptop (Lenovo T500) and build a tip/master
> > kernel for it (x86_64 debian distro .config).
> > 
> > Seems to boot just fine.. 3/3 so far.
> > 
> > Any other clues?
> 
> One additional data point, my affected system is a Dell Latitude E6400
> laptop which has a P8400 CPU:
> 
>   vendor_id     : GenuineIntel
>   cpu family    : 6
>   model         : 23
>   model name    : Intel(R) Core(TM)2 Duo CPU     P8400  @ 2.26GHz
>   stepping      : 6
>   microcode     : 0x610
> 
> Judging from what is being discussed in the Arch forums, it does seem
> to related to the CPU having unstable TSC and transitioning to another
> clock source.

Yes; Core2 doesn't have stable TSC.

> Workarounds that seem to be reliable are either booting
> with clocksource=<something_not_tsc> or with nosmp.

nosmp is weird; because even on UP TSC should stop in C state.

processor_idle (acpi_idle) should mark the TSC as unstable on Core2 when
it loads (does so on my T500).

> One person did point out that the commit that introduced the kthread
> did so to remove a deadlock - is the circular locking dependency
> mentioned in that commit still relevant?
> 
> commit 01548f4d3e8e94caf323a4f664eb347fd34a34ab
> Author: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Date:   Tue Aug 18 17:09:42 2009 +0200
> 
>     clocksource: Avoid clocksource watchdog circular locking dependency
> 
>     stop_machine from a multithreaded workqueue is not allowed because
>     of a circular locking dependency between cpu_down and the workqueue
>     execution. Use a kernel thread to do the clocksource downgrade.

I cannot find stop_machine usage there; either it went away or I need to
like wake up.

Re: REGRESSION: boot stalls on several old dual core Intel CPUs

[Thomas Gleixner]

On Mon, 3 Sep 2018, Peter Zijlstra wrote:
> On Sat, Sep 01, 2018 at 11:51:26AM +0930, Kevin Shanahan wrote:
> > commit 01548f4d3e8e94caf323a4f664eb347fd34a34ab
> > Author: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > Date:   Tue Aug 18 17:09:42 2009 +0200
> > 
> >     clocksource: Avoid clocksource watchdog circular locking dependency
> > 
> >     stop_machine from a multithreaded workqueue is not allowed because
> >     of a circular locking dependency between cpu_down and the workqueue
> >     execution. Use a kernel thread to do the clocksource downgrade.
> 
> I cannot find stop_machine usage there; either it went away or I need to
> like wake up.

timekeeping_notify() which is involved in switching clock source uses stomp
machine.

Thanks,

	tglx

Re: REGRESSION: boot stalls on several old dual core Intel CPUs

[Peter Zijlstra]

On Mon, Sep 03, 2018 at 09:38:15AM +0200, Thomas Gleixner wrote:
> On Mon, 3 Sep 2018, Peter Zijlstra wrote:
> > On Sat, Sep 01, 2018 at 11:51:26AM +0930, Kevin Shanahan wrote:
> > > commit 01548f4d3e8e94caf323a4f664eb347fd34a34ab
> > > Author: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > > Date:   Tue Aug 18 17:09:42 2009 +0200
> > > 
> > >     clocksource: Avoid clocksource watchdog circular locking dependency
> > > 
> > >     stop_machine from a multithreaded workqueue is not allowed because
> > >     of a circular locking dependency between cpu_down and the workqueue
> > >     execution. Use a kernel thread to do the clocksource downgrade.
> > 
> > I cannot find stop_machine usage there; either it went away or I need to
> > like wake up.
> 
> timekeeping_notify() which is involved in switching clock source uses stomp
> machine.

ARGH... OK, lemme see if I can come up with something other than
endlessly spawning that kthread.

A special purpose kthread_worker would make more sense than that.

Re: REGRESSION: boot stalls on several old dual core Intel CPUs

[Peter Zijlstra]

On Mon, Sep 03, 2018 at 10:54:23AM +0200, Peter Zijlstra wrote:
> On Mon, Sep 03, 2018 at 09:38:15AM +0200, Thomas Gleixner wrote:
> > On Mon, 3 Sep 2018, Peter Zijlstra wrote:
> > > On Sat, Sep 01, 2018 at 11:51:26AM +0930, Kevin Shanahan wrote:
> > > > commit 01548f4d3e8e94caf323a4f664eb347fd34a34ab
> > > > Author: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > > > Date:   Tue Aug 18 17:09:42 2009 +0200
> > > > 
> > > >     clocksource: Avoid clocksource watchdog circular locking dependency
> > > > 
> > > >     stop_machine from a multithreaded workqueue is not allowed because
> > > >     of a circular locking dependency between cpu_down and the workqueue
> > > >     execution. Use a kernel thread to do the clocksource downgrade.
> > > 
> > > I cannot find stop_machine usage there; either it went away or I need to
> > > like wake up.
> > 
> > timekeeping_notify() which is involved in switching clock source uses stomp
> > machine.
> 
> ARGH... OK, lemme see if I can come up with something other than
> endlessly spawning that kthread.
> 
> A special purpose kthread_worker would make more sense than that.

Can someone test this?

---
 kernel/time/clocksource.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index f74fb00d8064..898976d0082a 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -112,13 +112,28 @@ static int finished_booting;
 static u64 suspend_start;
 
 #ifdef CONFIG_CLOCKSOURCE_WATCHDOG
-static void clocksource_watchdog_work(struct work_struct *work);
+static void clocksource_watchdog_work(struct kthread_work *work);
 static void clocksource_select(void);
 
 static LIST_HEAD(watchdog_list);
 static struct clocksource *watchdog;
 static struct timer_list watchdog_timer;
-static DECLARE_WORK(watchdog_work, clocksource_watchdog_work);
+
+/*
+ * We must use a kthread_worker here, because:
+ *
+ *   clocksource_watchdog_work()
+ *     clocksource_select()
+ *       __clocksource_select()
+ *         timekeeping_notify()
+ *           stop_machine()
+ *
+ * cannot be called from a reqular workqueue, because of deadlocks between
+ * workqueue and stopmachine.
+ */
+static struct kthread_worker *watchdog_worker;
+static DEFINE_KTHREAD_WORK(watchdog_work, clocksource_watchdog_work);
+
 static DEFINE_SPINLOCK(watchdog_lock);
 static int watchdog_running;
 static atomic_t watchdog_reset_pending;
@@ -158,7 +173,7 @@ static void __clocksource_unstable(struct clocksource *cs)
 
 	/* kick clocksource_watchdog_work() */
 	if (finished_booting)
-		schedule_work(&watchdog_work);
+		kthread_queue_work(watchdog_worker, &watchdog_work);
 }
 
 /**
@@ -199,7 +214,7 @@ static void clocksource_watchdog(struct timer_list *unused)
 		/* Clocksource already marked unstable? */
 		if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
 			if (finished_booting)
-				schedule_work(&watchdog_work);
+				kthread_queue_work(watchdog_worker, &watchdog_work);
 			continue;
 		}
 
@@ -269,7 +284,7 @@ static void clocksource_watchdog(struct timer_list *unused)
 			 */
 			if (cs != curr_clocksource) {
 				cs->flags |= CLOCK_SOURCE_RESELECT;
-				schedule_work(&watchdog_work);
+				kthread_queue_work(watchdog_worker, &watchdog_work);
 			} else {
 				tick_clock_notify();
 			}
@@ -418,7 +433,7 @@ static int __clocksource_watchdog_work(void)
 	return select;
 }
 
-static void clocksource_watchdog_work(struct work_struct *work)
+static void clocksource_watchdog_work(struct kthread_work *work)
 {
 	mutex_lock(&clocksource_mutex);
 	if (__clocksource_watchdog_work())
@@ -806,6 +821,7 @@ static int __init clocksource_done_booting(void)
 {
 	mutex_lock(&clocksource_mutex);
 	curr_clocksource = clocksource_default_clock();
+	watchdog_worker = kthread_create_worker(0, "cs-watchdog");
 	finished_booting = 1;
 	/*
 	 * Run the watchdog first to eliminate unstable clock sources

Re: REGRESSION: boot stalls on several old dual core Intel CPUs

[Viktor Jägersküpper]

Peter Zijlstra:
> On Mon, Sep 03, 2018 at 10:54:23AM +0200, Peter Zijlstra wrote:
>> On Mon, Sep 03, 2018 at 09:38:15AM +0200, Thomas Gleixner wrote:
>>> On Mon, 3 Sep 2018, Peter Zijlstra wrote:
>>>> On Sat, Sep 01, 2018 at 11:51:26AM +0930, Kevin Shanahan wrote:
>>>>> commit 01548f4d3e8e94caf323a4f664eb347fd34a34ab
>>>>> Author: Martin Schwidefsky <schwidefsky@de.ibm.com>
>>>>> Date:   Tue Aug 18 17:09:42 2009 +0200
>>>>>
>>>>>     clocksource: Avoid clocksource watchdog circular locking dependency
>>>>>
>>>>>     stop_machine from a multithreaded workqueue is not allowed because
>>>>>     of a circular locking dependency between cpu_down and the workqueue
>>>>>     execution. Use a kernel thread to do the clocksource downgrade.
>>>>
>>>> I cannot find stop_machine usage there; either it went away or I need to
>>>> like wake up.
>>>
>>> timekeeping_notify() which is involved in switching clock source uses stomp
>>> machine.
>>
>> ARGH... OK, lemme see if I can come up with something other than
>> endlessly spawning that kthread.
>>
>> A special purpose kthread_worker would make more sense than that.
> 
> Can someone test this?
> 
> ---
>  kernel/time/clocksource.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index f74fb00d8064..898976d0082a 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -112,13 +112,28 @@ static int finished_booting;
>  static u64 suspend_start;
>  
>  #ifdef CONFIG_CLOCKSOURCE_WATCHDOG
> -static void clocksource_watchdog_work(struct work_struct *work);
> +static void clocksource_watchdog_work(struct kthread_work *work);
>  static void clocksource_select(void);
>  
>  static LIST_HEAD(watchdog_list);
>  static struct clocksource *watchdog;
>  static struct timer_list watchdog_timer;
> -static DECLARE_WORK(watchdog_work, clocksource_watchdog_work);
> +
> +/*
> + * We must use a kthread_worker here, because:
> + *
> + *   clocksource_watchdog_work()
> + *     clocksource_select()
> + *       __clocksource_select()
> + *         timekeeping_notify()
> + *           stop_machine()
> + *
> + * cannot be called from a reqular workqueue, because of deadlocks between
> + * workqueue and stopmachine.
> + */
> +static struct kthread_worker *watchdog_worker;
> +static DEFINE_KTHREAD_WORK(watchdog_work, clocksource_watchdog_work);
> +
>  static DEFINE_SPINLOCK(watchdog_lock);
>  static int watchdog_running;
>  static atomic_t watchdog_reset_pending;
> @@ -158,7 +173,7 @@ static void __clocksource_unstable(struct clocksource *cs)
>  
>  	/* kick clocksource_watchdog_work() */
>  	if (finished_booting)
> -		schedule_work(&watchdog_work);
> +		kthread_queue_work(watchdog_worker, &watchdog_work);
>  }
>  
>  /**
> @@ -199,7 +214,7 @@ static void clocksource_watchdog(struct timer_list *unused)
>  		/* Clocksource already marked unstable? */
>  		if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
>  			if (finished_booting)
> -				schedule_work(&watchdog_work);
> +				kthread_queue_work(watchdog_worker, &watchdog_work);
>  			continue;
>  		}
>  
> @@ -269,7 +284,7 @@ static void clocksource_watchdog(struct timer_list *unused)
>  			 */
>  			if (cs != curr_clocksource) {
>  				cs->flags |= CLOCK_SOURCE_RESELECT;
> -				schedule_work(&watchdog_work);
> +				kthread_queue_work(watchdog_worker, &watchdog_work);
>  			} else {
>  				tick_clock_notify();
>  			}
> @@ -418,7 +433,7 @@ static int __clocksource_watchdog_work(void)
>  	return select;
>  }
>  
> -static void clocksource_watchdog_work(struct work_struct *work)
> +static void clocksource_watchdog_work(struct kthread_work *work)
>  {
>  	mutex_lock(&clocksource_mutex);
>  	if (__clocksource_watchdog_work())
> @@ -806,6 +821,7 @@ static int __init clocksource_done_booting(void)
>  {
>  	mutex_lock(&clocksource_mutex);
>  	curr_clocksource = clocksource_default_clock();
> +	watchdog_worker = kthread_create_worker(0, "cs-watchdog");
>  	finished_booting = 1;
>  	/*
>  	 * Run the watchdog first to eliminate unstable clock sources
> 

Applied on mainline tag v4.19-rc2. Tested without additional parameters,
with "quiet" and with "debug", my PC booted successfully in all three
cases, whereas it stalled almost always in these three cases before.

Thanks!

Re: REGRESSION: boot stalls on several old dual core Intel CPUs

[Kevin Shanahan]

On Mon, Sep 03, 2018 at 11:33:05AM +0200, Peter Zijlstra wrote:
> On Mon, Sep 03, 2018 at 10:54:23AM +0200, Peter Zijlstra wrote:
> > On Mon, Sep 03, 2018 at 09:38:15AM +0200, Thomas Gleixner wrote:
> > > On Mon, 3 Sep 2018, Peter Zijlstra wrote:
> > > > On Sat, Sep 01, 2018 at 11:51:26AM +0930, Kevin Shanahan wrote:
> > > > > commit 01548f4d3e8e94caf323a4f664eb347fd34a34ab
> > > > > Author: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > > > > Date:   Tue Aug 18 17:09:42 2009 +0200
> > > > > 
> > > > >     clocksource: Avoid clocksource watchdog circular locking dependency
> > > > > 
> > > > >     stop_machine from a multithreaded workqueue is not allowed because
> > > > >     of a circular locking dependency between cpu_down and the workqueue
> > > > >     execution. Use a kernel thread to do the clocksource downgrade.
> > > > 
> > > > I cannot find stop_machine usage there; either it went away or I need to
> > > > like wake up.
> > > 
> > > timekeeping_notify() which is involved in switching clock source uses stomp
> > > machine.
> > 
> > ARGH... OK, lemme see if I can come up with something other than
> > endlessly spawning that kthread.
> > 
> > A special purpose kthread_worker would make more sense than that.
> 
> Can someone test this?

Boots for me (applied on top of 4.18.5).

Tested-by: Kevin Shanahan <kevin@shanahan.id.au>

> ---
>  kernel/time/clocksource.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index f74fb00d8064..898976d0082a 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -112,13 +112,28 @@ static int finished_booting;
>  static u64 suspend_start;
>  
>  #ifdef CONFIG_CLOCKSOURCE_WATCHDOG
> -static void clocksource_watchdog_work(struct work_struct *work);
> +static void clocksource_watchdog_work(struct kthread_work *work);
>  static void clocksource_select(void);
>  
>  static LIST_HEAD(watchdog_list);
>  static struct clocksource *watchdog;
>  static struct timer_list watchdog_timer;
> -static DECLARE_WORK(watchdog_work, clocksource_watchdog_work);
> +
> +/*
> + * We must use a kthread_worker here, because:
> + *
> + *   clocksource_watchdog_work()
> + *     clocksource_select()
> + *       __clocksource_select()
> + *         timekeeping_notify()
> + *           stop_machine()
> + *
> + * cannot be called from a reqular workqueue, because of deadlocks between
> + * workqueue and stopmachine.
> + */
> +static struct kthread_worker *watchdog_worker;
> +static DEFINE_KTHREAD_WORK(watchdog_work, clocksource_watchdog_work);
> +
>  static DEFINE_SPINLOCK(watchdog_lock);
>  static int watchdog_running;
>  static atomic_t watchdog_reset_pending;
> @@ -158,7 +173,7 @@ static void __clocksource_unstable(struct clocksource *cs)
>  
>  	/* kick clocksource_watchdog_work() */
>  	if (finished_booting)
> -		schedule_work(&watchdog_work);
> +		kthread_queue_work(watchdog_worker, &watchdog_work);
>  }
>  
>  /**
> @@ -199,7 +214,7 @@ static void clocksource_watchdog(struct timer_list *unused)
>  		/* Clocksource already marked unstable? */
>  		if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
>  			if (finished_booting)
> -				schedule_work(&watchdog_work);
> +				kthread_queue_work(watchdog_worker, &watchdog_work);
>  			continue;
>  		}
>  
> @@ -269,7 +284,7 @@ static void clocksource_watchdog(struct timer_list *unused)
>  			 */
>  			if (cs != curr_clocksource) {
>  				cs->flags |= CLOCK_SOURCE_RESELECT;
> -				schedule_work(&watchdog_work);
> +				kthread_queue_work(watchdog_worker, &watchdog_work);
>  			} else {
>  				tick_clock_notify();
>  			}
> @@ -418,7 +433,7 @@ static int __clocksource_watchdog_work(void)
>  	return select;
>  }
>  
> -static void clocksource_watchdog_work(struct work_struct *work)
> +static void clocksource_watchdog_work(struct kthread_work *work)
>  {
>  	mutex_lock(&clocksource_mutex);
>  	if (__clocksource_watchdog_work())
> @@ -806,6 +821,7 @@ static int __init clocksource_done_booting(void)
>  {
>  	mutex_lock(&clocksource_mutex);
>  	curr_clocksource = clocksource_default_clock();
> +	watchdog_worker = kthread_create_worker(0, "cs-watchdog");
>  	finished_booting = 1;
>  	/*
>  	 * Run the watchdog first to eliminate unstable clock sources

Re: REGRESSION: boot stalls on several old dual core Intel CPUs

[Siegfried Metz]

On 9/3/18 11:33 AM, Peter Zijlstra wrote:
> On Mon, Sep 03, 2018 at 10:54:23AM +0200, Peter Zijlstra wrote:
>> On Mon, Sep 03, 2018 at 09:38:15AM +0200, Thomas Gleixner wrote:
>>> On Mon, 3 Sep 2018, Peter Zijlstra wrote:
>>>> On Sat, Sep 01, 2018 at 11:51:26AM +0930, Kevin Shanahan wrote:
>>>>> commit 01548f4d3e8e94caf323a4f664eb347fd34a34ab
>>>>> Author: Martin Schwidefsky <schwidefsky@de.ibm.com>
>>>>> Date:   Tue Aug 18 17:09:42 2009 +0200
>>>>>
>>>>>      clocksource: Avoid clocksource watchdog circular locking dependency
>>>>>
>>>>>      stop_machine from a multithreaded workqueue is not allowed because
>>>>>      of a circular locking dependency between cpu_down and the workqueue
>>>>>      execution. Use a kernel thread to do the clocksource downgrade.
>>>>
>>>> I cannot find stop_machine usage there; either it went away or I need to
>>>> like wake up.
>>>
>>> timekeeping_notify() which is involved in switching clock source uses stomp
>>> machine.
>>
>> ARGH... OK, lemme see if I can come up with something other than
>> endlessly spawning that kthread.
>>
>> A special purpose kthread_worker would make more sense than that.
> 
> Can someone test this?
> 
> ---
>   kernel/time/clocksource.c | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index f74fb00d8064..898976d0082a 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -112,13 +112,28 @@ static int finished_booting;
>   static u64 suspend_start;
>   
>   #ifdef CONFIG_CLOCKSOURCE_WATCHDOG
> -static void clocksource_watchdog_work(struct work_struct *work);
> +static void clocksource_watchdog_work(struct kthread_work *work);
>   static void clocksource_select(void);
>   
>   static LIST_HEAD(watchdog_list);
>   static struct clocksource *watchdog;
>   static struct timer_list watchdog_timer;
> -static DECLARE_WORK(watchdog_work, clocksource_watchdog_work);
> +
> +/*
> + * We must use a kthread_worker here, because:
> + *
> + *   clocksource_watchdog_work()
> + *     clocksource_select()
> + *       __clocksource_select()
> + *         timekeeping_notify()
> + *           stop_machine()
> + *
> + * cannot be called from a reqular workqueue, because of deadlocks between
> + * workqueue and stopmachine.
> + */
> +static struct kthread_worker *watchdog_worker;
> +static DEFINE_KTHREAD_WORK(watchdog_work, clocksource_watchdog_work);
> +
>   static DEFINE_SPINLOCK(watchdog_lock);
>   static int watchdog_running;
>   static atomic_t watchdog_reset_pending;
> @@ -158,7 +173,7 @@ static void __clocksource_unstable(struct clocksource *cs)
>   
>   	/* kick clocksource_watchdog_work() */
>   	if (finished_booting)
> -		schedule_work(&watchdog_work);
> +		kthread_queue_work(watchdog_worker, &watchdog_work);
>   }
>   
>   /**
> @@ -199,7 +214,7 @@ static void clocksource_watchdog(struct timer_list *unused)
>   		/* Clocksource already marked unstable? */
>   		if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
>   			if (finished_booting)
> -				schedule_work(&watchdog_work);
> +				kthread_queue_work(watchdog_worker, &watchdog_work);
>   			continue;
>   		}
>   
> @@ -269,7 +284,7 @@ static void clocksource_watchdog(struct timer_list *unused)
>   			 */
>   			if (cs != curr_clocksource) {
>   				cs->flags |= CLOCK_SOURCE_RESELECT;
> -				schedule_work(&watchdog_work);
> +				kthread_queue_work(watchdog_worker, &watchdog_work);
>   			} else {
>   				tick_clock_notify();
>   			}
> @@ -418,7 +433,7 @@ static int __clocksource_watchdog_work(void)
>   	return select;
>   }
>   
> -static void clocksource_watchdog_work(struct work_struct *work)
> +static void clocksource_watchdog_work(struct kthread_work *work)
>   {
>   	mutex_lock(&clocksource_mutex);
>   	if (__clocksource_watchdog_work())
> @@ -806,6 +821,7 @@ static int __init clocksource_done_booting(void)
>   {
>   	mutex_lock(&clocksource_mutex);
>   	curr_clocksource = clocksource_default_clock();
> +	watchdog_worker = kthread_create_worker(0, "cs-watchdog");
>   	finished_booting = 1;
>   	/*
>   	 * Run the watchdog first to eliminate unstable clock sources
> 

Successfully booted my Intel Core 2 Duo with the patch applied on top of
4.18.5 (based on default Arch Linux config).

I tested with at least 8/8 successful boots in total - with no
additional kernel boot parameters and also with "quiet", and "debug".
No problems seen so far.

Thank you for your effort and developing this patch.


Siegfried

Re: REGRESSION: boot stalls on several old dual core Intel CPUs

[Niklas Cassel]

On Mon, Sep 03, 2018 at 11:33:05AM +0200, Peter Zijlstra wrote:
> On Mon, Sep 03, 2018 at 10:54:23AM +0200, Peter Zijlstra wrote:
> > On Mon, Sep 03, 2018 at 09:38:15AM +0200, Thomas Gleixner wrote:
> > > On Mon, 3 Sep 2018, Peter Zijlstra wrote:
> > > > On Sat, Sep 01, 2018 at 11:51:26AM +0930, Kevin Shanahan wrote:
> > > > > commit 01548f4d3e8e94caf323a4f664eb347fd34a34ab
> > > > > Author: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > > > > Date:   Tue Aug 18 17:09:42 2009 +0200
> > > > > 
> > > > >     clocksource: Avoid clocksource watchdog circular locking dependency
> > > > > 
> > > > >     stop_machine from a multithreaded workqueue is not allowed because
> > > > >     of a circular locking dependency between cpu_down and the workqueue
> > > > >     execution. Use a kernel thread to do the clocksource downgrade.
> > > > 
> > > > I cannot find stop_machine usage there; either it went away or I need to
> > > > like wake up.
> > > 
> > > timekeeping_notify() which is involved in switching clock source uses stomp
> > > machine.
> > 
> > ARGH... OK, lemme see if I can come up with something other than
> > endlessly spawning that kthread.
> > 
> > A special purpose kthread_worker would make more sense than that.
> 
> Can someone test this?
> 
> ---
>  kernel/time/clocksource.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index f74fb00d8064..898976d0082a 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -112,13 +112,28 @@ static int finished_booting;
>  static u64 suspend_start;
>  
>  #ifdef CONFIG_CLOCKSOURCE_WATCHDOG
> -static void clocksource_watchdog_work(struct work_struct *work);
> +static void clocksource_watchdog_work(struct kthread_work *work);
>  static void clocksource_select(void);
>  
>  static LIST_HEAD(watchdog_list);
>  static struct clocksource *watchdog;
>  static struct timer_list watchdog_timer;
> -static DECLARE_WORK(watchdog_work, clocksource_watchdog_work);
> +
> +/*
> + * We must use a kthread_worker here, because:
> + *
> + *   clocksource_watchdog_work()
> + *     clocksource_select()
> + *       __clocksource_select()
> + *         timekeeping_notify()
> + *           stop_machine()
> + *
> + * cannot be called from a reqular workqueue, because of deadlocks between
> + * workqueue and stopmachine.
> + */
> +static struct kthread_worker *watchdog_worker;
> +static DEFINE_KTHREAD_WORK(watchdog_work, clocksource_watchdog_work);
> +
>  static DEFINE_SPINLOCK(watchdog_lock);
>  static int watchdog_running;
>  static atomic_t watchdog_reset_pending;
> @@ -158,7 +173,7 @@ static void __clocksource_unstable(struct clocksource *cs)
>  
>  	/* kick clocksource_watchdog_work() */
>  	if (finished_booting)
> -		schedule_work(&watchdog_work);
> +		kthread_queue_work(watchdog_worker, &watchdog_work);
>  }
>  
>  /**
> @@ -199,7 +214,7 @@ static void clocksource_watchdog(struct timer_list *unused)
>  		/* Clocksource already marked unstable? */
>  		if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
>  			if (finished_booting)
> -				schedule_work(&watchdog_work);
> +				kthread_queue_work(watchdog_worker, &watchdog_work);
>  			continue;
>  		}
>  
> @@ -269,7 +284,7 @@ static void clocksource_watchdog(struct timer_list *unused)
>  			 */
>  			if (cs != curr_clocksource) {
>  				cs->flags |= CLOCK_SOURCE_RESELECT;
> -				schedule_work(&watchdog_work);
> +				kthread_queue_work(watchdog_worker, &watchdog_work);
>  			} else {
>  				tick_clock_notify();
>  			}
> @@ -418,7 +433,7 @@ static int __clocksource_watchdog_work(void)
>  	return select;
>  }
>  
> -static void clocksource_watchdog_work(struct work_struct *work)
> +static void clocksource_watchdog_work(struct kthread_work *work)
>  {
>  	mutex_lock(&clocksource_mutex);
>  	if (__clocksource_watchdog_work())
> @@ -806,6 +821,7 @@ static int __init clocksource_done_booting(void)
>  {
>  	mutex_lock(&clocksource_mutex);
>  	curr_clocksource = clocksource_default_clock();
> +	watchdog_worker = kthread_create_worker(0, "cs-watchdog");

Hello Peter,

watchdog_worker is only defined if CONFIG_CLOCKSOURCE_WATCHDOG is set,
so you might want to wrap it with an ifdef to avoid build errors.

Kind regards,
Niklas

>  	finished_booting = 1;
>  	/*
>  	 * Run the watchdog first to eliminate unstable clock sources

[PATCH] clocksource: Revert "Remove kthread"

[Peter Zijlstra]

On Tue, Sep 04, 2018 at 03:44:50PM +0200, Niklas Cassel wrote:
> watchdog_worker is only defined if CONFIG_CLOCKSOURCE_WATCHDOG is set,
> so you might want to wrap it with an ifdef to avoid build errors.

Yes, so I wrote a version that only spawned the kthread_worker when a
MUST_VERIFY clocksource was registered -- which turned out to be a lot
harder than it sounds, because clocksource registration happens _waaay_
before kthreadd gets spawned.

But after I finished that I realized that this work only ever runs once
or twice during the lifetime of the kernel. So spawning a permanent
thread is quite pointless.

So lets just revert the patch and add a wee comment there explaining why
we spawn the kthread.

---
Subject: clocksource: Revert "Remove kthread"

It turns out that the silly spawn kthread from worker was actually
needed, revert the patch but add a comment that explain why we jump
through such apparently silly hoops.

Fixes: 7197e77abcb6 ("clocksource: Remove kthread")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/time/clocksource.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index f74fb00d8064..0e6e97a01942 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -133,19 +133,40 @@ 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)
+{
+	/*
+	 * We cannot directly run clocksource_watchdog_kthread() here, because
+	 * clocksource_select() calls timekeeping_notify() which uses
+	 * stop_machine(). One cannot use stop_machine() from a workqueue() due
+	 * lock inversions wrt CPU hotplug.
+	 *
+	 * Also, we only ever run this work once or twice during the lifetime
+	 * of the kernel, so there is no point in creating a more permanent
+	 * kthread for this.
+	 *
+	 * 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_work() will
+	 * If the clocksource is registered clocksource_watchdog_kthread() will
 	 * re-rate and re-select.
 	 */
 	if (list_empty(&cs->list)) {
@@ -156,7 +177,7 @@ static void __clocksource_unstable(struct clocksource *cs)
 	if (cs->mark_unstable)
 		cs->mark_unstable(cs);
 
-	/* kick clocksource_watchdog_work() */
+	/* kick clocksource_watchdog_kthread() */
 	if (finished_booting)
 		schedule_work(&watchdog_work);
 }
@@ -166,7 +187,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 work.
+ * it defers demotion and re-selection to a kthread.
  */
 void clocksource_mark_unstable(struct clocksource *cs)
 {
@@ -391,9 +412,7 @@ static void clocksource_dequeue_watchdog(struct clocksource *cs)
 	}
 }
 
-static void __clocksource_change_rating(struct clocksource *cs, int rating);
-
-static int __clocksource_watchdog_work(void)
+static int __clocksource_watchdog_kthread(void)
 {
 	struct clocksource *cs, *tmp;
 	unsigned long flags;
@@ -418,12 +437,13 @@ static int __clocksource_watchdog_work(void)
 	return select;
 }
 
-static void clocksource_watchdog_work(struct work_struct *work)
+static int clocksource_watchdog_kthread(void *data)
 {
 	mutex_lock(&clocksource_mutex);
-	if (__clocksource_watchdog_work())
+	if (__clocksource_watchdog_kthread())
 		clocksource_select();
 	mutex_unlock(&clocksource_mutex);
+	return 0;
 }
 
 static bool clocksource_is_watchdog(struct clocksource *cs)
@@ -442,7 +462,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_work(void) { return 0; }
+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) { }
 
@@ -810,7 +830,7 @@ static int __init clocksource_done_booting(void)
 	/*
 	 * Run the watchdog first to eliminate unstable clock sources
 	 */
-	__clocksource_watchdog_work();
+	__clocksource_watchdog_kthread();
 	clocksource_select();
 	mutex_unlock(&clocksource_mutex);
 	return 0;

[tip:timers/urgent] clocksource: Revert "Remove kthread"

[tip-bot for Peter Zijlstra]

Commit-ID:  760902b24960679c2e8592de3a56359d2c205731
Gitweb:     https://git.kernel.org/tip/760902b24960679c2e8592de3a56359d2c205731
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 5 Sep 2018 10:41:58 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 6 Sep 2018 12:42:28 +0200

clocksource: Revert "Remove kthread"

I turns out that the silly spawn kthread from worker was actually needed.

clocksource_watchdog_kthread() cannot be called directly from
clocksource_watchdog_work(), because clocksource_select() calls
timekeeping_notify() which uses stop_machine(). One cannot use
stop_machine() from a workqueue() due lock inversions wrt CPU hotplug.

Revert the patch but add a comment that explain why we jump through such
apparently silly hoops.

Fixes: 7197e77abcb6 ("clocksource: Remove kthread")
Reported-by: Siegfried Metz <frame@mailbox.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
Tested-by: Kevin Shanahan <kevin@shanahan.id.au>
Tested-by: viktor_jaegerskuepper@freenet.de
Tested-by: Siegfried Metz <frame@mailbox.org>
Cc: rafael.j.wysocki@intel.com
Cc: len.brown@intel.com
Cc: diego.viola@gmail.com
Cc: rui.zhang@intel.com
Cc: bjorn.andersson@linaro.org
Link: https://lkml.kernel.org/r/20180905084158.GR24124@hirez.programming.kicks-ass.net
---
 kernel/time/clocksource.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index f74fb00d8064..0e6e97a01942 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -133,19 +133,40 @@ 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)
+{
+	/*
+	 * We cannot directly run clocksource_watchdog_kthread() here, because
+	 * clocksource_select() calls timekeeping_notify() which uses
+	 * stop_machine(). One cannot use stop_machine() from a workqueue() due
+	 * lock inversions wrt CPU hotplug.
+	 *
+	 * Also, we only ever run this work once or twice during the lifetime
+	 * of the kernel, so there is no point in creating a more permanent
+	 * kthread for this.
+	 *
+	 * 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_work() will
+	 * If the clocksource is registered clocksource_watchdog_kthread() will
 	 * re-rate and re-select.
 	 */
 	if (list_empty(&cs->list)) {
@@ -156,7 +177,7 @@ static void __clocksource_unstable(struct clocksource *cs)
 	if (cs->mark_unstable)
 		cs->mark_unstable(cs);
 
-	/* kick clocksource_watchdog_work() */
+	/* kick clocksource_watchdog_kthread() */
 	if (finished_booting)
 		schedule_work(&watchdog_work);
 }
@@ -166,7 +187,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 work.
+ * it defers demotion and re-selection to a kthread.
  */
 void clocksource_mark_unstable(struct clocksource *cs)
 {
@@ -391,9 +412,7 @@ static void clocksource_dequeue_watchdog(struct clocksource *cs)
 	}
 }
 
-static void __clocksource_change_rating(struct clocksource *cs, int rating);
-
-static int __clocksource_watchdog_work(void)
+static int __clocksource_watchdog_kthread(void)
 {
 	struct clocksource *cs, *tmp;
 	unsigned long flags;
@@ -418,12 +437,13 @@ static int __clocksource_watchdog_work(void)
 	return select;
 }
 
-static void clocksource_watchdog_work(struct work_struct *work)
+static int clocksource_watchdog_kthread(void *data)
 {
 	mutex_lock(&clocksource_mutex);
-	if (__clocksource_watchdog_work())
+	if (__clocksource_watchdog_kthread())
 		clocksource_select();
 	mutex_unlock(&clocksource_mutex);
+	return 0;
 }
 
 static bool clocksource_is_watchdog(struct clocksource *cs)
@@ -442,7 +462,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_work(void) { return 0; }
+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) { }
 
@@ -810,7 +830,7 @@ static int __init clocksource_done_booting(void)
 	/*
 	 * Run the watchdog first to eliminate unstable clock sources
 	 */
-	__clocksource_watchdog_work();
+	__clocksource_watchdog_kthread();
 	clocksource_select();
 	mutex_unlock(&clocksource_mutex);
 	return 0;

[tip:timers/urgent] clocksource: Revert "Remove kthread"

[tip-bot for Peter Zijlstra]

Commit-ID:  e2c631ba75a7e727e8db0a9d30a06bfd434adb3a
Gitweb:     https://git.kernel.org/tip/e2c631ba75a7e727e8db0a9d30a06bfd434adb3a
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 5 Sep 2018 10:41:58 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 6 Sep 2018 23:38:35 +0200

clocksource: Revert "Remove kthread"

I turns out that the silly spawn kthread from worker was actually needed.

clocksource_watchdog_kthread() cannot be called directly from
clocksource_watchdog_work(), because clocksource_select() calls
timekeeping_notify() which uses stop_machine(). One cannot use
stop_machine() from a workqueue() due lock inversions wrt CPU hotplug.

Revert the patch but add a comment that explain why we jump through such
apparently silly hoops.

Fixes: 7197e77abcb6 ("clocksource: Remove kthread")
Reported-by: Siegfried Metz <frame@mailbox.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
Tested-by: Kevin Shanahan <kevin@shanahan.id.au>
Tested-by: viktor_jaegerskuepper@freenet.de
Tested-by: Siegfried Metz <frame@mailbox.org>
Cc: rafael.j.wysocki@intel.com
Cc: len.brown@intel.com
Cc: diego.viola@gmail.com
Cc: rui.zhang@intel.com
Cc: bjorn.andersson@linaro.org
Link: https://lkml.kernel.org/r/20180905084158.GR24124@hirez.programming.kicks-ass.net
---
 kernel/time/clocksource.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index f74fb00d8064..0e6e97a01942 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -133,19 +133,40 @@ 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)
+{
+	/*
+	 * We cannot directly run clocksource_watchdog_kthread() here, because
+	 * clocksource_select() calls timekeeping_notify() which uses
+	 * stop_machine(). One cannot use stop_machine() from a workqueue() due
+	 * lock inversions wrt CPU hotplug.
+	 *
+	 * Also, we only ever run this work once or twice during the lifetime
+	 * of the kernel, so there is no point in creating a more permanent
+	 * kthread for this.
+	 *
+	 * 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_work() will
+	 * If the clocksource is registered clocksource_watchdog_kthread() will
 	 * re-rate and re-select.
 	 */
 	if (list_empty(&cs->list)) {
@@ -156,7 +177,7 @@ static void __clocksource_unstable(struct clocksource *cs)
 	if (cs->mark_unstable)
 		cs->mark_unstable(cs);
 
-	/* kick clocksource_watchdog_work() */
+	/* kick clocksource_watchdog_kthread() */
 	if (finished_booting)
 		schedule_work(&watchdog_work);
 }
@@ -166,7 +187,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 work.
+ * it defers demotion and re-selection to a kthread.
  */
 void clocksource_mark_unstable(struct clocksource *cs)
 {
@@ -391,9 +412,7 @@ static void clocksource_dequeue_watchdog(struct clocksource *cs)
 	}
 }
 
-static void __clocksource_change_rating(struct clocksource *cs, int rating);
-
-static int __clocksource_watchdog_work(void)
+static int __clocksource_watchdog_kthread(void)
 {
 	struct clocksource *cs, *tmp;
 	unsigned long flags;
@@ -418,12 +437,13 @@ static int __clocksource_watchdog_work(void)
 	return select;
 }
 
-static void clocksource_watchdog_work(struct work_struct *work)
+static int clocksource_watchdog_kthread(void *data)
 {
 	mutex_lock(&clocksource_mutex);
-	if (__clocksource_watchdog_work())
+	if (__clocksource_watchdog_kthread())
 		clocksource_select();
 	mutex_unlock(&clocksource_mutex);
+	return 0;
 }
 
 static bool clocksource_is_watchdog(struct clocksource *cs)
@@ -442,7 +462,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_work(void) { return 0; }
+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) { }
 
@@ -810,7 +830,7 @@ static int __init clocksource_done_booting(void)
 	/*
 	 * Run the watchdog first to eliminate unstable clock sources
 	 */
-	__clocksource_watchdog_work();
+	__clocksource_watchdog_kthread();
 	clocksource_select();
 	mutex_unlock(&clocksource_mutex);
 	return 0;