Skip to contentSkip to conversation

Oops path and debugger hooks rework

PATCH In review [email protected] 8 participants 29 messages Mar 21, 2007 – Apr 10, 2007 PowerPC

lore
Share & tools
lore

[email protected]

author
#0 lore raw

cover Oops path and debugger hooks rework

The following series cleans up our oops printout code and adds some missing functionality. It also changes xmon over to use the notify_die hooks and removes the awful debugger* hooks.

author
#1 lore raw

PATCH 1/10 Add missing oops_enter/oops_exit

Add missing oops_enter/oops_exit, makes pause_on_oops boot parameter work.

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/kernel/traps.c +3 -0
Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
@@ -99,6 +99,8 @@
 	if (debugger(regs))
 		return 1;
 
+	oops_enter();
+
 	console_verbose();
 	spin_lock_irq(&die_lock);
 	bust_spinlocks(1);

@@ -145,6 +147,7 @@
 	if (panic_on_oops)
 		panic("Fatal exception");
 
+	oops_exit();
 	do_exit(err);
 
 	return 0;

--

author
#2 lore raw

PATCH 2/10 Clean up pmac_backlight_unblank in oops path

In reply to [email protected]

Move pmac_backlight_unblank into its own function and only take the pmac_backlight_mutex when we are on a pmac for that added bit of paranoia.

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/kernel/traps.c +20 -11
Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
@@ -90,6 +90,24 @@
  * Trap & Exception support
  */
 
+#ifdef CONFIG_PMAC_BACKLIGHT
+static void pmac_backlight_unblank(void)
+{
+	mutex_lock(&pmac_backlight_mutex);
+	if (pmac_backlight) {
+		struct backlight_properties *props;
+
+		props = &pmac_backlight->props;
+		props->brightness = props->max_brightness;
+		props->power = FB_BLANK_UNBLANK;
+		backlight_update_status(pmac_backlight);
+	}
+	mutex_unlock(&pmac_backlight_mutex);
+}
+#else
+static inline void pmac_backlight_unblank(void) { }
+#endif
+
 static DEFINE_SPINLOCK(die_lock);
 
 int die(const char *str, struct pt_regs *regs, long err)

@@ -104,18 +122,9 @@
 	console_verbose();
 	spin_lock_irq(&die_lock);
 	bust_spinlocks(1);
-#ifdef CONFIG_PMAC_BACKLIGHT
-	mutex_lock(&pmac_backlight_mutex);
-	if (machine_is(powermac) && pmac_backlight) {
-		struct backlight_properties *props;
+	if (machine_is(powermac))
+		pmac_backlight_unblank();
 
-		props = &pmac_backlight->props;
-		props->brightness = props->max_brightness;
-		props->power = FB_BLANK_UNBLANK;
-		backlight_update_status(pmac_backlight);
-	}
-	mutex_unlock(&pmac_backlight_mutex);
-#endif
 	printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
 #ifdef CONFIG_PREEMPT
 	printk("PREEMPT ");

--

author
#3 lore raw

PATCH 3/10 Handle recursive oopses

In reply to [email protected]

Handle recursive oopses, like on x86. We had a few cases recently where we locked up in oops printing and didnt make it into crashdump.

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/kernel/traps.c +36 -16
Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
@@ -108,42 +108,62 @@
 static inline void pmac_backlight_unblank(void) { }
 #endif
 
-static DEFINE_SPINLOCK(die_lock);
-
 int die(const char *str, struct pt_regs *regs, long err)
 {
+	static struct {
+		spinlock_t lock;
+		u32 lock_owner;
+		int lock_owner_depth;
+	} die = {
+		.lock =			__SPIN_LOCK_UNLOCKED(die.lock),
+		.lock_owner =		-1,
+		.lock_owner_depth =	0
+	};
 	static int die_counter;
+	unsigned long flags;
 
 	if (debugger(regs))
 		return 1;
 
 	oops_enter();
 
-	console_verbose();
-	spin_lock_irq(&die_lock);
-	bust_spinlocks(1);
-	if (machine_is(powermac))
-		pmac_backlight_unblank();
+	if (die.lock_owner != raw_smp_processor_id()) {
+		console_verbose();
+		spin_lock_irqsave(&die.lock, flags);
+		die.lock_owner = smp_processor_id();
+		die.lock_owner_depth = 0;
+		bust_spinlocks(1);
+		if (machine_is(powermac))
+			pmac_backlight_unblank();
+	} else {
+		local_save_flags(flags);
+	}
 
-	printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
+	if (++die.lock_owner_depth < 3) {
+		printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
 #ifdef CONFIG_PREEMPT
-	printk("PREEMPT ");
+		printk("PREEMPT ");
 #endif
 #ifdef CONFIG_SMP
-	printk("SMP NR_CPUS=%d ", NR_CPUS);
+		printk("SMP NR_CPUS=%d ", NR_CPUS);
 #endif
 #ifdef CONFIG_DEBUG_PAGEALLOC
-	printk("DEBUG_PAGEALLOC ");
+		printk("DEBUG_PAGEALLOC ");
 #endif
 #ifdef CONFIG_NUMA
-	printk("NUMA ");
+		printk("NUMA ");
 #endif
-	printk("%s\n", ppc_md.name ? "" : ppc_md.name);
+		printk("%s\n", ppc_md.name ? "" : ppc_md.name);
+
+		print_modules();
+		show_regs(regs);
+	} else {
+		printk("Recursive die() failure, output suppressed\n");
+	}
 
-	print_modules();
-	show_regs(regs);
 	bust_spinlocks(0);
-	spin_unlock_irq(&die_lock);
+	die.lock_owner = -1;
+	spin_unlock_irqrestore(&die.lock, flags);
 
 	if (kexec_should_crash(current) ||
 		kexec_sr_activated(smp_processor_id()))

--

author
#4 lore raw

PATCH 4/10 Fix backwards ? : when printing machine type

In reply to [email protected]

Looks like someone got this backwards, highlighting the perils of the ? : !!! :)

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/kernel/traps.c +1 -1
Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
@@ -153,7 +153,7 @@
 #ifdef CONFIG_NUMA
 		printk("NUMA ");
 #endif
