Generic BUG for parisc

Indeed. that's cool.

...

This looks good to me.
It's up to kyle/willy on how they want it pushed back upstream.

I'm sure it will affect the runtime of the kernel becuase it moves a fair chunk of code "out of line". Ie we should get slightly better i-cache utilization with this change.

With this, we could use BUG() to _measure_ exactly how many times we hit a particular BUG() test. It would be usefule to determine how performance critical particular BUG() tests are.
Add a 32-bit counter to the generic struct bug_entry and then increment that in the trap handler. Hrm...need a /proc or /sys interface to dump all of them.

I'd not worry about optimizations until after it's seen broader use. Keep in mind 12 bytes (for 32-bit kernel) is only 3 instructions. So each BUG() only uses 16 bytes + file name string. That's already a major improvement over our existing BUG() which has to setup printk()/dump_stack()/panic calls - at least 56 bytes (14 ops) each.

Keep in mind this change saves us around 55kB (1379*40). Saving another 2 bytes isn't that critical. IMHO, it's not worth making things too complicated just to save another 3kB.

Kudos!

thanks,
grant

Yes, i think so.

I think the way you have it is fine, the break traps only when executed, so it should not affect normal execution.

The only thing I noticed is that your patch has various "cleanups" or maybe just whitespace changes that is mixed it. It's not a big deal, but maybe it'll be cleaner if those are separated out.

Please don't do this. It will make things confusing.

Note that there are some other tools that use the break infrastructure, and some of them will be in kernel space. For example, kdb uses "break" to implement breakpoints. If we implement tools like kprobes, they will probably need to use "break" too. We need to have unique break numbers if these tools were to coexist. For reference, the parisc kdb implementation uses the following:

+#define KDB_BREAK_BREAK 0x111 /* kdb breakpoint in kernel */
+#define KDB_BREAK_ENTER 0x112 /* KDB_ENTER() */
+

+#define PA_BREAK_INSN(n)       ((n)<<13)
+#define KDB_BREAK_BREAK_INSN   PA_BREAK_INSN(KDB_BREAK_BREAK)
+#define KDB_BREAK_ENTER_INSN   PA_BREAK_INSN(KDB_BREAK_ENTER)
+
+#define KDB_ENTER2(b)  asm("\tbreak 0,"#b"\n")
+#define KDB_ENTER1(b)  KDB_ENTER2(b)
+#define KDB_ENTER()    KDB_ENTER1(KDB_BREAK_ENTER)

my 2 cents :)
randolph

On Sunday 10 December 2006 06:37, Grant Grundler wrote:
On Sunday 10 December 2006 14:38, Randolph Chung wrote:
...
Thanks a lot for your both's feedback!

In summary I read, that it's better to keep the patch like it is now without major changes.

Further possible enhancements would be:
- add performance couters to measure how often the tests ran
- implement WARN() the same way

A slightly modified patch (in asm-parisc/bug.h) is attached. It still has 1-2 minimal other cleanups.

Kyle/Willy: Do you think you can integrate this patch into what goes to Linus for 2.6.20 ?

Thanks!
Helge

diff-tree 571c18bb8c00794f38bc522a9d62d699ec17f9b8 (from 12166cbf2a4b90bc9d82262f52a8568362166f91)

Author: Helge Deller <[email protected]>
Date:   Sat Dec 9 16:41:00 2006 +0100

[PATCH] Generic BUG for parisc

arch/parisc/Kconfig | 5 ++
arch/parisc/kernel/module.c | 4 +
arch/parisc/kernel/traps.c | 79 ++++++++++++++++-----------------------
arch/parisc/kernel/vmlinux.lds.S | 2
include/asm-parisc/bug.h | 38 ++++++++++++++++--
5 files changed, 76 insertions(+), 52 deletions(-)

index d210123..a946776 100644
@@ -29,6 +29,11 @@ config GENERIC_FIND_NEXT_BIT
 	bool
 	default y
 
+config GENERIC_BUG
+	bool
+	default y
+	depends on BUG
+
 config GENERIC_HWEIGHT
 	bool
 	default y

index 47ea4e4..1808f85 100644
@@ -46,6 +46,7 @@
 #include <linux/fs.h>
 #include <linux/string.h>
 #include <linux/kernel.h>
+#include <linux/bug.h>
 
 #include <asm/unwind.h>
 

@@ -851,10 +852,11 @@ int module_finalize(const Elf_Ehdr *hdr,
 	nsyms = newptr - (Elf_Sym *)symhdr->sh_addr;
 	DEBUGP("NEW num_symtab %lu\n", nsyms);
 	symhdr->sh_size = nsyms * sizeof(Elf_Sym);
-	return 0;
+	return module_bug_finalize(hdr, sechdrs, me);
 }
 
 void module_arch_cleanup(struct module *mod)
 {
 	deregister_unwind_table(mod);
+	module_bug_cleanup(mod);
 }

index 7b943b4..7b149df 100644
@@ -68,6 +68,8 @@ SECTIONS
 
   RODATA
 
+  BUG_TABLE
+
   /* writeable */
   . = ALIGN(ASM_PAGE_SIZE);	/* Make sure this is page aligned so
   				   that we can properly leave these

index 695588d..367e458 100644
@@ -1,14 +1,42 @@
 #ifndef _PARISC_BUG_H
 #define _PARISC_BUG_H
 
+/*
+ * Tell the user there is some problem.
+ * The offending file and line are encoded in the __bug_table section.
+ */
+
 #ifdef CONFIG_BUG
 #define HAVE_ARCH_BUG
-#define BUG() do { \
-	printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__); \
-	dump_stack(); \
-	panic("BUG!"); \
-} while (0)
+
+/* the break instruction is used as BUG() marker.  */
+#define	PARISC_BUG_BREAK_ASM	"break 0x1f, 0x1fff"
+#define	PARISC_BUG_BREAK_INSN	0x03ffe01f  /* PARISC_BUG_BREAK_ASM */
+
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+#define BUG()								\
+	do {								\
+		asm volatile("\n"					\
+			     "1:\t" PARISC_BUG_BREAK_ASM "\n"		\
+			     "\t.pushsection __bug_table,\"a\"\n"	\
+			     "2:\t.long 1b, %c0\n"			\
+			     "\t.short %c1, 0\n"			\
+			     "\t.org 2b+%c2\n"				\
+			     "\t.popsection"				\
+			     : : "i" (__FILE__), "i" (__LINE__),	\
+			     "i" (sizeof(struct bug_entry)) );		\
+		for(;;) ;						\
+	} while(0)
+
+#else
+#define BUG()								\
+	do {								\
+		asm volatile(PARISC_BUG_BREAK_ASM : : );		\
+		for(;;) ;						\
+	} while(0)
+#endif
 #endif
 
 #include <asm-generic/bug.h>
 #endif
+

_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux