[PATCH 0/3] softlockup fixlets

[PATCH 0/3] softlockup fixlets

[Johannes Weiner]

Hi,

here are three small fixlets for the softlockup watchdog that fell out
while studying the code.

	Hannes

 arch/x86/kernel/nmi_32.c |    2 +-
 arch/x86/kernel/nmi_64.c |    2 +-
 include/linux/nmi.h      |    2 +-
 include/linux/sched.h    |    4 ++++
 kernel/softlockup.c      |   25 +++++++++++++------------
 5 files changed, 20 insertions(+), 15 deletions(-)

[PATCH 1/3] softlockup: only reset timestamp from NMI code

[Johannes Weiner]

[-- Attachment #1: softlockup-only-reset-from-nmi-code.patch --]
[-- Type: text/plain, Size: 4229 bytes --]

Since 9c106c119e "softlockup: fix NMI hangs due to lock race -
2.6.26-rc regression", touching the softlockup watchdog from the
outside world means resetting the timestamp to zero instead of
updating it to the current time.

However, the following code sequence is an example of where this
causes the watchdog task not getting awakened for quite some time:

tick_nohz_handler()
  touch_softlockup_watchdog()	/* reset timestamp */
  update_process_times()
    softlockup_tick()		/* update timestamp */

The same path is in tick_sched_timer().

The idea is to just not warn about an expected delay here, but it
prevents the watchdog task from being woken, too, and no check for
hung tasks will be performed.

This patch introduces reset_softlockup_watchdog() for the NMI users
and reverts touch_softlockup_watchdog() to its old behaviour.

Signed-off-by: Johannes Weiner <hannes@saeurebad.de>
---
 arch/x86/kernel/nmi_32.c |    2 +-
 arch/x86/kernel/nmi_64.c |    2 +-
 include/linux/nmi.h      |    2 +-
 include/linux/sched.h    |    4 ++++
 kernel/softlockup.c      |   15 ++++++++-------
 5 files changed, 15 insertions(+), 10 deletions(-)

--- a/arch/x86/kernel/nmi_32.c
+++ b/arch/x86/kernel/nmi_32.c
@@ -310,7 +310,7 @@ void touch_nmi_watchdog(void)
 	/*
 	 * Tickle the softlockup detector too:
 	 */
-	touch_softlockup_watchdog();
+	reset_softlockup_watchdog();
 }
 EXPORT_SYMBOL(touch_nmi_watchdog);
 
--- a/arch/x86/kernel/nmi_64.c
+++ b/arch/x86/kernel/nmi_64.c
@@ -309,7 +309,7 @@ void touch_nmi_watchdog(void)
 		}
 	}
 
-	touch_softlockup_watchdog();
+	reset_softlockup_watchdog();
 }
 EXPORT_SYMBOL(touch_nmi_watchdog);
 
