[PATCH] softlockup: fix NMI hangs due to lock race - 2.6.26-rc regression

[PATCH] softlockup: fix NMI hangs due to lock race - 2.6.26-rc regression

[Jason Wessel]

The touch_nmi_watchdog() routine on x86 ultimately calls
touch_softlockup_watchdog().  The problem is that to touch the
softlockup watchdog, the cpu_clock code has to be called which could
involve multiple cpu locks and can lead to a hard hang if one of the
locks is held by a processor that is not going to return anytime soon
(such as could be the case with kgdb or perhaps even with some other
kind of exception).

This patch causes the public version of the
touch_softlockup_watchdog() to defer the cpu clock access to a later
point.

The test case for this problem is to use the following kernel config
options:

CONFIG_KGDB_TESTS=y
CONFIG_KGDB_TESTS_ON_BOOT=y
CONFIG_KGDB_TESTS_BOOT_STRING="V1F100I100000"

It should be noted that kgdb test suite and these options were not
available until 2.6.26-rc2, so it was necessary to patch the kgdb 
test suite during the bisection.

I would consider this patch a regression fix because the problem first
appeared in commit 27ec4407790d075c325e1f4da0a19c56953cce23 when some
logic was added to try to periodically sync the clocks.  It was
possible to work around this particular problem by simply not
performing the sync anytime the system was in a critical context.
This was ok until commit 3e51f33fcc7f55e6df25d15b55ed10c8b4da84cd,
which added config option CONFIG_HAVE_UNSTABLE_SCHED_CLOCK and some
multi-cpu locks to sync the clocks.  It became clear that accessing
this code from an nmi was the source of the lockups.  Avoiding the
access to the low level clock code from an code inside the NMI
processing also fixed the problem with the 27ec44... commit.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>

---
 kernel/softlockup.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -49,12 +49,17 @@ static unsigned long get_timestamp(int t
 	return cpu_clock(this_cpu) >> 30LL;  /* 2^30 ~= 10^9 */
 }
 
-void touch_softlockup_watchdog(void)
+static void __touch_softlockup_watchdog(void)
 {
 	int this_cpu = raw_smp_processor_id();
 
 	__raw_get_cpu_var(touch_timestamp) = get_timestamp(this_cpu);
 }
+
+void touch_softlockup_watchdog(void)
+{
+	__raw_get_cpu_var(touch_timestamp) = 0;
+}
 EXPORT_SYMBOL(touch_softlockup_watchdog);
 
 void touch_all_softlockup_watchdogs(void)
@@ -80,7 +85,7 @@ void softlockup_tick(void)
 	unsigned long now;
 
 	if (touch_timestamp == 0) {
-		touch_softlockup_watchdog();
+		__touch_softlockup_watchdog();
 		return;
 	}
 
@@ -95,7 +100,7 @@ void softlockup_tick(void)
 
 	/* do not print during early bootup: */
 	if (unlikely(system_state != SYSTEM_RUNNING)) {
-		touch_softlockup_watchdog();
+		__touch_softlockup_watchdog();
 		return;
 	}
 
@@ -214,7 +219,7 @@ static int watchdog(void *__bind_cpu)
 	sched_setscheduler(current, SCHED_FIFO, &param);
 
 	/* initialize timestamp */
