[PATCH] Don't sleep in oops_begin()

[PATCH] Don't sleep in oops_begin()

[Andi Kleen]

When the kernel is oopsing no realtime guarantees are needed anymore and sleeping
here is unsafe; e.g. in case the crash site had interrupts disabled.

- Convert die_lock to a raw spinlock
- Really disable interrupts

Signed-off-by: Andi Kleen <ak@suse.de>

Index: linux-2.6.23-rc4-rt1/arch/x86_64/kernel/traps.c
===================================================================
--- linux-2.6.23-rc4-rt1.orig/arch/x86_64/kernel/traps.c
+++ linux-2.6.23-rc4-rt1/arch/x86_64/kernel/traps.c
@@ -467,7 +467,7 @@ void out_of_line_bug(void)
 EXPORT_SYMBOL(out_of_line_bug);
 #endif
 
-static DEFINE_SPINLOCK(die_lock);
+static __raw_spinlock_t die_lock = __RAW_SPIN_LOCK_UNLOCKED;
 static int die_owner = -1;
 static unsigned int die_nest_count;
 
@@ -479,13 +479,13 @@ unsigned __kprobes long oops_begin(void)
 	oops_enter();
 
 	/* racy, but better than risking deadlock. */
-	local_irq_save(flags);
+	raw_local_irq_save(flags);
 	cpu = smp_processor_id();
