Don't sleep in oops_begin()

PATCH In review Andi Kleen 5 participants 13 messages Sep 17, 2007 – Sep 18, 2007

Share & tools

Andi Kleen

author

2007-09-17 18:12 UTC #0 lore raw

PATCH 1/1 Don't sleep in oops_begin()

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 <[email protected]>

arch/x86_64/kernel/traps.c +7 -9

Index: 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();

Daniel Walker

2007-09-17 18:30 UTC #1 lore raw

Quoted 13 lines · 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 <[email protected]>
Index: linux-2.6.23-rc4-rt1/arch/x86_64/kernel/traps.c
===================================================================

arch/x86_64/kernel/traps.c void out_of_line_bug(void) · line 470

Quoted 5 lines

arch/x86_64/kernel/traps.c +1 -1

@@ -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..

arch/x86_64/kernel/traps.c unsigned __kprobes long oops_begin(void) · line 482

Quoted 9 lines

arch/x86_64/kernel/traps.c +1 -1

 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

arch/x86_64/kernel/traps.c unsigned __kprobes long oops_begin(void) · line 484

Quoted 3 lines

arch/x86_64/kernel/traps.c +1 -1

 	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

Andi Kleen

author

2007-09-17 18:46 UTC #2 lore raw

Quoted 6 lines

arch/x86_64/kernel/traps.c void out_of_line_bug(void) · line 470
Quoted 3 lines
arch/x86_64/kernel/traps.c +1 -1
-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

Daniel Walker

2007-09-17 18:56 UTC #3 lore raw

Quoted 10 lines · Andi Kleen

Quoted 5 lines
arch/x86_64/kernel/traps.c void out_of_line_bug(void) · line 470
Quoted 2 lines
arch/x86_64/kernel/traps.c +1 -1
-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

Andi Kleen

author

2007-09-17 19:00 UTC #4 lore raw

Quoted 4 lines

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

Daniel Walker

2007-09-17 19:09 UTC #5 lore raw

Quoted 6 lines · Andi Kleen

Quoted 3 lines
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

Sven-Thorsten Dietrich

2007-09-17 19:38 UTC #6 lore raw

Quoted 4 lines · Daniel Walker

Quoted 3 lines · Andi Kleen
Quoted 3 lines
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

Quoted 11 lines

Quoted 3 lines
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

Quoted 10 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Andi Kleen

author

2007-09-17 20:22 UTC #7 lore raw

Quoted 7 lines · Sven-Thorsten Dietrich

Quoted 4 lines · Daniel Walker
Quoted 3 lines · Andi Kleen
Quoted 3 lines
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.

Quoted 1 line

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

Sven-Thorsten Dietrich

2007-09-17 20:31 UTC #8 lore raw

Quoted 9 lines · Andi Kleen

Quoted 7 lines · Sven-Thorsten Dietrich
Quoted 4 lines · Daniel Walker
Quoted 3 lines · Andi Kleen
Quoted 3 lines
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...

Quoted 7 lines

It's definitely not in 2.6.23-rc4-rt1 and also wasn't in the oops trace I looked at.
Quoted 1 line
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

Quoted 3 lines

-Andi

Gregory Haskins

2007-09-17 23:15 UTC #9 lore raw

In reply to Andi Kleen

Quoted 2 lines · Andi Kleen

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

Daniel Walker

2007-09-18 00:13 UTC #10 lore raw

In reply to Andi Kleen

Quoted 9 lines · Andi Kleen

Quoted 7 lines · Sven-Thorsten Dietrich
Quoted 4 lines · Daniel Walker
Quoted 3 lines · Andi Kleen
Quoted 3 lines
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...

Quoted 7 lines

It's definitely not in 2.6.23-rc4-rt1 and also wasn't in the oops trace I looked at.
Quoted 1 line
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

Andi Kleen

author

2007-09-18 08:07 UTC #11 lore raw

Quoted 5 lines

Quoted 3 lines
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

Thomas Gleixner

2007-09-18 08:30 UTC #12 lore raw

Quoted 7 lines · Andi Kleen

Quoted 4 lines
Quoted 2 lines
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