-	touch_softlockup_watchdog();
+	__touch_softlockup_watchdog();
 
 	set_current_state(TASK_INTERRUPTIBLE);
 	/*
@@ -223,7 +228,7 @@ static int watchdog(void *__bind_cpu)
 	 * debug-printout triggers in softlockup_tick().
 	 */
 	while (!kthread_should_stop()) {
-		touch_softlockup_watchdog();
+		__touch_softlockup_watchdog();
 		schedule();
 
 		if (kthread_should_stop())

Re: [PATCH] softlockup: fix NMI hangs due to lock race - 2.6.26-rc regression

[Peter Zijlstra]

On Tue, 2008-05-27 at 12:23 -0500, Jason Wessel wrote:
> The touch_nmi_watchdog() routine on x86 ultimately calls
> touch_softlockup_watchdog().  The problem is that to touch the
> softlockup watchdog, the cpu_clock code has to be called which could
> involve multiple cpu locks and can lead to a hard hang if one of the
> locks is held by a processor that is not going to return anytime soon
> (such as could be the case with kgdb or perhaps even with some other
> kind of exception).
> 
> This patch causes the public version of the
> touch_softlockup_watchdog() to defer the cpu clock access to a later
> point.
> 
> The test case for this problem is to use the following kernel config
> options:
> 
> CONFIG_KGDB_TESTS=y
> CONFIG_KGDB_TESTS_ON_BOOT=y
> CONFIG_KGDB_TESTS_BOOT_STRING="V1F100I100000"
> 
> It should be noted that kgdb test suite and these options were not
> available until 2.6.26-rc2, so it was necessary to patch the kgdb 
> test suite during the bisection.
> 
> I would consider this patch a regression fix because the problem first
> appeared in commit 27ec4407790d075c325e1f4da0a19c56953cce23 when some
> logic was added to try to periodically sync the clocks.  It was
> possible to work around this particular problem by simply not
> performing the sync anytime the system was in a critical context.
> This was ok until commit 3e51f33fcc7f55e6df25d15b55ed10c8b4da84cd,
> which added config option CONFIG_HAVE_UNSTABLE_SCHED_CLOCK and some
> multi-cpu locks to sync the clocks.  It became clear that accessing
> this code from an nmi was the source of the lockups.  Avoiding the
> access to the low level clock code from an code inside the NMI
> processing also fixed the problem with the 27ec44... commit.


While I do not object to this approach, I ran into something similar
while poking at .25-rt.

How about we make sched_clock_cpu() use trylocks to update the ->clock
value, and on failure just return the ->clock without updating it?

> Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
> 
> ---
>  kernel/softlockup.c |   15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> --- a/kernel/softlockup.c
> +++ b/kernel/softlockup.c
> @@ -49,12 +49,17 @@ static unsigned long get_timestamp(int t
>  	return cpu_clock(this_cpu) >> 30LL;  /* 2^30 ~= 10^9 */
>  }
>  
> -void touch_softlockup_watchdog(void)
> +static void __touch_softlockup_watchdog(void)
>  {
>  	int this_cpu = raw_smp_processor_id();
>  
>  	__raw_get_cpu_var(touch_timestamp) = get_timestamp(this_cpu);
>  }
> +
> +void touch_softlockup_watchdog(void)
> +{
> +	__raw_get_cpu_var(touch_timestamp) = 0;
> +}
>  EXPORT_SYMBOL(touch_softlockup_watchdog);
>  
>  void touch_all_softlockup_watchdogs(void)
> @@ -80,7 +85,7 @@ void softlockup_tick(void)
>  	unsigned long now;
>  
>  	if (touch_timestamp == 0) {
> -		touch_softlockup_watchdog();
> +		__touch_softlockup_watchdog();
>  		return;
>  	}
>  
> @@ -95,7 +100,7 @@ void softlockup_tick(void)
>  
>  	/* do not print during early bootup: */
>  	if (unlikely(system_state != SYSTEM_RUNNING)) {
> -		touch_softlockup_watchdog();
> +		__touch_softlockup_watchdog();
>  		return;
>  	}
>  
> @@ -214,7 +219,7 @@ static int watchdog(void *__bind_cpu)
>  	sched_setscheduler(current, SCHED_FIFO, &param);
>  
>  	/* initialize timestamp */
> -	touch_softlockup_watchdog();
> +	__touch_softlockup_watchdog();
>  
>  	set_current_state(TASK_INTERRUPTIBLE);
>  	/*
> @@ -223,7 +228,7 @@ static int watchdog(void *__bind_cpu)
>  	 * debug-printout triggers in softlockup_tick().
>  	 */
>  	while (!kthread_should_stop()) {
> -		touch_softlockup_watchdog();
> +		__touch_softlockup_watchdog();
>  		schedule();
>  
>  		if (kthread_should_stop())

Re: [PATCH] softlockup: fix NMI hangs due to lock race - 2.6.26-rc regression

[Jason Wessel]

Peter Zijlstra wrote:
> On Tue, 2008-05-27 at 12:23 -0500, Jason Wessel wrote:
>> The touch_nmi_watchdog() routine on x86 ultimately calls
>> touch_softlockup_watchdog().  The problem is that to touch the
>> softlockup watchdog, the cpu_clock code has to be called which could
>> involve multiple cpu locks and can lead to a hard hang if one of the
>> locks is held by a processor that is not going to return anytime soon
>> (such as could be the case with kgdb or perhaps even with some other
>> kind of exception).
>>
>> This patch causes the public version of the
>> touch_softlockup_watchdog() to defer the cpu clock access to a later
>> point.
>>
>> The test case for this problem is to use the following kernel config
>> options:
>>
>> CONFIG_KGDB_TESTS=y
>> CONFIG_KGDB_TESTS_ON_BOOT=y
>> CONFIG_KGDB_TESTS_BOOT_STRING="V1F100I100000"
>>
>> It should be noted that kgdb test suite and these options were not
>> available until 2.6.26-rc2, so it was necessary to patch the kgdb
>> test suite during the bisection.
>>
>> I would consider this patch a regression fix because the problem first
>> appeared in commit 27ec4407790d075c325e1f4da0a19c56953cce23 when some
>> logic was added to try to periodically sync the clocks.  It was
>> possible to work around this particular problem by simply not
>> performing the sync anytime the system was in a critical context.
>> This was ok until commit 3e51f33fcc7f55e6df25d15b55ed10c8b4da84cd,
>> which added config option CONFIG_HAVE_UNSTABLE_SCHED_CLOCK and some
>> multi-cpu locks to sync the clocks.  It became clear that accessing
>> this code from an nmi was the source of the lockups.  Avoiding the
>> access to the low level clock code from an code inside the NMI
>> processing also fixed the problem with the 27ec44... commit.
>
>
> While I do not object to this approach, I ran into something similar
> while poking at .25-rt.
>
> How about we make sched_clock_cpu() use trylocks to update the ->clock
> value, and on failure just return the ->clock without updating it?
>


If the try locks are used it will certainly solve the NMI problem, but
it seems that the probability that lock could not be obtained would
increase for the "normal" operation case.  In the "normal" operation
case the lock would generally be available in a few more cpu ticks and
the update would proceed correctly.  It is not clear to me if there is
some ramification of returning the ->clock without the update in this
case, which is why I opted for deferring the update in the NMI case.

Perhaps if you return the clock without updating the unstable clock
source is more unstable?  It would be best to figure out how to
address regression so as to observe the intent of seemly more complex
clock logic.

Jason.

Re: [PATCH] softlockup: fix NMI hangs due to lock race - 2.6.26-rc regression

[Ingo Molnar]

* Jason Wessel <jason.wessel@windriver.com> wrote:

> The touch_nmi_watchdog() routine on x86 ultimately calls 
> touch_softlockup_watchdog().  The problem is that to touch the 
> softlockup watchdog, the cpu_clock code has to be called which could 
> involve multiple cpu locks and can lead to a hard hang if one of the 
> locks is held by a processor that is not going to return anytime soon 
> (such as could be the case with kgdb or perhaps even with some other 
> kind of exception).
> 
> This patch causes the public version of the 
> touch_softlockup_watchdog() to defer the cpu clock access to a later 
> point.

applied to tip/core/softlockup and cherry-picked it into tip/core/urgent 
as well for v2.6.26 merge. Thanks,

	Ingo