-		printk("%s\n", ppc_md.name ? "" : ppc_md.name);
+		printk("%s\n", ppc_md.name ? ppc_md.name : "");
 
 		print_modules();
 		show_regs(regs);

--

author
#5 lore raw

PATCH 5/10 Use KERN_EMERG everywhere in oops printout

In reply to [email protected]

Turn the volume up to 11. Distros will silence our oops messages no more. Use of sunglasses recommended.

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/kernel/traps.c +7 -3
Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
@@ -140,7 +140,10 @@
 	}
 
 	if (++die.lock_owner_depth < 3) {
-		printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
+		printk(KERN_EMERG "Oops: %s, sig: %ld [#%d]\n", str, err,
+		       ++die_counter);
+
+		printk(KERN_EMERG);
 #ifdef CONFIG_PREEMPT
 		printk("PREEMPT ");
 #endif

@@ -155,10 +158,11 @@
 #endif
 		printk("%s\n", ppc_md.name ? ppc_md.name : "");
 
+		printk(KERN_EMERG);
 		print_modules();
-		show_regs(regs);
+		show_regs_log_lvl(regs, KERN_EMERG);
 	} else {
-		printk("Recursive die() failure, output suppressed\n");
+		printk(KERN_EMERG "Recursive die() failure, output suppressed\n");
 	}
 
 	bust_spinlocks(0);
arch/powerpc/kernel/process.c +33 -19

134 lines not on this page · raw message

include/asm-powerpc/system.h +1 -0
Index: linux-2.6/include/asm-powerpc/system.h
===================================================================
@@ -103,6 +103,7 @@
 
 extern int set_dabr(unsigned long dabr);
 extern void print_backtrace(unsigned long *);
+extern void show_regs_log_lvl(struct pt_regs * regs, char * log_lvl);
 extern void show_regs(struct pt_regs * regs);
 extern void flush_instruction_cache(void);
 extern void hard_reset_now(void);
