softlockup: do the wakeup from a hrtimer

Subject: softlockup: do the wakeup from a hrtimer
From: Ingo Molnar <[email protected]>

David Miller reported soft lockup false-positives that trigger on NOHZ due to CPUs idling for more than 10 seconds.

The solution is to drive the wakeup of the watchdog threads not from the timer tick (which has no guaranteed frequency), but from the watchdog tasks themselves.

http://bugzilla.kernel.org/show_bug.cgi?id=9409

Reported-by David Miller <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/softlockup.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Index: linux/kernel/softlockup.c
===================================================================

kernel/softlockup.c +0 -4

@@ -100,10 +100,6 @@ void softlockup_tick(void)
 
 	now = get_timestamp(this_cpu);
 
-	/* Wake up the high-prio watchdog task every second: */
-	if (now > (touch_timestamp + 1))
-		wake_up_process(per_cpu(watchdog_task, this_cpu));
-
 	/* Warn about unreasonable 10+ seconds delays: */
 	if (now <= (touch_timestamp + softlockup_thresh))
 		return;

Quoted 41 lines · On Tue, 20 Nov 2007 09:46:11 +0100 Ingo Molnar <[email protected]> wrote:

Subject: softlockup: do the wakeup from a hrtimer
From: Ingo Molnar <[email protected]>

David Miller reported soft lockup false-positives that trigger on NOHZ due to CPUs idling for more than 10 seconds.

The solution is to drive the wakeup of the watchdog threads not from the timer tick (which has no guaranteed frequency), but from the watchdog tasks themselves.

http://bugzilla.kernel.org/show_bug.cgi?id=9409

Reported-by David Miller <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/softlockup.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Index: linux/kernel/softlockup.c
===================================================================

kernel/softlockup.c +1 -5

@@ -100,10 +100,6 @@ void softlockup_tick(void)
 
 	now = get_timestamp(this_cpu);
 
-	/* Wake up the high-prio watchdog task every second: */
-	if (now > (touch_timestamp + 1))
-		wake_up_process(per_cpu(watchdog_task, this_cpu));
-
 	/* Warn about unreasonable 10+ seconds delays: */
 	if (now <= (touch_timestamp + softlockup_thresh))
 		return;

@@ -141,7 +137,7 @@ static int watchdog(void *__bind_cpu)
 	while (!kthread_should_stop()) {
 		set_current_state(TASK_INTERRUPTIBLE);
 		touch_softlockup_watchdog();
-		schedule();
+		msleep(1000);
 	}
 

kernel/softlockup.c static int watchdog(void *__bind_cpu) · line 143

Quoted 1 line

kernel/softlockup.c +0 -0

 	return 0;

I think you wanted msleep_interruptible() there to avoid contributing to load average?

The set_current_state() can go away.

This will introduce an up-to-one-second delay in responding to kthread_should_stop(). Is that bad?

these can be fixed, but:

Quoted 2 lines

This will introduce an up-to-one-second delay in responding to kthread_should_stop(). Is that bad?

grumble, it's bad. I guess David is right that this should be fixed the right way ;-) So the above patch cannot go in.