--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -22,7 +22,7 @@ extern void acpi_nmi_enable(void);
 #else
 static inline void touch_nmi_watchdog(void)
 {
-	touch_softlockup_watchdog();
+	reset_softlockup_watchdog();
 }
 static inline void acpi_nmi_disable(void) { }
 static inline void acpi_nmi_enable(void) { }
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -293,6 +293,7 @@ extern void sched_show_task(struct task_
 extern void softlockup_tick(void);
 extern void spawn_softlockup_task(void);
 extern void touch_softlockup_watchdog(void);
+extern void reset_softlockup_watchdog(void);
 extern void touch_all_softlockup_watchdogs(void);
 extern unsigned long  softlockup_thresh;
 extern unsigned long sysctl_hung_task_check_count;
@@ -308,6 +309,9 @@ static inline void spawn_softlockup_task
 static inline void touch_softlockup_watchdog(void)
 {
 }
+static inline void reset_softlockup_watchdog(void)
+{
+}
 static inline void touch_all_softlockup_watchdogs(void)
 {
 }
--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -49,18 +49,19 @@ static unsigned long get_timestamp(int t
 	return cpu_clock(this_cpu) >> 30LL;  /* 2^30 ~= 10^9 */
 }
 
-static void __touch_softlockup_watchdog(void)
+void touch_softlockup_watchdog(void)
 {
 	int this_cpu = raw_smp_processor_id();
 
 	__raw_get_cpu_var(touch_timestamp) = get_timestamp(this_cpu);
 }
+EXPORT_SYMBOL(touch_softlockup_watchdog);
 
-void touch_softlockup_watchdog(void)
+void reset_softlockup_watchdog(void)
 {
 	__raw_get_cpu_var(touch_timestamp) = 0;
 }
-EXPORT_SYMBOL(touch_softlockup_watchdog);
+EXPORT_SYMBOL(reset_softlockup_watchdog);
 
 void touch_all_softlockup_watchdogs(void)
 {
@@ -85,7 +86,7 @@ void softlockup_tick(void)
 	unsigned long now;
 
 	if (touch_timestamp == 0) {
-		__touch_softlockup_watchdog();
+		touch_softlockup_watchdog();
 		return;
 	}
 
@@ -100,7 +101,7 @@ void softlockup_tick(void)
 
 	/* do not print during early bootup: */
 	if (unlikely(system_state != SYSTEM_RUNNING)) {
-		__touch_softlockup_watchdog();
+		touch_softlockup_watchdog();
 		return;
 	}
 
@@ -219,7 +220,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);
 	/*
@@ -228,7 +229,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())

-- 

[PATCH 2/3] softlockup: sanitize print-out limit checks

[Johannes Weiner]

[-- Attachment #1: softlockup-bogus-report-delay-checks.patch --]
[-- Type: text/plain, Size: 1063 bytes --]

The print_timestamp can never be bigger than the touch_timestamp, at
maximum it can be equal.  And if it is, the second check for
touch_timestamp + 1 bigger print_timestamp is always true, too.

The check for equality is sufficient as we proceed in one-second-steps
and are at least one second away from the last print-out if we have
another timestamp.

Signed-off-by: Johannes Weiner <hannes@saeurebad.de>
---
 kernel/softlockup.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -93,11 +93,11 @@ void softlockup_tick(void)
 	print_timestamp = per_cpu(print_timestamp, this_cpu);
 
 	/* report at most once a second */
-	if ((print_timestamp >= touch_timestamp &&
-			print_timestamp < (touch_timestamp + 1)) ||
-			did_panic || !per_cpu(watchdog_task, this_cpu)) {
+	if (print_timestamp == touch_timestamp)
+		return;
+
+	if (did_panic || !per_cpu(watchdog_task, this_cpu))
 		return;
-	}
 
 	/* do not print during early bootup: */
 	if (unlikely(system_state != SYSTEM_RUNNING)) {

-- 

[PATCH 3/3] softlockup: fix watchdog task wakeup frequency

[Johannes Weiner]

[-- Attachment #1: softlockup-fix-watchdog-wakeup.patch --]
[-- Type: text/plain, Size: 653 bytes --]

Wake up the watchdog every second instead of every second.  If the
current timestamp is bigger than the last touch, we are already (at
least) a second ahead.

Signed-off-by: Johannes Weiner <hannes@saeurebad.de>
---
 kernel/softlockup.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -108,7 +108,7 @@ void softlockup_tick(void)
 	now = get_timestamp(this_cpu);
 
 	/* Wake up the high-prio watchdog task every second: */
-	if (now > (touch_timestamp + 1))
+	if (now > touch_timestamp)
 		wake_up_process(per_cpu(watchdog_task, this_cpu));
 
 	/* Warn about unreasonable delays: */

-- 

Re: [PATCH 3/3] softlockup: fix watchdog task wakeup frequency

[Ingo Molnar]

* Johannes Weiner <hannes@saeurebad.de> wrote:

> Wake up the watchdog every second instead of every second. [...]

now _that_ is a real improvement ;-)

>  	/* Wake up the high-prio watchdog task every second: */
> -	if (now > (touch_timestamp + 1))
> +	if (now > touch_timestamp)
>  		wake_up_process(per_cpu(watchdog_task, this_cpu));

you mean wake up every second instead of every two seconds?

i think the best sleep period for the watchdog is half of the threshold. 
It makes no point to check sooner than that. I.e. softlockup_thresh/2 
would be better?

	Ingo

Re: [PATCH 3/3] softlockup: fix watchdog task wakeup frequency

[Johannes Weiner]

Ingo Molnar <mingo@elte.hu> writes:

> * Johannes Weiner <hannes@saeurebad.de> wrote:
>
>> Wake up the watchdog every second instead of every second. [...]
>
> now _that_ is a real improvement ;-)
>
>>  	/* Wake up the high-prio watchdog task every second: */
>> -	if (now > (touch_timestamp + 1))
>> +	if (now > touch_timestamp)
>>  		wake_up_process(per_cpu(watchdog_task, this_cpu));
>
> you mean wake up every second instead of every two seconds?

Every second second :D The second `second' [adjective] refers to the
first `second' [noun].

> i think the best sleep period for the watchdog is half of the threshold. 
> It makes no point to check sooner than that. I.e. softlockup_thresh/2 
> would be better?

Hm, it updates the timestamp, so it makes only sense if it runs at a
maximum every second (timestamp granularity) or even less.  The check
for hung tasks uses the cpu timestamp as well for comparison, so that
would be okay too.

Like this?

diff --git a/kernel/softlockup.c b/kernel/softlockup.c
index c828c23..b884546 100644
--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -106,8 +106,9 @@ 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 the high-prio watchdog task twice per
+	 * threshold timespan. */
+	if (now > (touch_timestamp + softlockup_thresh / 2))
 		wake_up_process(per_cpu(watchdog_task, this_cpu));
 
 	/* Warn about unreasonable delays: */

Re: [PATCH 3/3] softlockup: fix watchdog task wakeup frequency

[Ingo Molnar]

* Johannes Weiner <hannes@saeurebad.de> wrote:

> Hm, it updates the timestamp, so it makes only sense if it runs at a 
> maximum every second (timestamp granularity) or even less.  The check 
> for hung tasks uses the cpu timestamp as well for comparison, so that 
> would be okay too.
> 
> Like this?
> 
> diff --git a/kernel/softlockup.c b/kernel/softlockup.c
> index c828c23..b884546 100644
> --- a/kernel/softlockup.c
> +++ b/kernel/softlockup.c
> @@ -106,8 +106,9 @@ 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 the high-prio watchdog task twice per
> +	 * threshold timespan. */
> +	if (now > (touch_timestamp + softlockup_thresh / 2))
>  		wake_up_process(per_cpu(watchdog_task, this_cpu));

yeah - but please use the best possible coding style. Two-line comments 
should be in the:

  /*
   * Here we ......................
   * ........................ come:
   */

... format. And the arithmetics should be:

	if (now > touch_timestamp + softlockup_thresh/2)

(the unnecessary paranthesis was a small style mistake in the original 
too)

	Ingo

Re: [PATCH 3/3] softlockup: fix watchdog task wakeup frequency

[Johannes Weiner]

Hi,

Ingo Molnar <mingo@elte.hu> writes:

> * Johannes Weiner <hannes@saeurebad.de> wrote:
>
>> Hm, it updates the timestamp, so it makes only sense if it runs at a 
>> maximum every second (timestamp granularity) or even less.  The check 
>> for hung tasks uses the cpu timestamp as well for comparison, so that 
>> would be okay too.
>> 
>> Like this?
>> 
>> diff --git a/kernel/softlockup.c b/kernel/softlockup.c
>> index c828c23..b884546 100644
>> --- a/kernel/softlockup.c
>> +++ b/kernel/softlockup.c
>> @@ -106,8 +106,9 @@ 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 the high-prio watchdog task twice per
>> +	 * threshold timespan. */
>> +	if (now > (touch_timestamp + softlockup_thresh / 2))
>>  		wake_up_process(per_cpu(watchdog_task, this_cpu));
>
> yeah - but please use the best possible coding style. Two-line comments 
> should be in the:
>
>   /*
>    * Here we ......................
>    * ........................ come:
>    */
>
> ... format.

Alright, that looks much better.


> And the arithmetics should be:
>
> 	if (now > touch_timestamp + softlockup_thresh/2)
>
> (the unnecessary paranthesis was a small style mistake in the original 
> too)

I tried to fit it into the rest of the code but also prefer the one
without parens.

Thanks for the suggestions, update appended.

	Hannes

---
From: Johannes Weiner <hannes@saeurebad.de>
Subject: [PATCH 3/3] softlockup: wake up watchdog twice per threshold timespan

Updating the timestamp more often is pointless as we print the warnings
only if we exceed the threshold.  And the check for hung tasks relies on
the last timestamp, so it will keep working correctly, too.

Signed-off-by: Johannes Weiner <hannes@saeurebad.de>
---
 kernel/softlockup.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -107,8 +107,11 @@ 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 the high-prio watchdog task twice per
+	 * threshold timespan.
+	 */
+	if (now > touch_timestamp + softlockup_thresh/2)
 		wake_up_process(per_cpu(watchdog_task, this_cpu));
 
 	/* Warn about unreasonable delays: */

Re: [PATCH 3/3] softlockup: fix watchdog task wakeup frequency

[Johannes Weiner]

Hi,

Johannes Weiner <hannes@saeurebad.de> writes:

> From: Johannes Weiner <hannes@saeurebad.de>
> Subject: [PATCH 3/3] softlockup: wake up watchdog twice per threshold timespan
>
> Updating the timestamp more often is pointless as we print the warnings
> only if we exceed the threshold.  And the check for hung tasks relies on
> the last timestamp, so it will keep working correctly, too.
>
> Signed-off-by: Johannes Weiner <hannes@saeurebad.de>
> ---
>  kernel/softlockup.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> --- a/kernel/softlockup.c
> +++ b/kernel/softlockup.c
> @@ -107,8 +107,11 @@ 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 the high-prio watchdog task twice per
> +	 * threshold timespan.
> +	 */
> +	if (now > touch_timestamp + softlockup_thresh/2)
>  		wake_up_process(per_cpu(watchdog_task, this_cpu));

That defeats patch 1/3 and I think it can be dropped (#1).

	Hannes

Re: [PATCH 3/3] softlockup: fix watchdog task wakeup frequency

[Ingo Molnar]

* Johannes Weiner <hannes@saeurebad.de> wrote:

> > +	/*
> > +	 * Wake up the high-prio watchdog task twice per
> > +	 * threshold timespan.
> > +	 */
> > +	if (now > touch_timestamp + softlockup_thresh/2)
> >  		wake_up_process(per_cpu(watchdog_task, this_cpu));
> 
> That defeats patch 1/3 and I think it can be dropped (#1).

applied this updated patch to tip/core/softlockup. #3 didnt apply - 
could you send a delta patch against tip/core/softlockup please? You can 
pick it up via:

  http://people.redhat.com/mingo/tip.git/README

do:

  git-checkout -b core/softlockup tip/core/softlockup

to check it out.

	Ingo

Re: [PATCH 3/3] softlockup: fix watchdog task wakeup frequency

[Johannes Weiner]

Hi,

Ingo Molnar <mingo@elte.hu> writes:

> * Johannes Weiner <hannes@saeurebad.de> wrote:
>
>> > +	/*
>> > +	 * Wake up the high-prio watchdog task twice per
>> > +	 * threshold timespan.
>> > +	 */
>> > +	if (now > touch_timestamp + softlockup_thresh/2)
>> >  		wake_up_process(per_cpu(watchdog_task, this_cpu));
>> 
>> That defeats patch 1/3 and I think it can be dropped (#1).
>
> applied this updated patch to tip/core/softlockup. #3 didnt apply - 
> could you send a delta patch against tip/core/softlockup please? You can 
> pick it up via:
>
>   http://people.redhat.com/mingo/tip.git/README
>
> do:
>
>   git-checkout -b core/softlockup tip/core/softlockup
>
> to check it out.

Uhm, I see this patch is already in.  Misunderstanding or did you fix it
up yourself?

	Hannes

Re: [PATCH 3/3] softlockup: fix watchdog task wakeup frequency

[Ingo Molnar]

* Johannes Weiner <hannes@saeurebad.de> wrote:

> Hi,
> 
> Ingo Molnar <mingo@elte.hu> writes:
> 
> > * Johannes Weiner <hannes@saeurebad.de> wrote:
> >
> >> > +	/*
> >> > +	 * Wake up the high-prio watchdog task twice per
> >> > +	 * threshold timespan.
> >> > +	 */
> >> > +	if (now > touch_timestamp + softlockup_thresh/2)
> >> >  		wake_up_process(per_cpu(watchdog_task, this_cpu));
> >> 
> >> That defeats patch 1/3 and I think it can be dropped (#1).
> >
> > applied this updated patch to tip/core/softlockup. #3 didnt apply - 
> > could you send a delta patch against tip/core/softlockup please? You can 
> > pick it up via:
> >
> >   http://people.redhat.com/mingo/tip.git/README
> >
> > do:
> >
> >   git-checkout -b core/softlockup tip/core/softlockup
> >
> > to check it out.
> 
> Uhm, I see this patch is already in.  Misunderstanding or did you fix 
> it up yourself?

i applied and tested everything that would apply (modulo trivial 
conflict resolution) - but not all of your patches applied so if there's 
still anything missing please send a delta patch against this branch. 
(or against tip/master, which too has all these changes included)

If it's "no further patch needed" that's fine - and if anything is 
missing please resend that bit.

	Ingo

Re: [PATCH 3/3] softlockup: fix watchdog task wakeup frequency

[Johannes Weiner]

Hi Ingo,

Ingo Molnar <mingo@elte.hu> writes:

> * Johannes Weiner <hannes@saeurebad.de> wrote:
>
>> Hi,
>> 
>> Ingo Molnar <mingo@elte.hu> writes:
>> 
>> > * Johannes Weiner <hannes@saeurebad.de> wrote:
>> >
>> >> > +	/*
>> >> > +	 * Wake up the high-prio watchdog task twice per
>> >> > +	 * threshold timespan.
>> >> > +	 */
>> >> > +	if (now > touch_timestamp + softlockup_thresh/2)
>> >> >  		wake_up_process(per_cpu(watchdog_task, this_cpu));
>> >> 
>> >> That defeats patch 1/3 and I think it can be dropped (#1).
>> >
>> > applied this updated patch to tip/core/softlockup. #3 didnt apply - 
>> > could you send a delta patch against tip/core/softlockup please? You can 
>> > pick it up via:
>> >
>> >   http://people.redhat.com/mingo/tip.git/README
>> >
>> > do:
>> >
>> >   git-checkout -b core/softlockup tip/core/softlockup
>> >
>> > to check it out.
>> 
>> Uhm, I see this patch is already in.  Misunderstanding or did you fix 
>> it up yourself?
>
> i applied and tested everything that would apply (modulo trivial 
> conflict resolution) - but not all of your patches applied so if there's 
> still anything missing please send a delta patch against this branch. 
> (or against tip/master, which too has all these changes included)

#1 is crap, #3 is in the tree and here is #2 against tip/core/softlockup:

--
From: Johannes Weiner <hannes@saeurebad.de>
Subject: softlockup: sanitize timestamp comparison

The print_timestamp can never be bigger than the touch_timestamp, at
maximum it can be equal.  And if it is, the second check for
touch_timestamp + 1 bigger print_timestamp is always true, too.

The check for equality is sufficient as we proceed in one-second-steps
and are at least one second away from the last print-out if we have
another timestamp.

Signed-off-by: Johannes Weiner <hannes@saeurebad.de>
---
 kernel/softlockup.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/kernel/softlockup.c b/kernel/softlockup.c
index d53ab70..7bd8d1a 100644
--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -116,11 +116,8 @@ void softlockup_tick(void)
 	print_timestamp = per_cpu(print_timestamp, this_cpu);
 
 	/* report at most once a second */
-	if ((print_timestamp >= touch_timestamp &&
-			print_timestamp < (touch_timestamp + 1)) ||
-			did_panic) {
+	if (print_timestamp == touch_timestamp || did_panic)
 		return;
-	}
 
 	/* do not print during early bootup: */
 	if (unlikely(system_state != SYSTEM_RUNNING)) {

Re: [PATCH 3/3] softlockup: fix watchdog task wakeup frequency

[Ingo Molnar]

* Johannes Weiner <hannes@saeurebad.de> wrote:

> > i applied and tested everything that would apply (modulo trivial 
> > conflict resolution) - but not all of your patches applied so if 
> > there's still anything missing please send a delta patch against 
> > this branch. (or against tip/master, which too has all these changes 
> > included)
> 
> #1 is crap, #3 is in the tree and here is #2 against 
> #tip/core/softlockup:

applied to tip/core/softlockup - thanks Johannes.

	Ingo