arch/powerpc/mm/fault.c +4 -4
Index: linux-2.6/arch/powerpc/mm/fault.c
===================================================================
@@ -429,20 +429,20 @@
 	switch (regs->trap) {
 	case 0x300:
 	case 0x380:
-		printk(KERN_ALERT "Unable to handle kernel paging request for "
+		printk(KERN_EMERG "Unable to handle kernel paging request for "
 			"data at address 0x%08lx\n", regs->dar);
 		break;
 	case 0x400:
 	case 0x480:
-		printk(KERN_ALERT "Unable to handle kernel paging request for "
+		printk(KERN_EMERG "Unable to handle kernel paging request for "
 			"instruction fetch\n");
 		break;
 	default:
-		printk(KERN_ALERT "Unable to handle kernel paging request for "
+		printk(KERN_EMERG "Unable to handle kernel paging request for "
 			"unknown fault\n");
 		break;
 	}
-	printk(KERN_ALERT "Faulting instruction address: 0x%08lx\n",
+	printk(KERN_EMERG "Faulting instruction address: 0x%08lx\n",
 		regs->nip);
 
 	die("Kernel access of bad area", regs, sig);

--

#7 lore raw

Re: [patch 05/10] Use KERN_EMERG everywhere in oops printout

In reply to [email protected]

arch/powerpc/kernel/process.c line 940

Quoted 9 lines · [email protected] writes:
arch/powerpc/kernel/process.c +2 -1
@@ -926,9 +934,10 @@
 		    && stack[FRAME_MARKER] == REGS_MARKER) {
 			struct pt_regs *regs = (struct pt_regs *)
 				(sp + STACK_FRAME_OVERHEAD);
-			printk("--- Exception: %lx", regs->trap);
+			printk("%s--- Exception: %lx", log_lvl, regs->trap);
 			print_symbol(" at %s\n", regs->nip);
 			lr = regs->link;
+			printk(KERN_EMERG);

Why is this one KERN_EMERG rather than log_lvl? Just an oversight?

Paul.

author
#8 lore raw

PATCH 6/10 Add notify die hooks and remove some redundant debugger hooks

In reply to [email protected]

Add a DIE_OOPS and DIE_MACHINE_CHECK notify_die hook and remove some redundant debugger* hooks.

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/kernel/traps.c +13 -6
Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
@@ -122,6 +122,10 @@
 	static int die_counter;
 	unsigned long flags;
 
+	if (notify_die(DIE_OOPS, str, regs, err, regs->trap, SIGSEGV) ==
+		       NOTIFY_STOP)
+		return 1;
+
 	if (debugger(regs))
 		return 1;
 

@@ -374,6 +378,11 @@
 	return;
 #endif
 
+        if (notify_die(DIE_MACHINE_CHECK, "machine_check", regs, 7, 7,
+		       SIGBUS) == NOTIFY_STOP) {
+		return;
+	}
+
 	if (debugger_fault_handler(regs)) {
 		regs->msr |= MSR_RI;
 		return;

@@ -505,8 +514,6 @@
 	 */
 	platform_machine_check(regs);
 
-	if (debugger_fault_handler(regs))
-		return;
 	die("Machine check", regs, SIGBUS);
 
 	/* Must die if the interrupt is not recoverable */

@@ -890,7 +897,6 @@
 {
 	printk(KERN_ERR "Non-recoverable exception at PC=%lx MSR=%lx\n",
 	       regs->nip, regs->msr);
-	debugger(regs);
 	die("nonrecoverable exception", regs, SIGKILL);
 }
 

@@ -936,10 +942,8 @@
 
 	CHECK_FULL_REGS(regs);
 
-	if (!user_mode(regs)) {
-		debugger(regs);
+	if (!user_mode(regs))
 		die("Kernel Mode Software FPU Emulation", regs, SIGFPE);
-	}
 
 #ifdef CONFIG_MATH_EMULATION
 	errcode = do_mathemu(regs);

@@ -992,6 +996,9 @@
 			mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~DBCR0_IC);
 			/* Clear the instruction completion event */
 			mtspr(SPRN_DBSR, DBSR_IC);
+			if (notify_die(DIE_SSTEP, "single_step", regs, 5, 5,
+				       SIGTRAP) == NOTIFY_STOP)
+				return;
 			if (debugger_sstep(regs))
 				return;
 		}
include/asm-powerpc/kdebug.h +1 -0
Index: linux-2.6/include/asm-powerpc/kdebug.h
===================================================================
@@ -30,6 +30,7 @@
 	DIE_BPT,
 	DIE_SSTEP,
 	DIE_PAGE_FAULT,
+	DIE_MACHINE_CHECK,
 };
 
 static inline int notify_die(enum die_val val,char *str,struct pt_regs *regs,long err,int trap, int sig)

--

#9 lore raw

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

include/asm-powerpc/kdebug.h line 36

Quoted 12 lines

Index: linux-2.6/include/asm-powerpc/kdebug.h
===================================================================

include/asm-powerpc/kdebug.h +1 -0
@@ -30,6 +30,7 @@
 	DIE_BPT,
 	DIE_SSTEP,
 	DIE_PAGE_FAULT,
+	DIE_MACHINE_CHECK,
 };
 
 static inline int notify_die(enum die_val val,char *str,struct pt_regs *regs,long err,int trap, int sig)

this is not around anymore after my patch to move the die notifier to common code and will need some trivial rediffing.

#10 lore raw

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

In reply to [email protected]
Quoted 2 lines · [email protected] writes:

Add a DIE_OOPS and DIE_MACHINE_CHECK notify_die hook and remove some redundant debugger* hooks.

As far as I can see, at the moment there is just one thing that gets registered to be called by die_notify, and that is kprobe_exceptions_notify. It doesn't do anything with DIE_OOPS or DIE_MACHINE_CHECK.

I would rather get rid of die_notify and have a kprobe_notify instead that just directly calls kprobe_exceptions_notify. In fact having separate kprobe routines for the different events would be even better.

It's not like our cpus are particularly good at indirect functions calls... ;(

Paul.

#11 lore raw

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

Quoted 10 lines · Paul Mackerras
Quoted 2 lines · [email protected] writes:

Add a DIE_OOPS and DIE_MACHINE_CHECK notify_die hook and remove some redundant debugger* hooks.

As far as I can see, at the moment there is just one thing that gets registered to be called by die_notify, and that is kprobe_exceptions_notify. It doesn't do anything with DIE_OOPS or DIE_MACHINE_CHECK.

Quoted 7 lines

I would rather get rid of die_notify and have a kprobe_notify instead that just directly calls kprobe_exceptions_notify. In fact having separate kprobe routines for the different events would be even better.

It's not like our cpus are particularly good at indirect functions calls... ;(

See the patch I posted a few days ago for the pagefault path..

#12 lore raw

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

In reply to Paul Mackerras
Quoted 9 lines · Paul Mackerras
Quoted 2 lines · [email protected] writes:

Add a DIE_OOPS and DIE_MACHINE_CHECK notify_die hook and remove some redundant debugger* hooks.

As far as I can see, at the moment there is just one thing that gets registered to be called by die_notify, and that is kprobe_exceptions_notify. It doesn't do anything with DIE_OOPS or DIE_MACHINE_CHECK.

Make that two: patch [8/10] of this series converts xmon to use die_notify and these two were added to facilitate that. In any case, kprobes just returns NOTIFY_DONE for these callbacks.

Quoted 4 lines

I would rather get rid of die_notify and have a kprobe_notify instead that just directly calls kprobe_exceptions_notify. In fact having separate kprobe routines for the different events would be even better.

Well, the die_notify hooks for DIE_SSTEP, DIE_BPT (and when kwatch is implemented for powerpc, even DIE_DABR_MATCH), are shared between kprobes and xmon, after the xmon conversion to use notifiers.

Ananth

author
#13 lore raw

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

In reply to Paul Mackerras

Hi,

Quoted 4 lines

As far as I can see, at the moment there is just one thing that gets registered to be called by die_notify, and that is kprobe_exceptions_notify. It doesn't do anything with DIE_OOPS or DIE_MACHINE_CHECK.

Keep reading :) Later on I remove all the debugger() indirect branches.

Quoted 7 lines

I would rather get rid of die_notify and have a kprobe_notify instead that just directly calls kprobe_exceptions_notify. In fact having separate kprobe routines for the different events would be even better.

It's not like our cpus are particularly good at indirect functions calls... ;(

True and we are removing a set of indirect branches that exist already (debugger). Id prefer not to have 2 sets of duplicate hooks for all this stuff.

Keep in mind with xmon and kprobes disabled these calls are fast. I verified it in sim after Christophs 300 cycle rant, a pessimistic estimate is 20 cycles on power5, including function call overhead.

Anton

#14 lore raw

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

Quoted 1 line · Anton Blanchard

Keep reading :) Later on I remove all the debugger() indirect branches.

Well, you replace one indirect branch with another. I'm not sure how that is any better.

Quoted 3 lines

Keep in mind with xmon and kprobes disabled these calls are fast. I verified it in sim after Christophs 300 cycle rant, a pessimistic estimate is 20 cycles on power5, including function call overhead.

With xmon disabled the debugger() call is already fast. Generally I see notifiers as being useful when you have some event, or some group of events, where you can imagine lots of different software modules (e.g. random device drivers) being interested in it. I don't see that that is the case with any of the kprobes notifiers.

Also I don't see any real point to forcing these disparate things to go through the one notify_die() interface, with the consequent demultiplexing that you then have to do in the receiving function.

Paul.

#15 lore raw

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

