[PATCH] softlockup: stop spurious softlockup messages due to overflow

[PATCH] softlockup: stop spurious softlockup messages due to overflow

[Colin Ian King]

Ensure additions on touch_ts do not overflow.  This can occur when
the top 32 bits of the TSC reach 0xffffffff causing additions to
touch_ts to overflow and this in turn generates spurious softlockup
warnings.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 kernel/softlockup.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/softlockup.c b/kernel/softlockup.c
index 0d4c789..90d9aa0 100644
--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -111,10 +111,10 @@ int proc_dosoftlockup_thresh(struct ctl_table *table, int write,
 void softlockup_tick(void)
 {
 	int this_cpu = smp_processor_id();
-	unsigned long touch_ts = per_cpu(softlockup_touch_ts, this_cpu);
+	unsigned long long touch_ts = per_cpu(softlockup_touch_ts, this_cpu);
 	unsigned long print_ts;
 	struct pt_regs *regs = get_irq_regs();
-	unsigned long now;
+	unsigned long long now;
 
 	/* Is detection switched off? */
 	if (!per_cpu(softlockup_watchdog, this_cpu) || softlockup_thresh <= 0) {
@@ -165,7 +165,7 @@ void softlockup_tick(void)
 	per_cpu(softlockup_print_ts, this_cpu) = touch_ts;
 
 	spin_lock(&print_lock);
-	printk(KERN_ERR "BUG: soft lockup - CPU#%d stuck for %lus! [%s:%d]\n",
+	printk(KERN_ERR "BUG: soft lockup - CPU#%d stuck for %llus! [%s:%d]\n",
 			this_cpu, now - touch_ts,
 			current->comm, task_pid_nr(current));
 	print_modules();
-- 
1.6.3.3

Re: [PATCH] softlockup: stop spurious softlockup messages due to overflow

[Eric Dumazet]

Le lundi 15 mars 2010 à 14:01 +0000, Colin Ian King a écrit :
> Ensure additions on touch_ts do not overflow.  This can occur when
> the top 32 bits of the TSC reach 0xffffffff causing additions to
> touch_ts to overflow and this in turn generates spurious softlockup
> warnings.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  kernel/softlockup.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/softlockup.c b/kernel/softlockup.c
> index 0d4c789..90d9aa0 100644
> --- a/kernel/softlockup.c
> +++ b/kernel/softlockup.c
> @@ -111,10 +111,10 @@ int proc_dosoftlockup_thresh(struct ctl_table *table, int write,
>  void softlockup_tick(void)
>  {
>  	int this_cpu = smp_processor_id();
> -	unsigned long touch_ts = per_cpu(softlockup_touch_ts, this_cpu);
> +	unsigned long long touch_ts = per_cpu(softlockup_touch_ts, this_cpu);
>  	unsigned long print_ts;
>  	struct pt_regs *regs = get_irq_regs();
> -	unsigned long now;
> +	unsigned long long now;
>  
>  	/* Is detection switched off? */
>  	if (!per_cpu(softlockup_watchdog, this_cpu) || softlockup_thresh <= 0) {
> @@ -165,7 +165,7 @@ void softlockup_tick(void)
>  	per_cpu(softlockup_print_ts, this_cpu) = touch_ts;
>  
>  	spin_lock(&print_lock);
> -	printk(KERN_ERR "BUG: soft lockup - CPU#%d stuck for %lus! [%s:%d]\n",
> +	printk(KERN_ERR "BUG: soft lockup - CPU#%d stuck for %llus! [%s:%d]\n",
>  			this_cpu, now - touch_ts,
>  			current->comm, task_pid_nr(current));
>  	print_modules();

This looks wrong, touch_ts is a long, not a long long.

You probably want to change the comparisons instead.

if (now > touch_ts + softlockup_thresh/2)
	wake_up_process(per_cpu(softlockup_watchdog, this_cpu));
if (now <= (touch_ts + softlockup_thresh))
	return;
	
->

if ((long)(now - touch_ts) > softlockup_thresh/2)
	wake_up_process(per_cpu(softlockup_watchdog, this_cpu));
if ((long)(now - touch_ts) <= softlockup_thresh))
	return;

Or use standard time_after()/time_before() macros.

Re: [PATCH] softlockup: stop spurious softlockup messages due to overflow

[Ingo Molnar]

* Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le lundi 15 mars 2010 ?? 14:01 +0000, Colin Ian King a ??crit :
> > Ensure additions on touch_ts do not overflow.  This can occur when
> > the top 32 bits of the TSC reach 0xffffffff causing additions to
> > touch_ts to overflow and this in turn generates spurious softlockup
> > warnings.
> > 
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> >  kernel/softlockup.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/softlockup.c b/kernel/softlockup.c
> > index 0d4c789..90d9aa0 100644
> > --- a/kernel/softlockup.c
> > +++ b/kernel/softlockup.c
> > @@ -111,10 +111,10 @@ int proc_dosoftlockup_thresh(struct ctl_table *table, int write,
> >  void softlockup_tick(void)
> >  {
> >  	int this_cpu = smp_processor_id();
> > -	unsigned long touch_ts = per_cpu(softlockup_touch_ts, this_cpu);
> > +	unsigned long long touch_ts = per_cpu(softlockup_touch_ts, this_cpu);
> >  	unsigned long print_ts;
> >  	struct pt_regs *regs = get_irq_regs();
> > -	unsigned long now;
> > +	unsigned long long now;
> >  
> >  	/* Is detection switched off? */
> >  	if (!per_cpu(softlockup_watchdog, this_cpu) || softlockup_thresh <= 0) {
> > @@ -165,7 +165,7 @@ void softlockup_tick(void)
> >  	per_cpu(softlockup_print_ts, this_cpu) = touch_ts;
> >  
> >  	spin_lock(&print_lock);
> > -	printk(KERN_ERR "BUG: soft lockup - CPU#%d stuck for %lus! [%s:%d]\n",
> > +	printk(KERN_ERR "BUG: soft lockup - CPU#%d stuck for %llus! [%s:%d]\n",
> >  			this_cpu, now - touch_ts,
> >  			current->comm, task_pid_nr(current));
> >  	print_modules();
> 
> This looks wrong, touch_ts is a long, not a long long.

Could be increased to long long - but that's probably overkill as the touch_ts 
is in seconds, so the scope of comparisons should never truly get even close 
to ~2^31.

> You probably want to change the comparisons instead.
> 
> if (now > touch_ts + softlockup_thresh/2)
> 	wake_up_process(per_cpu(softlockup_watchdog, this_cpu));
> if (now <= (touch_ts + softlockup_thresh))
> 	return;
> 	
> ->
> 
> if ((long)(now - touch_ts) > softlockup_thresh/2)
> 	wake_up_process(per_cpu(softlockup_watchdog, this_cpu));
> if ((long)(now - touch_ts) <= softlockup_thresh))
> 	return;
> 
> Or use standard time_after()/time_before() macros.

Yeah, time_after/before would work better i suspect.

Thanks,

	Ingo

Re: [PATCH] softlockup: stop spurious softlockup messages due to overflow

[Colin Ian King]

On Tue, 2010-03-16 at 11:12 +0100, Ingo Molnar wrote:
> * Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > Le lundi 15 mars 2010 ?? 14:01 +0000, Colin Ian King a ??crit :
> > > Ensure additions on touch_ts do not overflow.  This can occur when
> > > the top 32 bits of the TSC reach 0xffffffff causing additions to
> > > touch_ts to overflow and this in turn generates spurious softlockup
> > > warnings.
> > > 
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > >  kernel/softlockup.c |    6 +++---
> > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/kernel/softlockup.c b/kernel/softlockup.c
> > > index 0d4c789..90d9aa0 100644
> > > --- a/kernel/softlockup.c
> > > +++ b/kernel/softlockup.c
> > > @@ -111,10 +111,10 @@ int proc_dosoftlockup_thresh(struct ctl_table *table, int write,
> > >  void softlockup_tick(void)
> > >  {
> > >  	int this_cpu = smp_processor_id();
> > > -	unsigned long touch_ts = per_cpu(softlockup_touch_ts, this_cpu);
> > > +	unsigned long long touch_ts = per_cpu(softlockup_touch_ts, this_cpu);
> > >  	unsigned long print_ts;
> > >  	struct pt_regs *regs = get_irq_regs();
> > > -	unsigned long now;
> > > +	unsigned long long now;
> > >  
> > >  	/* Is detection switched off? */
> > >  	if (!per_cpu(softlockup_watchdog, this_cpu) || softlockup_thresh <= 0) {
> > > @@ -165,7 +165,7 @@ void softlockup_tick(void)
> > >  	per_cpu(softlockup_print_ts, this_cpu) = touch_ts;
> > >  
> > >  	spin_lock(&print_lock);
> > > -	printk(KERN_ERR "BUG: soft lockup - CPU#%d stuck for %lus! [%s:%d]\n",
> > > +	printk(KERN_ERR "BUG: soft lockup - CPU#%d stuck for %llus! [%s:%d]\n",
> > >  			this_cpu, now - touch_ts,
> > >  			current->comm, task_pid_nr(current));
> > >  	print_modules();
> > 
> > This looks wrong, touch_ts is a long, not a long long.
> 
> Could be increased to long long - but that's probably overkill as the touch_ts 
> is in seconds, so the scope of comparisons should never truly get even close 
> to ~2^31.
> 
> > You probably want to change the comparisons instead.
> > 
> > if (now > touch_ts + softlockup_thresh/2)
> > 	wake_up_process(per_cpu(softlockup_watchdog, this_cpu));
> > if (now <= (touch_ts + softlockup_thresh))
> > 	return;
> > 	
> > ->
> > 
> > if ((long)(now - touch_ts) > softlockup_thresh/2)
> > 	wake_up_process(per_cpu(softlockup_watchdog, this_cpu));
> > if ((long)(now - touch_ts) <= softlockup_thresh))
> > 	return;
> > 
> > Or use standard time_after()/time_before() macros.
> 
> Yeah, time_after/before would work better i suspect.
> 
> Thanks,
> 
> 	Ingo

Using time_after/before:

diff --git a/kernel/softlockup.c b/kernel/softlockup.c
index 0d4c789..4b493f6 100644
--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -155,11 +155,11 @@ void softlockup_tick(void)
         * Wake up the high-prio watchdog task twice per
         * threshold timespan.
         */
-       if (now > touch_ts + softlockup_thresh/2)
+       if (time_after(now - softlockup_thresh/2, touch_ts))
                wake_up_process(per_cpu(softlockup_watchdog, this_cpu));
 
        /* Warn about unreasonable delays: */
-       if (now <= (touch_ts + softlockup_thresh))
+       if (time_before_eq(now - softlockup_thresh, touch_ts))
                return;
 
        per_cpu(softlockup_print_ts, this_cpu) = touch_ts;

Re: [PATCH] softlockup: stop spurious softlockup messages due to overflow

[Ingo Molnar]

* Colin Ian King <colin.king@canonical.com> wrote:

> On Tue, 2010-03-16 at 11:12 +0100, Ingo Molnar wrote:
> > * Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > 
> > > Le lundi 15 mars 2010 ?? 14:01 +0000, Colin Ian King a ??crit :
> > > > Ensure additions on touch_ts do not overflow.  This can occur when
> > > > the top 32 bits of the TSC reach 0xffffffff causing additions to
> > > > touch_ts to overflow and this in turn generates spurious softlockup
> > > > warnings.
> > > > 
> > > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > > ---
> > > >  kernel/softlockup.c |    6 +++---
> > > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/kernel/softlockup.c b/kernel/softlockup.c
> > > > index 0d4c789..90d9aa0 100644
> > > > --- a/kernel/softlockup.c
> > > > +++ b/kernel/softlockup.c
> > > > @@ -111,10 +111,10 @@ int proc_dosoftlockup_thresh(struct ctl_table *table, int write,
> > > >  void softlockup_tick(void)
> > > >  {
> > > >  	int this_cpu = smp_processor_id();
> > > > -	unsigned long touch_ts = per_cpu(softlockup_touch_ts, this_cpu);
> > > > +	unsigned long long touch_ts = per_cpu(softlockup_touch_ts, this_cpu);
> > > >  	unsigned long print_ts;
> > > >  	struct pt_regs *regs = get_irq_regs();
> > > > -	unsigned long now;
> > > > +	unsigned long long now;
> > > >  
> > > >  	/* Is detection switched off? */
> > > >  	if (!per_cpu(softlockup_watchdog, this_cpu) || softlockup_thresh <= 0) {
> > > > @@ -165,7 +165,7 @@ void softlockup_tick(void)
> > > >  	per_cpu(softlockup_print_ts, this_cpu) = touch_ts;
> > > >  
> > > >  	spin_lock(&print_lock);
> > > > -	printk(KERN_ERR "BUG: soft lockup - CPU#%d stuck for %lus! [%s:%d]\n",
> > > > +	printk(KERN_ERR "BUG: soft lockup - CPU#%d stuck for %llus! [%s:%d]\n",
> > > >  			this_cpu, now - touch_ts,
> > > >  			current->comm, task_pid_nr(current));
> > > >  	print_modules();
> > > 
> > > This looks wrong, touch_ts is a long, not a long long.
> > 
> > Could be increased to long long - but that's probably overkill as the touch_ts 
> > is in seconds, so the scope of comparisons should never truly get even close 
> > to ~2^31.
> > 
> > > You probably want to change the comparisons instead.
> > > 
> > > if (now > touch_ts + softlockup_thresh/2)
> > > 	wake_up_process(per_cpu(softlockup_watchdog, this_cpu));
> > > if (now <= (touch_ts + softlockup_thresh))
> > > 	return;
> > > 	
> > > ->
> > > 
> > > if ((long)(now - touch_ts) > softlockup_thresh/2)
> > > 	wake_up_process(per_cpu(softlockup_watchdog, this_cpu));
> > > if ((long)(now - touch_ts) <= softlockup_thresh))
> > > 	return;
> > > 
> > > Or use standard time_after()/time_before() macros.
> > 
> > Yeah, time_after/before would work better i suspect.
> > 
> > Thanks,
> > 
> > 	Ingo
> 
> Using time_after/before:
> 
> diff --git a/kernel/softlockup.c b/kernel/softlockup.c
> index 0d4c789..4b493f6 100644
> --- a/kernel/softlockup.c
> +++ b/kernel/softlockup.c
> @@ -155,11 +155,11 @@ void softlockup_tick(void)
>          * Wake up the high-prio watchdog task twice per
>          * threshold timespan.
>          */
> -       if (now > touch_ts + softlockup_thresh/2)
> +       if (time_after(now - softlockup_thresh/2, touch_ts))
>                 wake_up_process(per_cpu(softlockup_watchdog, this_cpu));
>  
>         /* Warn about unreasonable delays: */
> -       if (now <= (touch_ts + softlockup_thresh))
> +       if (time_before_eq(now - softlockup_thresh, touch_ts))
>                 return;

Ok, that looks like the most readable variant, agreed?

	Ingo

Re: [PATCH] softlockup: stop spurious softlockup messages due to overflow

[Eric Dumazet]

Le jeudi 18 mars 2010 à 16:25 +0100, Ingo Molnar a écrit :
> * Colin Ian King <colin.king@canonical.com> wrote:
> 
> > 
> > Using time_after/before:
> > 
> > diff --git a/kernel/softlockup.c b/kernel/softlockup.c
> > index 0d4c789..4b493f6 100644
> > --- a/kernel/softlockup.c
> > +++ b/kernel/softlockup.c
> > @@ -155,11 +155,11 @@ void softlockup_tick(void)
> >          * Wake up the high-prio watchdog task twice per
> >          * threshold timespan.
> >          */
> > -       if (now > touch_ts + softlockup_thresh/2)
> > +       if (time_after(now - softlockup_thresh/2, touch_ts))
> >                 wake_up_process(per_cpu(softlockup_watchdog, this_cpu));
> >  
> >         /* Warn about unreasonable delays: */
> > -       if (now <= (touch_ts + softlockup_thresh))
> > +       if (time_before_eq(now - softlockup_thresh, touch_ts))
> >                 return;
> 
> Ok, that looks like the most readable variant, agreed?
> 

Sure ! Colin please submit formally your patch :)

Thanks

Re: [PATCH] softlockup: stop spurious softlockup messages due to overflow

[Colin Ian King]

[-- Attachment #1: Type: text/plain, Size: 39 bytes --]

Re-submitted, thanks for you feedback.

[-- Attachment #2: 0001-PATCH-softlockup-stop-spurious-softlockup-messages-d.patch --]
[-- Type: text/x-patch, Size: 1266 bytes --]

>From d640e08bfba1abdd1a82f0f11cbde62f29b3f423 Mon Sep 17 00:00:00 2001
From: Colin Ian King <colin.king@canonical.com>
Date: Fri, 19 Mar 2010 10:17:03 +0000
Subject: [PATCH] softlockup: stop spurious softlockup messages due to overflow

Ensure additions on touch_ts do not overflow.  This can occur when
the top 32 bits of the TSC reach 0xffffffff causing additions to
touch_ts to overflow and this in turn generates spurious softlockup
warnings.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Cc: stable@kernel.org
---
 kernel/softlockup.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/softlockup.c b/kernel/softlockup.c
index 0d4c789..4b493f6 100644
--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -155,11 +155,11 @@ void softlockup_tick(void)
 	 * Wake up the high-prio watchdog task twice per
 	 * threshold timespan.
 	 */
-	if (now > touch_ts + softlockup_thresh/2)
+	if (time_after(now - softlockup_thresh/2, touch_ts))
 		wake_up_process(per_cpu(softlockup_watchdog, this_cpu));
 
 	/* Warn about unreasonable delays: */
-	if (now <= (touch_ts + softlockup_thresh))
+	if (time_before_eq(now - softlockup_thresh, touch_ts))
 		return;
 
 	per_cpu(softlockup_print_ts, this_cpu) = touch_ts;
-- 
1.6.3.3

[tip:core/urgent] softlockup: Stop spurious softlockup messages due to overflow

[tip-bot for Colin Ian King]

Commit-ID:  8c2eb4805d422bdbf60ba00ff233c794d23c3c00
Gitweb:     http://git.kernel.org/tip/8c2eb4805d422bdbf60ba00ff233c794d23c3c00
Author:     Colin Ian King <colin.king@canonical.com>
AuthorDate: Fri, 19 Mar 2010 10:28:02 +0000
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 21 Mar 2010 19:30:13 +0100

softlockup: Stop spurious softlockup messages due to overflow

Ensure additions on touch_ts do not overflow.  This can occur
when the top 32 bits of the TSC reach 0xffffffff causing
additions to touch_ts to overflow and this in turn generates
spurious softlockup warnings.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: <stable@kernel.org>
LKML-Reference: <1268994482.1798.6.camel@lenovo>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/softlockup.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/softlockup.c b/kernel/softlockup.c
index 0d4c789..4b493f6 100644
--- a/kernel/softlockup.c
+++ b/kernel/softlockup.c
@@ -155,11 +155,11 @@ void softlockup_tick(void)
 	 * Wake up the high-prio watchdog task twice per
 	 * threshold timespan.
 	 */
-	if (now > touch_ts + softlockup_thresh/2)
+	if (time_after(now - softlockup_thresh/2, touch_ts))
 		wake_up_process(per_cpu(softlockup_watchdog, this_cpu));
 
 	/* Warn about unreasonable delays: */
-	if (now <= (touch_ts + softlockup_thresh))
+	if (time_before_eq(now - softlockup_thresh, touch_ts))
 		return;
 
 	per_cpu(softlockup_print_ts, this_cpu) = touch_ts;