-	if (!spin_trylock(&die_lock)) { 
+	if (!__raw_spin_trylock(&die_lock)) { 
 		if (cpu == die_owner) 
 			/* nested oops. should stop eventually */;
 		else
-			spin_lock(&die_lock);
+			__raw_spin_lock(&die_lock);
 	}
 	die_nest_count++;
 	die_owner = cpu;
@@ -499,12 +499,10 @@ void __kprobes oops_end(unsigned long fl
 	die_owner = -1;
 	bust_spinlocks(0);
 	die_nest_count--;
-	if (die_nest_count)
-		/* We still own the lock */
-		local_irq_restore(flags);
-	else
+	if (!die_nest_count)
 		/* Nest count reaches zero, release the lock. */
-		spin_unlock_irqrestore(&die_lock, flags);
+		__raw_spin_unlock(&die_lock);
+	raw_local_irq_restore(flags);
 	if (panic_on_oops)
 		panic("Fatal exception");
 	oops_exit();

Re: [PATCH] Don't sleep in oops_begin()

[Daniel Walker]

On Mon, 2007-09-17 at 20:12 +0200, Andi Kleen wrote:
> When the kernel is oopsing no realtime guarantees are needed anymore and sleeping
> here is unsafe; e.g. in case the crash site had interrupts disabled.
> 
> - Convert die_lock to a raw spinlock
> - Really disable interrupts
> 
> Signed-off-by: Andi Kleen <ak@suse.de>
> 
> Index: linux-2.6.23-rc4-rt1/arch/x86_64/kernel/traps.c
> ===================================================================
> --- linux-2.6.23-rc4-rt1.orig/arch/x86_64/kernel/traps.c
> +++ linux-2.6.23-rc4-rt1/arch/x86_64/kernel/traps.c
> @@ -467,7 +467,7 @@ void out_of_line_bug(void)
>  EXPORT_SYMBOL(out_of_line_bug);
>  #endif
>  
> -static DEFINE_SPINLOCK(die_lock);
> +static __raw_spinlock_t die_lock = __RAW_SPIN_LOCK_UNLOCKED;

You mean DEFINE_RAW_SPINLOCK() maybe? Unless I'm not following what your
doing here..

>  static int die_owner = -1;
>  static unsigned int die_nest_count;
>  
> @@ -479,13 +479,13 @@ unsigned __kprobes long oops_begin(void)
>  	oops_enter();
>  
>  	/* racy, but better than risking deadlock. */
> -	local_irq_save(flags);
> +	raw_local_irq_save(flags);

local_irq_save() should disable interrupts .. The difference between the
two is one does interrupt off trace accounting 

>  	cpu = smp_processor_id();
> -	if (!spin_trylock(&die_lock)) { 
> +	if (!__raw_spin_trylock(&die_lock)) {

If you use DEFINE_RAW_SPINLOCK() above you shouldn't need to mod these
individually .. A call to spin_trylock() will automatically change
depending on the lock type.

Daniel

Re: [PATCH] Don't sleep in oops_begin()

[Andi Kleen]

> > -static DEFINE_SPINLOCK(die_lock);
> > +static __raw_spinlock_t die_lock = __RAW_SPIN_LOCK_UNLOCKED;
> 
> You mean DEFINE_RAW_SPINLOCK() maybe? Unless I'm not following what your
> doing here..

I just copied that from tsc_sync.c.  If it's correct there it's presumably
correct here too.

-Andi

Re: [PATCH] Don't sleep in oops_begin()

[Daniel Walker]

On Mon, 2007-09-17 at 20:46 +0200, Andi Kleen wrote:
> > > -static DEFINE_SPINLOCK(die_lock);
> > > +static __raw_spinlock_t die_lock = __RAW_SPIN_LOCK_UNLOCKED;
> > 
> > You mean DEFINE_RAW_SPINLOCK() maybe? Unless I'm not following what your
> > doing here..
> 
> I just copied that from tsc_sync.c.  If it's correct there it's presumably
> correct here too.
> 
> -Andi

/*
 * We use a raw spinlock in this exceptional case, because
 * we want to have the fastest, inlined, non-debug version
 * of a critical section, to be able to prove TSC time-warps:
 */
static __cpuinitdata __raw_spinlock_t sync_lock = __RAW_SPIN_LOCK_UNLOCKED;


Since it's all got "__" in the front, not good to use this method all
over .. If you just need a real spinlock best to use
DEFINE_RAW_SPINLOCK() unless your a special situation ..

Daniel

Re: [PATCH] Don't sleep in oops_begin()

[Andi Kleen]

> Since it's all got "__" in the front, not good to use this method all
> over .. If you just need a real spinlock best to use
> DEFINE_RAW_SPINLOCK() unless your a special situation ..

Oopsing is a special situation. Nobody knows if all the fancy infrastructure
lurking inside the other macros still works.

-Andi

Re: [PATCH] Don't sleep in oops_begin()

[Daniel Walker]

On Mon, 2007-09-17 at 21:00 +0200, Andi Kleen wrote:
> > Since it's all got "__" in the front, not good to use this method all
> > over .. If you just need a real spinlock best to use
> > DEFINE_RAW_SPINLOCK() unless your a special situation ..
> 
> Oopsing is a special situation. Nobody knows if all the fancy infrastructure
> lurking inside the other macros still works.

In the case of spinlocks, real time just differs from the mainline
kernel by make a spinlock_t into a mutex .. We know that's not going to
work in this situation. The rest of the debugging added to spinlocks is
mostly un-changed from mainline (like lockdep is still there)..

So you should be able to use a regular mainline style spinlock_t for the
die_lock, even with all the debugging ..

The real time spinlock macros are pretty complex , but it's mostly
compile related complexity that disappears when you run the kernel.

Daniel

Re: [PATCH] Don't sleep in oops_begin()

[Sven-Thorsten Dietrich]

On Mon, 2007-09-17 at 12:07 -0700, Daniel Walker wrote:
> On Mon, 2007-09-17 at 21:00 +0200, Andi Kleen wrote:
> > > Since it's all got "__" in the front, not good to use this method all
> > > over .. If you just need a real spinlock best to use
> > > DEFINE_RAW_SPINLOCK() unless your a special situation ..

die_lock has been RAW for a while, see older patches...

but DEFINE_RAW or DECLARE_RAW should be used when possible.

Sven

> > 
> > Oopsing is a special situation. Nobody knows if all the fancy infrastructure
> > lurking inside the other macros still works.
> 
> In the case of spinlocks, real time just differs from the mainline
> kernel by make a spinlock_t into a mutex .. We know that's not going to
> work in this situation. The rest of the debugging added to spinlocks is
> mostly un-changed from mainline (like lockdep is still there)..
> 
> So you should be able to use a regular mainline style spinlock_t for the
> die_lock, even with all the debugging ..\

I'll check in a minute, but pretty s

> 
> The real time spinlock macros are pretty complex , but it's mostly
> compile related complexity that disappears when you run the kernel.
> 
> Daniel
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Don't sleep in oops_begin()

[Andi Kleen]

On Monday 17 September 2007 21:38, Sven-Thorsten Dietrich wrote:
> On Mon, 2007-09-17 at 12:07 -0700, Daniel Walker wrote:
> > On Mon, 2007-09-17 at 21:00 +0200, Andi Kleen wrote:
> > > > Since it's all got "__" in the front, not good to use this method all
> > > > over .. If you just need a real spinlock best to use
> > > > DEFINE_RAW_SPINLOCK() unless your a special situation ..
>
> die_lock has been RAW for a while, see older patches...

It's definitely not in 2.6.23-rc4-rt1 and also wasn't in the oops
trace I looked at.

> but DEFINE_RAW or DECLARE_RAW should be used when possible.

I disagree for the oops case.  You want the simplest possible code
here.

-Andi

Re: [PATCH] Don't sleep in oops_begin()

[Sven-Thorsten Dietrich]

On Mon, 2007-09-17 at 22:20 +0200, Andi Kleen wrote:
> On Monday 17 September 2007 21:38, Sven-Thorsten Dietrich wrote:
> > On Mon, 2007-09-17 at 12:07 -0700, Daniel Walker wrote:
> > > On Mon, 2007-09-17 at 21:00 +0200, Andi Kleen wrote:
> > > > > Since it's all got "__" in the front, not good to use this method all
> > > > > over .. If you just need a real spinlock best to use
> > > > > DEFINE_RAW_SPINLOCK() unless your a special situation ..
> >
> > die_lock has been RAW for a while, see older patches...
> 
> It's definitely not in 2.6.23-rc4-rt1 and also wasn't in the oops
> trace I looked at.
> 
> > but DEFINE_RAW or DECLARE_RAW should be used when possible.
> 
> I disagree for the oops case.  You want the simplest possible code
> here.

Looks like its raw for all but the x* archs.

# grep die_lock *
preempt-realtime-arm.patch:-DEFINE_SPINLOCK(die_lock);
preempt-realtime-arm.patch:+DEFINE_RAW_SPINLOCK(die_lock);
preempt-realtime-mips.patch:-static DEFINE_SPINLOCK(die_lock);
preempt-realtime-mips.patch:+static DEFINE_RAW_SPINLOCK(die_lock);
preempt-realtime-powerpc.patch:-DEFINE_SPINLOCK(die_lock);
preempt-realtime-powerpc.patch:+DEFINE_RAW_SPINLOCK(die_lock);
preempt-realtime-sh.patch:-static DEFINE_SPINLOCK(die_lock);
preempt-realtime-sh.patch:+static DEFINE_RAW_SPINLOCK(die_lock);

Sven

> 
> -Andi
> 

Re: [PATCH] Don't sleep in oops_begin()

[Daniel Walker]

On Mon, 2007-09-17 at 22:20 +0200, Andi Kleen wrote:
> On Monday 17 September 2007 21:38, Sven-Thorsten Dietrich wrote:
> > On Mon, 2007-09-17 at 12:07 -0700, Daniel Walker wrote:
> > > On Mon, 2007-09-17 at 21:00 +0200, Andi Kleen wrote:
> > > > > Since it's all got "__" in the front, not good to use this method all
> > > > > over .. If you just need a real spinlock best to use
> > > > > DEFINE_RAW_SPINLOCK() unless your a special situation ..
> >
> > die_lock has been RAW for a while, see older patches...
> 
> It's definitely not in 2.6.23-rc4-rt1 and also wasn't in the oops
> trace I looked at.
> 
> > but DEFINE_RAW or DECLARE_RAW should be used when possible.
> 
> I disagree for the oops case.  You want the simplest possible code
> here.

If that's true for real time , should be true for mainline right?

Daniel

Re: [PATCH] Don't sleep in oops_begin()

[Andi Kleen]

> > I disagree for the oops case.  You want the simplest possible code
> > here.
>
> If that's true for real time , should be true for mainline right?

You mean it shouldn't use lockdep etc. there? Yes good point. I can change 
that

-Andi

Re: [PATCH] Don't sleep in oops_begin()

[Thomas Gleixner]

On Tue, 2007-09-18 at 10:05 +0200, Andi Kleen wrote:
> > > I disagree for the oops case.  You want the simplest possible code
> > > here.
> >
> > If that's true for real time , should be true for mainline right?
> 
> You mean it shouldn't use lockdep etc. there? Yes good point. I can change 
> that

Yes, the oops code path should be as decoupled as possible. We want to
know what happened in the first place and not what consequential damage
is caused by the initial problem.

	tglx

Re: [PATCH] Don't sleep in oops_begin()

[Gregory Haskins]

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

On Mon, 2007-09-17 at 22:20 +0200, Andi Kleen wrote:

> I disagree for the oops case.  You want the simplest possible code
> here.

I would have to agree with Andi.  Being conservative here is probably a
good thing to avoid nasties like oops recursion.  No sense in polishing
the brass....;)  We already know the ship is sinking due to an earlier
error (which is the more important error anyway).  I would go so far as
to say the other arches should probably follow suit as well.

Or am I missing something?

Regards,
-Greg


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]