Skip to contentSkip to conversation

garbled oopsen

DISCUSSION Andi Kleen 3 participants 5 messages May 8, 2003 2 messages not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

Andi Kleen

author
#2 lore raw
Quoted 5 lines · Andrew Morton
Quoted 1 line

Can these be cleaned up in any reasonable way?

It needs some additional spinlock in there. People have moaned for over a year, patches have been floating about but nobody has taken the time to finish one off and submit it.

I considered it for x86-64 and even implemented it, but never submitted in fear of deadlocks e.g. when an oops recurses. For this a spinlock_timeout() would be useful. Print anyways when you cannot get the lock in a second or two.

-Andi

#3 lore raw
Quoted 10 lines
Quoted 5 lines
Quoted 1 line

Can these be cleaned up in any reasonable way?

It needs some additional spinlock in there. People have moaned for over a year, patches have been floating about but nobody has taken the time to finish one off and submit it.

I considered it for x86-64 and even implemented it, but never submitted in fear of deadlocks e.g. when an oops recurses. For this a spinlock_timeout() would be useful. Print anyways when you cannot get the lock in a second or two.

The trouble is that the subsystems you want may be broken (eg timers). IMHO it's better to just spew whatever you can (the current crap) ... wait a couple of seconds, then have another go at doing it properly.

That way people can't complain it's worse than it is now in any way ;-)

M.

#4 lore raw
Quoted 7 lines · Martin J. Bligh
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines

Can these be cleaned up in any reasonable way?

It needs some additional spinlock in there. People have moaned for over a year, patches have been floating about but nobody has taken the time to finish one off and submit it.

Quoted 8 lines
Quoted 4 lines

I considered it for x86-64 and even implemented it, but never submitted in fear of deadlocks e.g. when an oops recurses. For this a spinlock_timeout() would be useful. Print anyways when you cannot get the lock in a second or two.

The trouble is that the subsystems you want may be broken (eg timers). IMHO it's better to just spew whatever you can (the current crap) ... wait a couple of seconds, then have another go at doing it properly.

A recursive oops is easy enough to detect anyway.

	preempt_disable();
	if (oops_cpu == -1 || oops_cpu != smp_processor_id()) {
		_raw_spin_lock(&oops_lock);
		oops_cpu = smp_processor_id();
	}
	<current stuff>
	oops_cpu = -1;
	spin_lock_init(&oops_lock);
	preempt_enable();

or something like that.

Quoted 1 line

That way people can't complain it's worse than it is now in any way ;-)

Too many complaints, too few unified diffs on this one.

author
#5 lore raw
Quoted 12 lines · Andrew Morton

A recursive oops is easy enough to detect anyway.

	preempt_disable();
	if (oops_cpu == -1 || oops_cpu != smp_processor_id()) {
		_raw_spin_lock(&oops_lock);
		oops_cpu = smp_processor_id();
	}
	<current stuff>
	oops_cpu = -1;
	spin_lock_init(&oops_lock);
	preempt_enable();
Quoted 1 line

or something like that.

yes I did it this way in my old 2.4 x86-64 patch. But i never felt comfortable enough about it to commit it.

(the in_interrupt thing was to avoid an interrupt stack problem on x86-64, not needed anymore or on i386)

But I would prefer the spinlock timeout I think. It's an safer and more obviously correct algorithm.

-Andi

Index: arch/x86_64/mm/fault.c
===================================================================
RCS file: /home/cvs/Repository/linux/arch/x86_64/mm/fault.c,v retrieving revision 1.33

x86_64/mm/fault.c +18 -0
diff -u -u -r1.33 fault.c
@@ -30,6 +30,9 @@
 #include <asm/proto.h>
 #include <asm/kdebug.h>
 
+spinlock_t pcrash_lock; 
+int crashing_cpu;
+
 extern spinlock_t console_lock, timerlist_lock;
 
 void bust_spinlocks(int yes)

@@ -251,6 +254,14 @@
 	console_verbose();
 	bust_spinlocks(1); 
 
+	if (!in_interrupt()) { 
+		if (!spin_trylock(&pcrash_lock)) { 
+			if (crashing_cpu != smp_processor_id()) 
+				spin_lock(&pcrash_lock);  		    
+		} 
+		crashing_cpu = smp_processor_id();
+	} 
+
 	if (address < PAGE_SIZE)
 		printk(KERN_ALERT "Unable to handle kernel NULL pointer dereference");
 	else

@@ -259,7 +270,14 @@
 	printk(" printing rip:\n");
 	printk("%016lx\n", regs->rip);
 	dump_pagetable(address);
+
 	die("Oops", regs, error_code);
+
+	if (!in_interrupt()) { 
+		crashing_cpu = -1;  /* small harmless window */ 
+		spin_unlock(&pcrash_lock);
+	}
+
 	bust_spinlocks(0); 
 	do_exit(SIGKILL);
author
#6 lore raw
In reply to Martin J. Bligh
Quoted 1 line · Martin J. Bligh

The trouble is that the subsystems you want may be broken (eg timers).

rdtsc/get_cycles() should still work. If that's broken too you have a really serious problem. It's only on the local CPU, so you don't need any complications for bro^wunsynced SMP systems.

-Andi