[PATCH] panic_on_oops: remove ssleep()

[PATCH] panic_on_oops: remove ssleep()

[Horms]

This patch is part of an effort to unify the panic_on_oops behaviour
across all architectures that implement it.

It was pointed out to me by Andi Kleen that if an oops has occured
in interrupt context, then calling sleep() in the oops path will only cause
a panic, and that it would be really better for it not to be in the path at
all. 

This patch removes the ssleep() call and reworks the console message
accordinly.  I have a slght concern that the resulting console message is
too long, feedback welcome.

For powerpc it also unifies the 32bit and 64bit behaviour.

Fror x86_64, this patch only updates the console message, as
ssleep() is already not present.

Signed-Off-By: Horms <horms@verge.net.au>

 arch/arm/kernel/traps.c     |    7 ++-----
 arch/i386/kernel/traps.c    |    8 +++-----
 arch/ia64/kernel/traps.c    |    7 ++-----
 arch/powerpc/kernel/traps.c |   10 +++-------
 arch/x86_64/kernel/traps.c  |    2 +-
 arch/xtensa/kernel/traps.c  |    8 +++-----
 6 files changed, 14 insertions(+), 28 deletions(-)

--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -232,11 +232,8 @@ NORET_TYPE void die(const char *str, str
 	bust_spinlocks(0);
 	spin_unlock_irq(&die_lock);
 
-	if (panic_on_oops) {
-		printk(KERN_EMERG "Fatal exception: panic in 5 seconds\n");
-		ssleep(5);
-		panic("Fatal exception");
-	}
+	if (panic_on_oops)
+		panic("Fatal exception: panic_on_oops");
 
 	do_exit(SIGSEGV);
 }
--- a/arch/i386/kernel/traps.c
+++ b/arch/i386/kernel/traps.c
@@ -442,11 +442,9 @@ #endif
 	if (in_interrupt())
 		panic("Fatal exception in interrupt");
 
-	if (panic_on_oops) {
-		printk(KERN_EMERG "Fatal exception: panic in 5 seconds\n");
-		ssleep(5);
-		panic("Fatal exception");
-	}
+	if (panic_on_oops)
+		panic("Fatal exception: panic_on_oops");
+
 	oops_exit();
 	do_exit(SIGSEGV);
 }
--- a/arch/ia64/kernel/traps.c
+++ b/arch/ia64/kernel/traps.c
@@ -117,11 +117,8 @@ die (const char *str, struct pt_regs *re
 	die.lock_owner = -1;
 	spin_unlock_irq(&die.lock);
 
-	if (panic_on_oops) {
-		printk(KERN_EMERG "Fatal exception: panic in 5 seconds\n");
-		ssleep(5);
-		panic("Fatal exception");
-	}
+	if (panic_on_oops)
+		panic("Fatal exception: panic_on_oops");
 
   	do_exit(SIGSEGV);
 }
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -150,13 +150,9 @@ #endif
 	if (in_interrupt())
 		panic("Fatal exception in interrupt");
 
-	if (panic_on_oops) {
-#ifdef CONFIG_PPC64
-		printk(KERN_EMERG "Fatal exception: panic in 5 seconds\n");
-		ssleep(5);
-#endif
-		panic("Fatal exception");
-	}
+	if (panic_on_oops)
+		panic("Fatal exception: panic_on_oops");
+
 	do_exit(err);
 
 	return 0;
--- a/arch/x86_64/kernel/traps.c
+++ b/arch/x86_64/kernel/traps.c
@@ -521,7 +521,7 @@ void __kprobes oops_end(unsigned long fl
 		/* Nest count reaches zero, release the lock. */
 		spin_unlock_irqrestore(&die_lock, flags);
 	if (panic_on_oops)
-		panic("Oops");
+		panic("Fatal exception: panic_on_oops");
 }
 
 void __kprobes __die(const char * str, struct pt_regs * regs, long err)
--- a/arch/xtensa/kernel/traps.c
+++ b/arch/xtensa/kernel/traps.c
@@ -487,11 +487,9 @@ #endif
 	if (in_interrupt())
 		panic("Fatal exception in interrupt");
 
-	if (panic_on_oops) {
-		printk(KERN_EMERG "Fatal exception: panic in 5 seconds\n");
-		ssleep(5);
-		panic("Fatal exception");
-	}
+	if (panic_on_oops)
+		panic("Fatal exception: panic_on_oops");
+
 	do_exit(err);
 }
 

Re: [PATCH] panic_on_oops: remove ssleep()

[Andi Kleen]

On Monday 17 July 2006 18:17, Horms wrote:
> This patch is part of an effort to unify the panic_on_oops behaviour
> across all architectures that implement it.
>
> It was pointed out to me by Andi Kleen that if an oops has occured
> in interrupt context, then calling sleep() in the oops path will only cause
> a panic, and that it would be really better for it not to be in the path at
> all.
>
> This patch removes the ssleep() call and reworks the console message
> accordinly.  I have a slght concern that the resulting console message is
> too long, feedback welcome.

Keeping the delay might be actually useful so that you can see the panic
before system reboots when reboot on panic is enabled. I would just use a loop
of mdelays(1) with touch_nmi_watchdog/touch_softirq_watchdog()s
inbetween.

-Andi

Re: [PATCH] panic_on_oops: remove ssleep()

[Horms]