In reply to Anton Blanchard
Quoted 3 lines · Anton Blanchard

True and we are removing a set of indirect branches that exist already (debugger). Id prefer not to have 2 sets of duplicate hooks for all this stuff.

There are two concerns I have here. First, using notifiers tends to obscure what's going on since the notify call goes off and calls some random set of functions somewhere, and it's not obvious when reading the code what that set of functions is. That's fine when there is a reason why the set of kernel code modules that want to know about the event needs to be able to be extended at will without impacting the generator of the event, but that isn't the case with single-step, breakpoint or page fault. If there are only two functions to be called, I'd rather have a couple of explicit calls than use a notifier.

The other thing is that we seem to be throwing all sorts of unrelated events into the die_notifier. The symptom of that is that you have one set of handlers that are only interested in the debug events, and another set that are only interested in the events where the machine really is dying (oops, etc.). That is, you have two sets of handlers that are interested in disjoint sets of events. That says to me that we shouldn't be using a single notifier for all the events. Particularly for page faults I really don't see the point of calling handlers that are only interested in oopses.

Paul.

#16 lore raw

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

Quoted 10 lines · Paul Mackerras

The other thing is that we seem to be throwing all sorts of unrelated events into the die_notifier. The symptom of that is that you have one set of handlers that are only interested in the debug events, and another set that are only interested in the events where the machine really is dying (oops, etc.). That is, you have two sets of handlers that are interested in disjoint sets of events. That says to me that we shouldn't be using a single notifier for all the events. Particularly for page faults I really don't see the point of calling handlers that are only interested in oopses.

The page fault no longer uses the die_notifier. In recent kernels, we have done away with registering the page fault notifier unconditionally. In the kprobe case, its active only if there are any active probes.

As to the debug events vs. dying events both using the die_notifier, we did it that way just to maintain consistency across platforms. i386, x86_64 and sparc64 were using similar stuff before we put in the powerpc portions.

Aside, I was told that some folks use the page fault notifiers to determine application behaviour or somesuch and have used it effectively to solve some of their performance issues. I don't have the details though.

Ananth

author
#17 lore raw

PATCH 7/10 Page fault handler should not depend on CONFIG_KPROBES

In reply to [email protected]

Like x86-64, change the page fault notifier code to not depend on CONFIG_KPROBES and export the handlers.

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/mm/fault.c +2 -8
Index: linux-2.6/arch/powerpc/mm/fault.c
===================================================================
@@ -39,7 +39,6 @@
 #include <asm/kdebug.h>
 #include <asm/siginfo.h>
 
-#ifdef CONFIG_KPROBES
 ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
 
 /* Hook to register for page fault notifications */

@@ -47,11 +46,13 @@
 {
 	return atomic_notifier_chain_register(&notify_page_fault_chain, nb);
 }
+EXPORT_SYMBOL_GPL(register_page_fault_notifier);
 
 int unregister_page_fault_notifier(struct notifier_block *nb)
 {
 	return atomic_notifier_chain_unregister(&notify_page_fault_chain, nb);
 }
+EXPORT_SYMBOL_GPL(unregister_page_fault_notifier);
 
 static inline int notify_page_fault(enum die_val val, const char *str,
 			struct pt_regs *regs, long err, int trap, int sig)

@@ -65,13 +66,6 @@
 	};
 	return atomic_notifier_call_chain(&notify_page_fault_chain, val, &args);
 }
-#else
-static inline int notify_page_fault(enum die_val val, const char *str,
-			struct pt_regs *regs, long err, int trap, int sig)
-{
-	return NOTIFY_DONE;
-}
-#endif
 
 /*
  * Check whether the instruction at regs->nip is a store using
include/asm-powerpc/kdebug.h +2 -1
Index: linux-2.6/include/asm-powerpc/kdebug.h
===================================================================
@@ -33,7 +33,8 @@
 	DIE_MACHINE_CHECK,
 };
 
-static inline int notify_die(enum die_val val,char *str,struct pt_regs *regs,long err,int trap, int sig)
+static inline int notify_die(enum die_val val, const char *str,
+			     struct pt_regs *regs, long err, int trap, int sig)
 {
 	struct die_args args = { .regs=regs, .str=str, .err=err, .trapnr=trap,.signr=sig };
 	return atomic_notifier_call_chain(&powerpc_die_chain, val, &args);

--

#18 lore raw

Re: [patch 07/10] Page fault handler should not depend on CONFIG_KPROBES

Quoted 2 lines · On Tue, Mar 20, 2007 at 08:38:17PM -0500, [email protected] wrote:

Like x86-64, change the page fault notifier code to not depend on CONFIG_KPROBES and export the handlers.

NACK until the performace implication a fixed. Martin measured this costs 300 cycles on s390 even without any notifier activce, and while ppc might be slightly bad that's still too much to add to the pagefault handler unconditionally. Please add a light weight method to check whether there is any activce user of the notifications. Also please move the code into kernel/ at the same time instead of having the exact same thing duplicated all over, as in my notify_die patches.

arch/powerpc/mm/fault.c line 55

Quoted 3 lines
arch/powerpc/mm/fault.c +2 -0
+EXPORT_SYMBOL_GPL(register_page_fault_notifier);
 
+EXPORT_SYMBOL_GPL(unregister_page_fault_notifier);

Please drop these exports. There's not modular users of it, and the target users (kprobes and debuggers) can't be modular either.

author
#19 lore raw

Re: [patch 07/10] Page fault handler should not depend on CONFIG_KPROBES

Quoted 8 lines

NACK until the performace implication a fixed. Martin measured this costs 300 cycles on s390 even without any notifier activce, and while ppc might be slightly bad that's still too much to add to the pagefault handler unconditionally. Please add a light weight method to check whether there is any activce user of the notifications. Also please move the code into kernel/ at the same time instead of having the exact same thing duplicated all over, as in my notify_die patches.

Im struggling to understand how they could take 300 cycles to execute this code sequence. We have seen similar questions on the ia64 side, they both must have some pretty fundamental branch prediction issues.

I measured this in sim and the sequence takes under 20 cycles on a power5 class machine, and thats a pessimistic estimate.

Anton

author
#20 lore raw

PATCH 8/10 Use notifier hooks for xmon

In reply to [email protected]

Use notifier hooks for xmon and remove all the debugger* junk. Based on a patch by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/xmon/xmon.c +80 -28
Index: linux-2.6/arch/powerpc/xmon/xmon.c
===================================================================
@@ -40,6 +40,7 @@
 #include <asm/spu.h>
 #include <asm/spu_priv1.h>
 #include <asm/firmware.h>
+#include <asm/kdebug.h>
 
 #ifdef CONFIG_PPC64
 #include <asm/hvcall.h>

@@ -2578,38 +2579,91 @@
 }
 #endif
 
-void xmon_init(int enable)
+static int xmon_exceptions_notify(struct notifier_block *self,
+				  unsigned long val, void *data)
+{
+	struct die_args *args = (struct die_args *)data;
+
+	switch (val) {
+		case DIE_OOPS:
+		case DIE_DEBUGGER:
+			if (xmon(args->regs))
+				return NOTIFY_STOP;
+			break;
+		case DIE_IABR_MATCH:
+			if (xmon_iabr_match(args->regs))
+				return NOTIFY_STOP;
+			break;
+		case DIE_DABR_MATCH:
+			if (xmon_dabr_match(args->regs))
+				return NOTIFY_STOP;
+			break;
+		case DIE_BPT:
+			if (xmon_bpt(args->regs))
+				return NOTIFY_STOP;
+			break;
+		case DIE_SSTEP:
+			if (xmon_sstep(args->regs))
+				return NOTIFY_STOP;
+			break;
+		case DIE_PAGE_FAULT:
+			if ((TRAP(args->regs) == 0x300 ||
+			    TRAP(args->regs) == 0x400) &&
+			    xmon_fault_handler(args->regs))
+				return NOTIFY_STOP;
+			break;
+		case DIE_MACHINE_CHECK:
+			if (xmon_fault_handler(args->regs)) {
+				/* XXX doesnt look safe to me */
+				args->regs->msr |= MSR_RI;
+				return NOTIFY_STOP;
+			}
+			break;
+		case DIE_IPI:
+			if (!xmon_ipi(args->regs))
+				return NOTIFY_STOP;
+			break;
+		default:
+			break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+/* XXX What priority should we set? */
+static struct notifier_block xmon_exceptions_nb = {
+	.notifier_call = xmon_exceptions_notify,
+};
+
+static struct notifier_block xmon_page_fault_nb = {
+	.notifier_call = xmon_exceptions_notify,
+};
+
+void xmon_enable(void)
 {
-#ifdef CONFIG_PPC_ISERIES
 	if (firmware_has_feature(FW_FEATURE_ISERIES))
 		return;
-#endif
-	if (enable) {
-		__debugger = xmon;
-		__debugger_ipi = xmon_ipi;
-		__debugger_bpt = xmon_bpt;
-		__debugger_sstep = xmon_sstep;
-		__debugger_iabr_match = xmon_iabr_match;
-		__debugger_dabr_match = xmon_dabr_match;
-		__debugger_fault_handler = xmon_fault_handler;
-	} else {
-		__debugger = NULL;
-		__debugger_ipi = NULL;
-		__debugger_bpt = NULL;
-		__debugger_sstep = NULL;
-		__debugger_iabr_match = NULL;
-		__debugger_dabr_match = NULL;
-		__debugger_fault_handler = NULL;
-	}
+
+	register_die_notifier(&xmon_exceptions_nb);
+	register_page_fault_notifier(&xmon_page_fault_nb);
 	xmon_map_scc();
 }
 
+void xmon_disable(void)
+{
+	if (firmware_has_feature(FW_FEATURE_ISERIES))
+		return;
+
+	unregister_die_notifier(&xmon_exceptions_nb);
+	unregister_page_fault_notifier(&xmon_page_fault_nb);
+}
+
 #ifdef CONFIG_MAGIC_SYSRQ
 static void sysrq_handle_xmon(int key, struct tty_struct *tty) 
 {
 	/* ensure xmon is enabled */
-	xmon_init(1);
-	debugger(get_irq_regs());
+	xmon_enable();
+	xmon(get_irq_regs());
 }
 
 static struct sysrq_key_op sysrq_xmon_op = 

@@ -2621,10 +2675,8 @@
 
 static int __init setup_xmon_sysrq(void)
 {
-#ifdef CONFIG_PPC_ISERIES
 	if (firmware_has_feature(FW_FEATURE_ISERIES))
 		return 0;
-#endif
 	register_sysrq_key('x', &sysrq_xmon_op);
 	return 0;
 }