On Tue, Jul 18, 2006 at 12:27:51AM +0200, Andi Kleen wrote:
> On Monday 17 July 2006 18:17, Horms wrote:
> > This patch is part of an effort to unify the panic_on_oops behaviour
> > across all architectures that implement it.
> >
> > It was pointed out to me by Andi Kleen that if an oops has occured
> > in interrupt context, then calling sleep() in the oops path will only cause
> > a panic, and that it would be really better for it not to be in the path at
> > all.
> >
> > This patch removes the ssleep() call and reworks the console message
> > accordinly.  I have a slght concern that the resulting console message is
> > too long, feedback welcome.
> 
> Keeping the delay might be actually useful so that you can see the panic
> before system reboots when reboot on panic is enabled. I would just use a loop
> of mdelays(1) with touch_nmi_watchdog/touch_softirq_watchdog()s
> inbetween.

Ok, I will look into making that happen. I agree that the pause is
quite useful.

-- 
Horms                                           
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Re: [PATCH] panic_on_oops: remove ssleep()

[Andrew Morton]

On Mon, 17 Jul 2006 19:10:59 -0400
Horms <horms@verge.net.au> wrote:

> On Tue, Jul 18, 2006 at 12:27:51AM +0200, Andi Kleen wrote:
> > On Monday 17 July 2006 18:17, Horms wrote:
> > ...
> > Keeping the delay might be actually useful so that you can see the panic
> > before system reboots when reboot on panic is enabled. I would just use a loop
> > of mdelays(1) with touch_nmi_watchdog/touch_softirq_watchdog()s
> > inbetween.
> 
> Ok, I will look into making that happen. I agree that the pause is
> quite useful.

It's kind-of already implemented, via pause_on_oops.  Perhaps doing
something like 

	if (panic_on_oops)
		pause_on_oops = max(pause_on_oops, 5*HZ);

would be sufficient.

Re: [PATCH] panic_on_oops: remove ssleep()

[Chuck Ebbert]

In-Reply-To: <31687.FP.7244@verge.net.au>

On Mon, 17 Jul 2006 12:17:20 -0400, Horms wrote:

> This patch is part of an effort to unify the panic_on_oops behaviour
> across all architectures that implement it.
> 
> It was pointed out to me by Andi Kleen that if an oops has occured
> in interrupt context, then calling sleep() in the oops path will only cause
> a panic, and that it would be really better for it not to be in the path at
> all. 

i386 already checks in_interrupt() and panics immediately:

--- a/arch/i386/kernel/traps.c
+++ b/arch/i386/kernel/traps.c
@@ -442,11 +442,9 @@ #endif
===>    if (in_interrupt())
===>            panic("Fatal exception in interrupt");
 
-       if (panic_on_oops) {
-               printk(KERN_EMERG "Fatal exception: panic in 5 seconds\n");
-               ssleep(5);
-               panic("Fatal exception");
-       }
+       if (panic_on_oops)
+               panic("Fatal exception: panic_on_oops");
+
        oops_exit();
        do_exit(SIGSEGV);
 }

-- 
Chuck
And did we tell you the name of the game, boy, we call it Riding the Gravy Train.

Re: [PATCH] panic_on_oops: remove ssleep()

[Horms]

On Mon, Jul 17, 2006 at 05:23:41PM -0700, Andrew Morton wrote:
> On Mon, 17 Jul 2006 19:10:59 -0400
> Horms <horms@verge.net.au> wrote:
> 
> > On Tue, Jul 18, 2006 at 12:27:51AM +0200, Andi Kleen wrote:
> > > On Monday 17 July 2006 18:17, Horms wrote:
> > > ...
> > > Keeping the delay might be actually useful so that you can see the panic
> > > before system reboots when reboot on panic is enabled. I would just use a loop
> > > of mdelays(1) with touch_nmi_watchdog/touch_softirq_watchdog()s
> > > inbetween.
> > 
> > Ok, I will look into making that happen. I agree that the pause is
> > quite useful.
> 
> It's kind-of already implemented, via pause_on_oops.  Perhaps doing
> something like 
> 
> 	if (panic_on_oops)
> 		pause_on_oops = max(pause_on_oops, 5*HZ);
> 
> would be sufficient.

Thanks, that may well be sufficient. And I assume that it is nicely out
of the arch-dependant code in die(). I will poke around a bit more.

-- 
Horms                                           
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Re: [PATCH] panic_on_oops: remove ssleep()

[Horms]

On Mon, Jul 17, 2006 at 09:22:17PM -0400, Chuck Ebbert wrote:
> In-Reply-To: <31687.FP.7244@verge.net.au>
> 
> On Mon, 17 Jul 2006 12:17:20 -0400, Horms wrote:
> 
> > This patch is part of an effort to unify the panic_on_oops behaviour
> > across all architectures that implement it.
> > 
> > It was pointed out to me by Andi Kleen that if an oops has occured
> > in interrupt context, then calling sleep() in the oops path will only cause
> > a panic, and that it would be really better for it not to be in the path at
> > all. 
> 
> i386 already checks in_interrupt() and panics immediately:

Very good point. I guess that needs to be moved to after
panic_on_oops() if the change that Andi suggests works out.

-- 
Horms                                           
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Re: [PATCH] panic_on_oops: remove ssleep()

[Paul Mackerras]

Horms writes:

> This patch is part of an effort to unify the panic_on_oops behaviour
> across all architectures that implement it.

Acked-by: Paul Mackerras <paulus@samba.org>