@@ -2637,10 +2689,10 @@
 {
 	if (!p || strncmp(p, "early", 5) == 0) {
 		/* just "xmon" is equivalent to "xmon=early" */
-		xmon_init(1);
+		xmon_enable();
 		xmon_early = 1;
 	} else if (strncmp(p, "on", 2) == 0)
-		xmon_init(1);
+		xmon_enable();
 	else if (strncmp(p, "off", 3) == 0)
 		xmon_off = 1;
 	else if (strncmp(p, "nobt", 4) == 0)

@@ -2656,10 +2708,10 @@
 {
 #ifdef CONFIG_XMON_DEFAULT
 	if (!xmon_off)
-		xmon_init(1);
+		xmon_enable();
 #endif
 	if (xmon_early)
-		debugger(NULL);
+		xmon(NULL);
 }
 
 #ifdef CONFIG_SPU_BASE
include/asm-powerpc/system.h +0 -36
Index: linux-2.6/include/asm-powerpc/system.h
===================================================================
@@ -65,42 +65,6 @@
 struct task_struct;
 struct pt_regs;
 
-#ifdef CONFIG_DEBUGGER
-
-extern int (*__debugger)(struct pt_regs *regs);
-extern int (*__debugger_ipi)(struct pt_regs *regs);
-extern int (*__debugger_bpt)(struct pt_regs *regs);
-extern int (*__debugger_sstep)(struct pt_regs *regs);
-extern int (*__debugger_iabr_match)(struct pt_regs *regs);
-extern int (*__debugger_dabr_match)(struct pt_regs *regs);
-extern int (*__debugger_fault_handler)(struct pt_regs *regs);
-
-#define DEBUGGER_BOILERPLATE(__NAME) \
-static inline int __NAME(struct pt_regs *regs) \
-{ \
-	if (unlikely(__ ## __NAME)) \
-		return __ ## __NAME(regs); \
-	return 0; \
-}
-
-DEBUGGER_BOILERPLATE(debugger)
-DEBUGGER_BOILERPLATE(debugger_ipi)
-DEBUGGER_BOILERPLATE(debugger_bpt)
-DEBUGGER_BOILERPLATE(debugger_sstep)
-DEBUGGER_BOILERPLATE(debugger_iabr_match)
-DEBUGGER_BOILERPLATE(debugger_dabr_match)
-DEBUGGER_BOILERPLATE(debugger_fault_handler)
-
-#else
-static inline int debugger(struct pt_regs *regs) { return 0; }
-static inline int debugger_ipi(struct pt_regs *regs) { return 0; }
-static inline int debugger_bpt(struct pt_regs *regs) { return 0; }
-static inline int debugger_sstep(struct pt_regs *regs) { return 0; }
-static inline int debugger_iabr_match(struct pt_regs *regs) { return 0; }
-static inline int debugger_dabr_match(struct pt_regs *regs) { return 0; }
-static inline int debugger_fault_handler(struct pt_regs *regs) { return 0; }
-#endif
-
 extern int set_dabr(unsigned long dabr);
 extern void print_backtrace(unsigned long *);
 extern void show_regs_log_lvl(struct pt_regs * regs, char * log_lvl);
arch/powerpc/kernel/traps.c +1 -35
Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
@@ -54,24 +54,6 @@
 #endif
 #include <asm/kexec.h>
 
-#ifdef CONFIG_DEBUGGER
-int (*__debugger)(struct pt_regs *regs);
-int (*__debugger_ipi)(struct pt_regs *regs);
-int (*__debugger_bpt)(struct pt_regs *regs);
-int (*__debugger_sstep)(struct pt_regs *regs);
-int (*__debugger_iabr_match)(struct pt_regs *regs);
-int (*__debugger_dabr_match)(struct pt_regs *regs);
-int (*__debugger_fault_handler)(struct pt_regs *regs);
-
-EXPORT_SYMBOL(__debugger);
-EXPORT_SYMBOL(__debugger_ipi);
-EXPORT_SYMBOL(__debugger_bpt);
-EXPORT_SYMBOL(__debugger_sstep);
-EXPORT_SYMBOL(__debugger_iabr_match);
-EXPORT_SYMBOL(__debugger_dabr_match);
-EXPORT_SYMBOL(__debugger_fault_handler);
-#endif
-
 ATOMIC_NOTIFIER_HEAD(powerpc_die_chain);
 
 int register_die_notifier(struct notifier_block *nb)

@@ -126,9 +108,6 @@
 		       NOTIFY_STOP)
 		return 1;
 
-	if (debugger(regs))
-		return 1;
-
 	oops_enter();
 
 	if (die.lock_owner != raw_smp_processor_id()) {

@@ -383,11 +362,6 @@
 		return;
 	}
 
-	if (debugger_fault_handler(regs)) {
-		regs->msr |= MSR_RI;
-		return;
-	}
-
 	if (check_io_access(regs))
 		return;
 

@@ -539,8 +513,6 @@
 	if (notify_die(DIE_IABR_MATCH, "iabr_match", regs, 5,
 					5, SIGTRAP) == NOTIFY_STOP)
 		return;
-	if (debugger_iabr_match(regs))
-		return;
 	_exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
 }
 

@@ -556,8 +528,6 @@
 	if (notify_die(DIE_SSTEP, "single_step", regs, 5,
 					5, SIGTRAP) == NOTIFY_STOP)
 		return;
-	if (debugger_sstep(regs))
-		return;
 
 	_exception(SIGTRAP, regs, TRAP_TRACE, regs->nip);
 }

@@ -798,8 +768,6 @@
 		if (notify_die(DIE_BPT, "breakpoint", regs, 5, 5, SIGTRAP)
 				== NOTIFY_STOP)
 			return;
-		if (debugger_bpt(regs))
-			return;
 
 		if (!(regs->msr & MSR_PR) &&  /* not user-mode */
 		    report_bug(regs->nip) == BUG_TRAP_TYPE_WARN) {

@@ -888,7 +856,7 @@
 {
 	printk(KERN_CRIT "Kernel stack overflow in process %p, r1=%lx\n",
 	       current, regs->gpr[1]);
-	debugger(regs);
+	notify_die(DIE_DEBUGGER, "die", regs, 5, 5, SIGSEGV);
 	show_regs(regs);
 	panic("kernel stack overflow");
 }

@@ -999,8 +967,6 @@
 			if (notify_die(DIE_SSTEP, "single_step", regs, 5, 5,
 				       SIGTRAP) == NOTIFY_STOP)
 				return;
-			if (debugger_sstep(regs))
-				return;
 		}
 		_exception(SIGTRAP, regs, TRAP_TRACE, 0);
 	}
arch/powerpc/mm/fault.c +0 -8
Index: linux-2.6/arch/powerpc/mm/fault.c
===================================================================
@@ -115,9 +115,6 @@
 			11, SIGSEGV) == NOTIFY_STOP)
 		return;
 
-	if (debugger_dabr_match(regs))
-		return;
-
 	/* Clear the DABR */
 	set_dabr(0);
 

@@ -173,11 +170,6 @@
 				11, SIGSEGV) == NOTIFY_STOP)
 		return 0;
 
-	if (trap == 0x300) {
-		if (debugger_fault_handler(regs))
-			return 0;
-	}
-
 	/* On a kernel SLB miss we can only check for a valid exception entry */
 	if (!user_mode(regs) && (address >= TASK_SIZE))
 		return SIGSEGV;
arch/powerpc/kernel/smp.c +8 -6
Index: linux-2.6/arch/powerpc/kernel/smp.c
===================================================================
@@ -45,6 +45,7 @@
 #include <asm/system.h>
 #include <asm/mpic.h>
 #include <asm/vdso_datapage.h>
+#include <asm/kdebug.h>
 #ifdef CONFIG_PPC64
 #include <asm/paca.h>
 #endif

@@ -108,10 +109,9 @@
 			crash_ipi_function_ptr(get_irq_regs());
 			break;
 		}
-#ifdef CONFIG_DEBUGGER
-		debugger_ipi(get_irq_regs());
-		break;
-#endif /* CONFIG_DEBUGGER */
+		if (notify_die(DIE_IPI, "smp_call_function",
+			       get_irq_regs(), 5, 5, SIGSEGV) == NOTIFY_STOP)
+			break;
 		/* FALLTHROUGH */
 	default:
 		printk("SMP %d: smp_message_recv(): unknown msg %d\n",

@@ -233,7 +233,8 @@
 			printk("smp_call_function on cpu %d: other cpus not "
 			       "responding (%d)\n", smp_processor_id(),
 			       atomic_read(&data.started));
-			debugger(NULL);
+			notify_die(DIE_DEBUGGER, "smp_call_function",
+				   NULL, 5, 5, SIGSEGV);
 			goto out;
 		}
 	}

@@ -247,7 +248,8 @@
 				       smp_processor_id(),
 				       atomic_read(&data.finished),
 				       atomic_read(&data.started));
-				debugger(NULL);
+				notify_die(DIE_DEBUGGER, "smp_call_function",
+					   NULL, 5, 5, SIGSEGV);
 				goto out;
 			}
 		}
include/asm-powerpc/kdebug.h +2 -0
Index: linux-2.6/include/asm-powerpc/kdebug.h
===================================================================
@@ -31,6 +31,8 @@
 	DIE_SSTEP,
 	DIE_PAGE_FAULT,
 	DIE_MACHINE_CHECK,
+	DIE_IPI,
+	DIE_DEBUGGER,
 };
 
 static inline int notify_die(enum die_val val, const char *str,

--

author
#21 lore raw

PATCH 9/10 Use lowercase for hex printouts in oops messages.

In reply to [email protected]

Use lowercase for hex printouts in oops messages. The number of times I have tried to copy and paste from an oops into an objdump search...

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/kernel/process.c +3 -3
Index: linux-2.6/arch/powerpc/kernel/process.c
===================================================================
@@ -400,11 +400,11 @@
 }
 
 #ifdef CONFIG_PPC64
-#define REG		"%016lX"
+#define REG		"%016lx"
 #define REGS_PER_LINE	4
 #define LAST_VOLATILE	13
 #else
-#define REG		"%08lX"
+#define REG		"%08lx"
 #define REGS_PER_LINE	8
 #define LAST_VOLATILE	12
 #endif

@@ -422,7 +422,7 @@
 	       regs, regs->trap, print_tainted(), init_utsname()->release);
 	printk("%sMSR: "REG" ", log_lvl, regs->msr);
 	printbits(regs->msr, msr_bits);
-	printk("  CR: %08lX  XER: %08lX\n", regs->ccr, regs->xer);
+	printk("  CR: %08lx  XER: %08lx\n", regs->ccr, regs->xer);
 	trap = TRAP(regs);
 	if (trap == 0x300 || trap == 0x600)
 		printk("%sDAR: "REG", DSISR: "REG"\n", log_lvl, regs->dar,

--

Acked-by on v1 9/10
#22 lore raw

Re: [patch 09/10] Use lowercase for hex printouts in oops messages.

Quoted 2 lines · On Tue, Mar 20, 2007 at 08:38:19PM -0500, [email protected] wrote:

Use lowercase for hex printouts in oops messages. The number of times I have tried to copy and paste from an oops into an objdump search...

YES! Finally someone bothered to fix this. Thank you!

Quoted 1 line
Signed-off-by Anton Blanchard <[email protected]>
Acked-by Olof Johansson <[email protected]>

-Olof

#23 lore raw

Re: [patch 09/10] Use lowercase for hex printouts in oops messages.

Quoted 5 lines · Olof Johansson
Quoted 2 lines · On Tue, Mar 20, 2007 at 08:38:19PM -0500, [email protected] wrote:

Use lowercase for hex printouts in oops messages. The number of times I have tried to copy and paste from an oops into an objdump search...

YES! Finally someone bothered to fix this. Thank you!

What about people pasting them into dc? ;-)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE) [email protected] ------- The Corporate Village, Da Vincilaan 7-D1 Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

#24 lore raw

Re: [patch 09/10] Use lowercase for hex printouts in oops messages.

Quoted 4 lines
Quoted 4 lines
Quoted 2 lines

Use lowercase for hex printouts in oops messages. The number of times I have tried to copy and paste from an oops into an objdump search...

YES! Finally someone bothered to fix this. Thank you!

Bis.

Quoted 1 line

What about people pasting them into dc? ;-)

So _that's_ how it works in dc! :-)

Segher

author
#25 lore raw

PATCH 10/10 Make sure we only enable xmon once

In reply to [email protected]

Now we use notifier hooks we have to be careful not to enable xmon multiple times.

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/xmon/xmon.c +16 -8
Index: linux-2.6/arch/powerpc/xmon/xmon.c
===================================================================
@@ -2658,11 +2658,20 @@
 	unregister_page_fault_notifier(&xmon_page_fault_nb);
 }
 
+#ifdef CONFIG_XMON_DEFAULT
+int __initdata xmon_on = 1;
+#else
+int __initdata xmon_on = 0;
+#endif
+
 #ifdef CONFIG_MAGIC_SYSRQ
 static void sysrq_handle_xmon(int key, struct tty_struct *tty) 
 {
 	/* ensure xmon is enabled */
-	xmon_enable();
+	if (!xmon_on) {
+		xmon_on = 1;
+		xmon_enable();
+	}
 	xmon(get_irq_regs());
 }
 

@@ -2683,18 +2692,18 @@
 __initcall(setup_xmon_sysrq);
 #endif /* CONFIG_MAGIC_SYSRQ */
 
-int __initdata xmon_early, xmon_off;
+int __initdata xmon_early;
 
 static int __init early_parse_xmon(char *p)
 {
 	if (!p || strncmp(p, "early", 5) == 0) {
 		/* just "xmon" is equivalent to "xmon=early" */
-		xmon_enable();
+		xmon_on = 1;
 		xmon_early = 1;
 	} else if (strncmp(p, "on", 2) == 0)
-		xmon_enable();
+		xmon_on = 1;
 	else if (strncmp(p, "off", 3) == 0)
-		xmon_off = 1;
+		xmon_on = 0;
 	else if (strncmp(p, "nobt", 4) == 0)
 		xmon_no_auto_backtrace = 1;
 	else

@@ -2706,10 +2715,9 @@
 
 void __init xmon_setup(void)
 {
-#ifdef CONFIG_XMON_DEFAULT
-	if (!xmon_off)
+	if (xmon_on)
 		xmon_enable();
-#endif
+
 	if (xmon_early)
 		xmon(NULL);
 }

--

#26 lore raw

Re: [patch 10/10] Make sure we only enable xmon once

Quoted 11 lines · On Tue, Mar 20, 2007 at 08:38:20PM -0500, [email protected] wrote:

Now we use notifier hooks we have to be careful not to enable xmon multiple times.

Signed-off-by Anton Blanchard <[email protected]>

---

Index: linux-2.6/arch/powerpc/xmon/xmon.c
===================================================================

arch/powerpc/xmon/xmon.c line 2665

Quoted 8 lines
arch/powerpc/xmon/xmon.c +5 -0
@@ -2658,11 +2658,20 @@
 	unregister_page_fault_notifier(&xmon_page_fault_nb);
 }
 
+#ifdef CONFIG_XMON_DEFAULT
+int __initdata xmon_on = 1;
+#else
+int __initdata xmon_on = 0;
+#endif

This shouldn't be __initdata, should it? It's used at runtime below.

arch/powerpc/xmon/xmon.c line 2676

Quoted 12 lines
arch/powerpc/xmon/xmon.c +5 -1
+
 #ifdef CONFIG_MAGIC_SYSRQ
 static void sysrq_handle_xmon(int key, struct tty_struct *tty) 
 {
 	/* ensure xmon is enabled */
-	xmon_enable();
+	if (!xmon_on) {
+		xmon_on = 1;
+		xmon_enable();
+	}
 	xmon(get_irq_regs());
 }

[...]

-Olof

author
#27 lore raw

Re: [patch 10/10] Make sure we only enable xmon once

Quoted 8 lines

arch/powerpc/xmon/xmon.c line 2665

Quoted 6 lines
arch/powerpc/xmon/xmon.c +5 -0
+#ifdef CONFIG_XMON_DEFAULT
+int __initdata xmon_on = 1;
+#else
+int __initdata xmon_on = 0;
+#endif

This shouldn't be __initdata, should it? It's used at runtime below.

Thanks for spotting! New patch on the way.

Anton

author
#28 lore raw

Re: [patch 10/10] Make sure we only enable xmon once

In reply to [email protected]

[Resent with __initdata fix from Olof]

Now we use notifier hooks we have to be careful not to enable xmon multiple times.

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/xmon/xmon.c +16 -8
Index: linux-2.6/arch/powerpc/xmon/xmon.c
===================================================================
@@ -2658,11 +2658,20 @@
 	unregister_page_fault_notifier(&xmon_page_fault_nb);
 }
 
+#ifdef CONFIG_XMON_DEFAULT
+int xmon_on = 1;
+#else
+int xmon_on = 0;
+#endif
+
 #ifdef CONFIG_MAGIC_SYSRQ
 static void sysrq_handle_xmon(int key, struct tty_struct *tty) 
 {
 	/* ensure xmon is enabled */
-	xmon_enable();
+	if (!xmon_on) {
+		xmon_on = 1;
+		xmon_enable();
+	}
 	xmon(get_irq_regs());
 }
 

@@ -2683,18 +2692,18 @@
 __initcall(setup_xmon_sysrq);
 #endif /* CONFIG_MAGIC_SYSRQ */
 
-int __initdata xmon_early, xmon_off;
+int __initdata xmon_early;
 
 static int __init early_parse_xmon(char *p)
 {
 	if (!p || strncmp(p, "early", 5) == 0) {
 		/* just "xmon" is equivalent to "xmon=early" */
-		xmon_enable();
+		xmon_on = 1;
 		xmon_early = 1;
 	} else if (strncmp(p, "on", 2) == 0)
-		xmon_enable();
+		xmon_on = 1;
 	else if (strncmp(p, "off", 3) == 0)
-		xmon_off = 1;
+		xmon_on = 0;
 	else if (strncmp(p, "nobt", 4) == 0)
 		xmon_no_auto_backtrace = 1;
 	else

@@ -2706,10 +2715,9 @@
 
 void __init xmon_setup(void)
 {
-#ifdef CONFIG_XMON_DEFAULT
-	if (!xmon_off)
+	if (xmon_on)
 		xmon_enable();
-#endif
+
 	if (xmon_early)
 		xmon(NULL);
 }

--

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev