[patch 0/5] enhance WARN_ON series

[patch 0/5] enhance WARN_ON series

[Arjan van de Ven]

3rd try for this patch series; now with a split up patch for __WARN_ON

This series has the goal of extending the usefulness of the WARN_ON() information,
at least on architectures that use the generic WARN_ON() infrastructure. (Those who
do their own thing either already have the extra information, or could consider
switching to the generic code). In order to do that, WARN_ON() first needs to 
be uninlined since there's like 1200 callsites and adding code to each of those
isn't pretty.

As part of this, I had to split the __WARN_ON patch in -mm into 2 pieces, one to 
introduce __WARN_ON, and a separate one to do the ifdef cleanup. 

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[patch 1/5] Introduce __WARN()

[Arjan van de Ven]

From: Olof Johansson <olof@lixom.net>

Introduce __WARN() in the generic case, so the generic WARN_ON()
can use arch-specific code for when the condition is true.

Signed-off-by: Olof Johansson <olof@lixom.net>
Cc: <linux-arch@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/asm-generic/bug.h |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Index: linux-2.6.24-rc6/include/asm-generic/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-generic/bug.h
+++ linux-2.6.24-rc6/include/asm-generic/bug.h
@@ -31,14 +31,19 @@ struct bug_entry {
 #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
 #endif
 
-#ifndef HAVE_ARCH_WARN_ON
+#ifndef __WARN
+#define __WARN() do {							\
+	printk("WARNING: at %s:%d %s()\n", __FILE__,			\
+		__LINE__, __FUNCTION__);				\
+	dump_stack();							\
+} while (0)
+#endif
+
+#ifndef WARN_ON
 #define WARN_ON(condition) ({						\
 	int __ret_warn_on = !!(condition);				\
-	if (unlikely(__ret_warn_on)) {					\
-		printk("WARNING: at %s:%d %s()\n", __FILE__,		\
-			__LINE__, __FUNCTION__);			\
-		dump_stack();						\
-	}								\
+	if (unlikely(__ret_warn_on))					\
+		__WARN();						\
 	unlikely(__ret_warn_on);					\
 })
 #endif

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[patch 2/5] move WARN_ON() out of line

[Arjan van de Ven]

Subject: move WARN_ON() out of line
From: Arjan van de Ven <arjan@linux.intel.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Olof Johansson <olof@lixom.net>
Acked-by: Matt Meckall <mpm@selenic.com>

A quick grep shows that there are currently 1145 instances of WARN_ON
in the kernel. Currently, WARN_ON is pretty much entirely inlined,
which makes it hard to enhance it without growing the size of the kernel
(and getting Andrew unhappy).

This patch build on top of Olof's patch that introduces __WARN,
and places the slowpath out of line. It also uses Ingo's suggestion
to not use __FUNCTION__ but to use kallsyms to do the lookup;
this saves a ton of extra space since gcc doesn't need to store the function
string twice now:

3936367  833603  624736 5394706  525112 vmlinux.before
3917508  833603  624736 5375847  520767 vmlinux-slowpath

15Kb savings...

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>


---
 include/asm-generic/bug.h |   10 +++++-----
 kernel/panic.c            |   15 +++++++++++++++
 2 files changed, 20 insertions(+), 5 deletions(-)

Index: linux-2.6.24-rc6/include/asm-generic/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-generic/bug.h
+++ linux-2.6.24-rc6/include/asm-generic/bug.h
@@ -32,11 +32,11 @@ struct bug_entry {
 #endif
 
 #ifndef __WARN
-#define __WARN() do {							\
-	printk("WARNING: at %s:%d %s()\n", __FILE__,			\
-		__LINE__, __FUNCTION__);				\
-	dump_stack();							\
-} while (0)
+#ifndef __ASSEMBLY__
+extern void warn_on_slowpath(const char *file, const int line);
+#define WANT_WARN_ON_SLOWPATH
+#endif
+#define __WARN() warn_on_slowpath(__FILE__, __LINE__)
 #endif
 
 #ifndef WARN_ON
Index: linux-2.6.24-rc6/kernel/panic.c
===================================================================
--- linux-2.6.24-rc6.orig/kernel/panic.c
+++ linux-2.6.24-rc6/kernel/panic.c
@@ -20,6 +20,7 @@
 #include <linux/kexec.h>
 #include <linux/debug_locks.h>
 #include <linux/random.h>
+#include <linux/kallsyms.h>
 
 int panic_on_oops;
 int tainted;
@@ -292,6 +293,20 @@ void oops_exit(void)
 		(unsigned long long)oops_id);
 }
 
+#ifdef WANT_WARN_ON_SLOWPATH
+void warn_on_slowpath(const char *file, int line)
+{
+	char function[KSYM_SYMBOL_LEN];
+	unsigned long caller = (unsigned long) __builtin_return_address(0);
+
+	sprint_symbol(function, caller);
+	printk(KERN_WARNING "WARNING: at %s:%d %s()\n", file,
+		line, function);
+	dump_stack();
+}
+EXPORT_SYMBOL(warn_on_slowpath);
+#endif
+
 #ifdef CONFIG_CC_STACKPROTECTOR
 /*
  * Called when gcc's -fstack-protector feature is used, and


--
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[patch 3/5] Add the end-of-trace marker and the module list to WARN_ON()

[Arjan van de Ven]

Subject: Add the end-of-trace marker and the module list to WARN_ON()
From: Arjan van de Ven <arjan@linux.intel.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Olof Johansson <olof@lixom.net>

Unlike oopses, WARN_ON() currently does't print the loaded modules list.
This makes it harder to take action on certain bug reports. For example,
recently there were a set of WARN_ON()s reported in the mac80211 stack,
which were just signalling a driver bug. It takes then anther round trip
to the bug reporter (if he responds at all) to find out which driver
is at fault.

Another issue is that, unlike oopses, WARN_ON() doesn't currently printk
the helpful "cut here" line, nor the "end of trace" marker.
Now that WARN_ON() is out of line, the size increase due to this is
minimal and it's worth adding.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

---
 kernel/panic.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Index: linux-2.6.24-rc6/kernel/panic.c
===================================================================
--- linux-2.6.24-rc6.orig/kernel/panic.c
+++ linux-2.6.24-rc6/kernel/panic.c
@@ -281,6 +281,13 @@ static int init_oops_id(void)
 }
 late_initcall(init_oops_id);
 
+static void print_oops_end_marker(void)
+{
+	init_oops_id();
+	printk(KERN_WARNING "---[ end trace %016llx ]---\n",
+		(unsigned long long)oops_id);
+}
+
 /*
  * Called when the architecture exits its oops handler, after printing
  * everything.
@@ -288,9 +295,7 @@ late_initcall(init_oops_id);
 void oops_exit(void)
 {
 	do_oops_enter_exit();
-	init_oops_id();
-	printk(KERN_WARNING "---[ end trace %016llx ]---\n",
-		(unsigned long long)oops_id);
+	print_oops_end_marker();
 }
 
 #ifdef WANT_WARNON_SLOWPATH
@@ -298,11 +303,14 @@ void warn_on_slowpath(const char *file, 
 {
 	char function[KSYM_SYMBOL_LEN];
 	unsigned long caller = (unsigned long) __builtin_return_address(0);
-
 	sprint_symbol(function, caller);
+
+	printk(KERN_WARNING "------------[ cut here ]------------\n");
 	printk(KERN_WARNING "WARNING: at %s:%d %s()\n", file,
 		line, function);
+	print_modules();
 	dump_stack();
+	print_oops_end_marker();
 }
 EXPORT_SYMBOL(warn_on_slowpath);
 #endif



-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[patch 4/5] bugh-remove-have_arch_bug--have_arch_warn

[Arjan van de Ven]

From: Olof Johansson <olof@lixom.net>

No need to have the HAVE_ARCH_BUG.* / HAVE_ARCH_WARN.* defines, when
the generic implementation can just use #ifndef on the macros themselves.

Signed-off-by: Olof Johansson <olof@lixom.net>
Cc: <linux-arch@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/asm-alpha/bug.h   |    1 -
 include/asm-arm/bug.h     |    1 -
 include/asm-avr32/bug.h   |    3 ---
 include/asm-frv/bug.h     |    1 -
 include/asm-generic/bug.h |   10 +++++-----
 include/asm-ia64/bug.h    |    1 -
 include/asm-m68k/bug.h    |    1 -
 include/asm-mips/bug.h    |    4 ----
 include/asm-parisc/bug.h  |    2 --
 include/asm-powerpc/bug.h |    3 ---
 include/asm-s390/bug.h    |    2 --
 include/asm-sparc/bug.h   |    1 -
 include/asm-sparc64/bug.h |    1 -
 include/asm-v850/bug.h    |    1 -
 include/asm-x86/bug.h     |    1 -
 15 files changed, 5 insertions(+), 28 deletions(-)

Index: linux-2.6.24-rc6/include/asm-alpha/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-alpha/bug.h
+++ linux-2.6.24-rc6/include/asm-alpha/bug.h
@@ -10,7 +10,6 @@
   __asm__ __volatile__("call_pal %0  # bugchk\n\t"".long %1\n\t.8byte %2" \
 		       : : "i" (PAL_bugchk), "i"(__LINE__), "i"(__FILE__))
 
-#define HAVE_ARCH_BUG
 #endif
 
 #include <asm-generic/bug.h>
Index: linux-2.6.24-rc6/include/asm-arm/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-arm/bug.h
+++ linux-2.6.24-rc6/include/asm-arm/bug.h
@@ -16,7 +16,6 @@ extern void __bug(const char *file, int 
 
 #endif
 
-#define HAVE_ARCH_BUG
 #endif
 
 #include <asm-generic/bug.h>
Index: linux-2.6.24-rc6/include/asm-avr32/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-avr32/bug.h
+++ linux-2.6.24-rc6/include/asm-avr32/bug.h
@@ -63,9 +63,6 @@
 		unlikely(__ret_warn_on);				\
 	})
 
-#define HAVE_ARCH_BUG
-#define HAVE_ARCH_WARN_ON
-
 #endif /* CONFIG_BUG */
 
 #include <asm-generic/bug.h>
Index: linux-2.6.24-rc6/include/asm-frv/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-frv/bug.h
+++ linux-2.6.24-rc6/include/asm-frv/bug.h
@@ -32,7 +32,6 @@ do {						\
 	asm volatile("nop");			\
 } while(0)
 
-#define HAVE_ARCH_BUG
 #define BUG()					\
 do {						\
 	_debug_bug_printk();			\
Index: linux-2.6.24-rc6/include/asm-generic/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-generic/bug.h
+++ linux-2.6.24-rc6/include/asm-generic/bug.h
@@ -20,14 +20,14 @@ struct bug_entry {
 #define BUGFLAG_WARNING	(1<<0)
 #endif	/* CONFIG_GENERIC_BUG */
 
-#ifndef HAVE_ARCH_BUG
+#ifndef BUG
 #define BUG() do { \
 	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __FUNCTION__); \
 	panic("BUG!"); \
 } while (0)
 #endif
 
-#ifndef HAVE_ARCH_BUG_ON
+#ifndef BUG_ON
 #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
 #endif
 
@@ -49,15 +49,15 @@ extern void warn_on_slowpath(const char 
 #endif
 
 #else /* !CONFIG_BUG */
-#ifndef HAVE_ARCH_BUG
+#ifndef BUG
 #define BUG()
 #endif
 
-#ifndef HAVE_ARCH_BUG_ON
+#ifndef BUG_ON
 #define BUG_ON(condition) do { if (condition) ; } while(0)
 #endif
 
-#ifndef HAVE_ARCH_WARN_ON
+#ifndef WARN_ON
 #define WARN_ON(condition) ({						\
 	int __ret_warn_on = !!(condition);				\
 	unlikely(__ret_warn_on);					\
Index: linux-2.6.24-rc6/include/asm-ia64/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-ia64/bug.h
+++ linux-2.6.24-rc6/include/asm-ia64/bug.h
@@ -6,7 +6,6 @@
 #define BUG() do { printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__); ia64_abort(); } while (0)
 
 /* should this BUG be made generic? */
-#define HAVE_ARCH_BUG
 #endif
 
 #include <asm-generic/bug.h>
Index: linux-2.6.24-rc6/include/asm-m68k/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-m68k/bug.h
+++ linux-2.6.24-rc6/include/asm-m68k/bug.h
@@ -21,7 +21,6 @@
 } while (0)
 #endif
 
-#define HAVE_ARCH_BUG
 #endif
 
 #include <asm-generic/bug.h>
Index: linux-2.6.24-rc6/include/asm-mips/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-mips/bug.h
+++ linux-2.6.24-rc6/include/asm-mips/bug.h
@@ -12,8 +12,6 @@ do {									\
 	__asm__ __volatile__("break %0" : : "i" (BRK_BUG));		\
 } while (0)
 
-#define HAVE_ARCH_BUG
-
 #if (_MIPS_ISA > _MIPS_ISA_MIPS1)
 
 #define BUG_ON(condition)						\
@@ -22,8 +20,6 @@ do {									\
 			     : : "r" (condition), "i" (BRK_BUG));	\
 } while (0)
 
-#define HAVE_ARCH_BUG_ON
-
 #endif /* _MIPS_ISA > _MIPS_ISA_MIPS1 */
 
 #endif
Index: linux-2.6.24-rc6/include/asm-parisc/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-parisc/bug.h
+++ linux-2.6.24-rc6/include/asm-parisc/bug.h
@@ -7,8 +7,6 @@
  */
 
 #ifdef CONFIG_BUG
-#define HAVE_ARCH_BUG
-#define HAVE_ARCH_WARN_ON
 
 /* the break instruction is used as BUG() marker.  */
 #define	PARISC_BUG_BREAK_ASM	"break 0x1f, 0x1fff"
Index: linux-2.6.24-rc6/include/asm-powerpc/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-powerpc/bug.h
+++ linux-2.6.24-rc6/include/asm-powerpc/bug.h
@@ -109,9 +109,6 @@
 	unlikely(__ret_warn_on);				\
 })
 
-#define HAVE_ARCH_BUG
-#define HAVE_ARCH_BUG_ON
-#define HAVE_ARCH_WARN_ON
 #endif /* __ASSEMBLY __ */
 #endif /* CONFIG_BUG */
 
Index: linux-2.6.24-rc6/include/asm-s390/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-s390/bug.h
+++ linux-2.6.24-rc6/include/asm-s390/bug.h
@@ -61,8 +61,6 @@
 	unlikely(__ret_warn_on);			\
 })
 
-#define HAVE_ARCH_BUG
-#define HAVE_ARCH_WARN_ON
 #endif /* CONFIG_BUG */
 
 #include <asm-generic/bug.h>
Index: linux-2.6.24-rc6/include/asm-sparc/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-sparc/bug.h
+++ linux-2.6.24-rc6/include/asm-sparc/bug.h
@@ -26,7 +26,6 @@ extern void do_BUG(const char *file, int
 #define BUG()		__bug_trap()
 #endif
 
-#define HAVE_ARCH_BUG
 #endif
 
 #include <asm-generic/bug.h>
Index: linux-2.6.24-rc6/include/asm-sparc64/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-sparc64/bug.h
+++ linux-2.6.24-rc6/include/asm-sparc64/bug.h
@@ -14,7 +14,6 @@ extern void do_BUG(const char *file, int
 #define BUG()		__builtin_trap()
 #endif
 
-#define HAVE_ARCH_BUG
 #endif
 
 #include <asm-generic/bug.h>
Index: linux-2.6.24-rc6/include/asm-v850/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-v850/bug.h
+++ linux-2.6.24-rc6/include/asm-v850/bug.h
@@ -17,7 +17,6 @@
 #ifdef CONFIG_BUG
 extern void __bug (void) __attribute__ ((noreturn));
 #define BUG()		__bug()
-#define HAVE_ARCH_BUG
 #endif
 
 #include <asm-generic/bug.h>
Index: linux-2.6.24-rc6/include/asm-x86/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-x86/bug.h
+++ linux-2.6.24-rc6/include/asm-x86/bug.h
@@ -2,7 +2,6 @@
 #define _ASM_X86_BUG_H
 
 #ifdef CONFIG_BUG
-#define HAVE_ARCH_BUG
 
 #ifdef CONFIG_DEBUG_BUGVERBOSE
 


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[patch 5/5] PowerPC: switch to generic WARN_ON / BUG_ON

[Arjan van de Ven]

From: Olof Johansson <olof@lixom.net>

Not using the ppc-specific WARN_ON/BUG_ON constructs actually saves about
4K text on a ppc64_defconfig.  The main reason seems to be that prepping
the arguments to the conditional trap instructions is more work than just
doing a compare and branch.

Signed-off-by: Olof Johansson <olof@lixom.net>
Cc: <linux-arch@vger.kernel.org>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>, 
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/asm-powerpc/bug.h |   37 -------------------------------------
 1 file changed, 37 deletions(-)

Index: linux-2.6.24-rc6/include/asm-powerpc/bug.h
===================================================================
--- linux-2.6.24-rc6.orig/include/asm-powerpc/bug.h
+++ linux-2.6.24-rc6/include/asm-powerpc/bug.h
@@ -54,12 +54,6 @@
 	".previous\n"
 #endif
 
-/*
- * BUG_ON() and WARN_ON() do their best to cooperate with compile-time
- * optimisations. However depending on the complexity of the condition
- * some compiler versions may not produce optimal results.
- */
-
 #define BUG() do {						\
 	__asm__ __volatile__(					\
 		"1:	twi 31,0,0\n"				\
@@ -69,20 +63,6 @@
 	for(;;) ;						\
 } while (0)
 
-#define BUG_ON(x) do {						\
-	if (__builtin_constant_p(x)) {				\
-		if (x)						\
-			BUG();					\
-	} else {						\
-		__asm__ __volatile__(				\
-		"1:	"PPC_TLNEI"	%4,0\n"			\
-		_EMIT_BUG_ENTRY					\
-		: : "i" (__FILE__), "i" (__LINE__), "i" (0),	\
-		  "i" (sizeof(struct bug_entry)),		\
-		  "r" ((__force long)(x)));			\
-	}							\
-} while (0)
-
 #define __WARN() do {						\
 	__asm__ __volatile__(					\
 		"1:	twi 31,0,0\n"				\
@@ -92,23 +72,6 @@
 		  "i" (sizeof(struct bug_entry)));		\
 } while (0)
 
-#define WARN_ON(x) ({						\
-	int __ret_warn_on = !!(x);				\
-	if (__builtin_constant_p(__ret_warn_on)) {		\
-		if (__ret_warn_on)				\
-			__WARN();				\
-	} else {						\
-		__asm__ __volatile__(				\
-		"1:	"PPC_TLNEI"	%4,0\n"			\
-		_EMIT_BUG_ENTRY					\
-		: : "i" (__FILE__), "i" (__LINE__),		\
-		  "i" (BUGFLAG_WARNING),			\
-		  "i" (sizeof(struct bug_entry)),		\
-		  "r" (__ret_warn_on));				\
-	}							\
-	unlikely(__ret_warn_on);				\
-})
-
 #endif /* __ASSEMBLY __ */
 #endif /* CONFIG_BUG */
 


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch 0/5] enhance WARN_ON series

[Ingo Molnar]

* Arjan van de Ven <arjan@infradead.org> wrote:

> 3rd try for this patch series; now with a split up patch for __WARN_ON
> 
> This series has the goal of extending the usefulness of the WARN_ON() 
> information, at least on architectures that use the generic WARN_ON() 
> infrastructure. (Those who do their own thing either already have the 
> extra information, or could consider switching to the generic code). 
> In order to do that, WARN_ON() first needs to be uninlined since 
> there's like 1200 callsites and adding code to each of those isn't 
> pretty.
> 
> As part of this, I had to split the __WARN_ON patch in -mm into 2 
> pieces, one to introduce __WARN_ON, and a separate one to do the ifdef 
> cleanup.

i've added the first 3 patches to x86.git, for further testing - and 
would expect -mm to pick up #4/#5.

	Ingo

Re: [patch 3/5] Add the end-of-trace marker and the module list to WARN_ON()

[David Woodhouse]

On Sat, 2008-01-05 at 19:10 -0800, Arjan van de Ven wrote:
> Another issue is that, unlike oopses, WARN_ON() doesn't currently
> printk the helpful "cut here" line, 

I'd rather see the 'cut here' line disappear altogether. Often, the
line(s) which come immediately before it are extremely relevant to the
problem. Cutting them is bad.

-- 
dwmw2

Re: [patch 5/5] PowerPC: switch to generic WARN_ON / BUG_ON

[Benjamin Herrenschmidt]

On Sat, 2008-01-05 at 19:12 -0800, Arjan van de Ven wrote:
> From: Olof Johansson <olof@lixom.net>
> 
> Not using the ppc-specific WARN_ON/BUG_ON constructs actually saves about
> 4K text on a ppc64_defconfig.  The main reason seems to be that prepping
> the arguments to the conditional trap instructions is more work than just
> doing a compare and branch.

I'm a bit annoyed by that one ... for obvious reasons... I wish gcc
could be better here. Also, we can't completely remove the support for
the trap since we use that in asm in various places...

Ben.

> Signed-off-by: Olof Johansson <olof@lixom.net>
> Cc: <linux-arch@vger.kernel.org>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>, 
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  include/asm-powerpc/bug.h |   37 -------------------------------------
>  1 file changed, 37 deletions(-)
> 
> Index: linux-2.6.24-rc6/include/asm-powerpc/bug.h
> ===================================================================
> --- linux-2.6.24-rc6.orig/include/asm-powerpc/bug.h
> +++ linux-2.6.24-rc6/include/asm-powerpc/bug.h
> @@ -54,12 +54,6 @@
>  	".previous\n"
>  #endif
>  
> -/*
> - * BUG_ON() and WARN_ON() do their best to cooperate with compile-time
> - * optimisations. However depending on the complexity of the condition
> - * some compiler versions may not produce optimal results.
> - */
> -
>  #define BUG() do {						\
>  	__asm__ __volatile__(					\
>  		"1:	twi 31,0,0\n"				\
> @@ -69,20 +63,6 @@
>  	for(;;) ;						\
>  } while (0)
>  
> -#define BUG_ON(x) do {						\
> -	if (__builtin_constant_p(x)) {				\
> -		if (x)						\
> -			BUG();					\
> -	} else {						\
> -		__asm__ __volatile__(				\
> -		"1:	"PPC_TLNEI"	%4,0\n"			\
> -		_EMIT_BUG_ENTRY					\
> -		: : "i" (__FILE__), "i" (__LINE__), "i" (0),	\
> -		  "i" (sizeof(struct bug_entry)),		\
> -		  "r" ((__force long)(x)));			\
> -	}							\
> -} while (0)
> -
>  #define __WARN() do {						\
>  	__asm__ __volatile__(					\
>  		"1:	twi 31,0,0\n"				\
> @@ -92,23 +72,6 @@
>  		  "i" (sizeof(struct bug_entry)));		\
>  } while (0)
>  
> -#define WARN_ON(x) ({						\
> -	int __ret_warn_on = !!(x);				\
> -	if (__builtin_constant_p(__ret_warn_on)) {		\
> -		if (__ret_warn_on)				\
> -			__WARN();				\
> -	} else {						\
> -		__asm__ __volatile__(				\
> -		"1:	"PPC_TLNEI"	%4,0\n"			\
> -		_EMIT_BUG_ENTRY					\
> -		: : "i" (__FILE__), "i" (__LINE__),		\
> -		  "i" (BUGFLAG_WARNING),			\
> -		  "i" (sizeof(struct bug_entry)),		\
> -		  "r" (__ret_warn_on));				\
> -	}							\
> -	unlikely(__ret_warn_on);				\
> -})
> -
>  #endif /* __ASSEMBLY __ */
>  #endif /* CONFIG_BUG */
>  
> 
> 

Re: [patch 1/5] Introduce __WARN()

[Richard Knutsson]

Arjan van de Ven wrote:
> From: Olof Johansson <olof@lixom.net>
>
> Introduce __WARN() in the generic case, so the generic WARN_ON()
> can use arch-specific code for when the condition is true.
>
> Signed-off-by: Olof Johansson <olof@lixom.net>
> Cc: <linux-arch@vger.kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
>  include/asm-generic/bug.h |   17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> Index: linux-2.6.24-rc6/include/asm-generic/bug.h
> ===================================================================
> --- linux-2.6.24-rc6.orig/include/asm-generic/bug.h
> +++ linux-2.6.24-rc6/include/asm-generic/bug.h
> @@ -31,14 +31,19 @@ struct bug_entry {
>  #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
>  #endif
>  
> -#ifndef HAVE_ARCH_WARN_ON
> +#ifndef __WARN
> +#define __WARN() do {							\
> +	printk("WARNING: at %s:%d %s()\n", __FILE__,			\
> +		__LINE__, __FUNCTION__);				\
> +	dump_stack();							\
> +} while (0)
> +#endif
> +
> +#ifndef WARN_ON
>  #define WARN_ON(condition) ({						\
>  	int __ret_warn_on = !!(condition);				\
>   
What about using a boolean for __ret_warn_on, which then let us remove 
the '!!'?
(btw, wouldn't 'var != 0' actually be the proper semantic instead of 
playing with '!'s?)
> -	if (unlikely(__ret_warn_on)) {					\
> -		printk("WARNING: at %s:%d %s()\n", __FILE__,		\
> -			__LINE__, __FUNCTION__);			\
> -		dump_stack();						\
> -	}								\
> +	if (unlikely(__ret_warn_on))					\
> +		__WARN();						\
>  	unlikely(__ret_warn_on);					\
>  })
>  #endif
>
>   

Re: [patch 5/5] PowerPC: switch to generic WARN_ON / BUG_ON

[Olof Johansson]

On Sun, Jan 06, 2008 at 10:16:04PM +1100, Benjamin Herrenschmidt wrote:
> 
> On Sat, 2008-01-05 at 19:12 -0800, Arjan van de Ven wrote:
> > From: Olof Johansson <olof@lixom.net>
> > 
> > Not using the ppc-specific WARN_ON/BUG_ON constructs actually saves about
> > 4K text on a ppc64_defconfig.  The main reason seems to be that prepping
> > the arguments to the conditional trap instructions is more work than just
> > doing a compare and branch.
> 
> I'm a bit annoyed by that one ... for obvious reasons... I wish gcc
> could be better here. Also, we can't completely remove the support for
> the trap since we use that in asm in various places...

The support isn't removed, and the trap is still used. This patch has
been posted a bunch of times before and been in -mm for quite a while.


-Olof

Re: [patch 1/5] Introduce __WARN()

[Arjan van de Ven]

On Sun, 06 Jan 2008 12:44:56 +0100
Richard Knutsson <ricknu-0@student.ltu.se> wrote:

> Arjan van de Ven wrote:
> > From: Olof Johansson <olof@lixom.net>
> >
> > Introduce __WARN() in the generic case, so the generic WARN_ON()
> > can use arch-specific code for when the condition is true.
> >
> > Signed-off-by: Olof Johansson <olof@lixom.net>
> > Cc: <linux-arch@vger.kernel.org>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >
> >  include/asm-generic/bug.h |   17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > Index: linux-2.6.24-rc6/include/asm-generic/bug.h
> > ===================================================================
> > --- linux-2.6.24-rc6.orig/include/asm-generic/bug.h
> > +++ linux-2.6.24-rc6/include/asm-generic/bug.h
> > @@ -31,14 +31,19 @@ struct bug_entry {
> >  #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); }
> > while(0) #endif
> >  
> > -#ifndef HAVE_ARCH_WARN_ON
> > +#ifndef __WARN
> > +#define __WARN() do
> > {							\
> > +	printk("WARNING: at %s:%d %s()\n",
> > __FILE__,			\
> > +		__LINE__,
> > __FUNCTION__);				\
> > +
> > dump_stack();
> > \ +} while (0) +#endif
> > +
> > +#ifndef WARN_ON
> >  #define WARN_ON(condition)
> > ({						\ int
> > __ret_warn_on = !!(condition);				\ 

> What about using a boolean for __ret_warn_on, which then let us
> remove the '!!'?

is iffy.. like what happens if an u64 is cast to a boolean?
No matter what the final assembly code will need to be the same

> (btw, wouldn't 'var != 0' actually be the proper semantic instead of 
> playing with '!'s?)

no because var could be a pointer for example...

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch 1/5] Introduce __WARN()

[Richard Knutsson]

Arjan van de Ven wrote:
> On Sun, 06 Jan 2008 12:44:56 +0100
> Richard Knutsson <ricknu-0@student.ltu.se> wrote:
>
>   
>> Arjan van de Ven wrote:
>>     
>>> From: Olof Johansson <olof@lixom.net>
>>>
>>> Introduce __WARN() in the generic case, so the generic WARN_ON()
>>> can use arch-specific code for when the condition is true.
>>>
>>> Signed-off-by: Olof Johansson <olof@lixom.net>
>>> Cc: <linux-arch@vger.kernel.org>
>>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>>> ---
>>>
>>>  include/asm-generic/bug.h |   17 +++++++++++------
>>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>> Index: linux-2.6.24-rc6/include/asm-generic/bug.h
>>> ===================================================================
>>> --- linux-2.6.24-rc6.orig/include/asm-generic/bug.h
>>> +++ linux-2.6.24-rc6/include/asm-generic/bug.h
>>> @@ -31,14 +31,19 @@ struct bug_entry {
>>>  #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); }
>>> while(0) #endif
>>>  
>>> -#ifndef HAVE_ARCH_WARN_ON
>>> +#ifndef __WARN
>>> +#define __WARN() do
>>> {							\
>>> +	printk("WARNING: at %s:%d %s()\n",
>>> __FILE__,			\
>>> +		__LINE__,
>>> __FUNCTION__);				\
>>> +
>>> dump_stack();
>>> \ +} while (0) +#endif
>>> +
>>> +#ifndef WARN_ON
>>>  #define WARN_ON(condition)
>>> ({						\ int
>>> __ret_warn_on = !!(condition);				\ 
>>>       
>
>   
>> What about using a boolean for __ret_warn_on, which then let us
>> remove the '!!'?
>>     
>
> is iffy.. like what happens if an u64 is cast to a boolean?
> No matter what the final assembly code will need to be the same
>   
Well, the main point were to use the boolean type instead of an integer...
What about u64? "true" is still "not zero", and is really the assembly 
the same for !!u8, !!u64 and !!pointer? Isn't that the reason to use a 
macro instead of a function?
(If you really mean "what happens?": any 'bool b = some_var;' will set 
'b' according to the C-idiom "if zero: 'false', otherwise 'true'".)
>   
>> (btw, wouldn't 'var != 0' actually be the proper semantic instead of 
>> playing with '!'s?)
>>     
>
> no because var could be a pointer for example...
>   
You mean because in that case it would be '!= NULL', do you? Sorry, do 
not see your point here.

regards,
Richard Knutsson

Re: [patch 1/5] Introduce __WARN()

[Arjan van de Ven]

On Sun, 06 Jan 2008 17:09:44 +0100
Richard Knutsson <ricknu-0@student.ltu.se> wrote:

> >> (btw, wouldn't 'var != 0' actually be the proper semantic instead
> >> of playing with '!'s?)
> >>     
> >
> > no because var could be a pointer for example...
> >   
> You mean because in that case it would be '!= NULL', do you? Sorry,
> do not see your point here.

my point is that you don't know which one to use.. 
But this isn't new discussion (nor something I'm changing at all); this has come
up since way back in 2005 :)
If you feel strongly of changing this, feel free to post a patch; for now I much
rather leave things as they are right now.


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch 1/5] Introduce __WARN()

[Richard Knutsson]

Arjan van de Ven wrote:
> On Sun, 06 Jan 2008 17:09:44 +0100
> Richard Knutsson <ricknu-0@student.ltu.se> wrote:
>
>   
>>>> (btw, wouldn't 'var != 0' actually be the proper semantic instead
>>>> of playing with '!'s?)
>>>>     
>>>>         
>>> no because var could be a pointer for example...
>>>   
>>>       
>> You mean because in that case it would be '!= NULL', do you? Sorry,
>> do not see your point here.
>>     
>
> my point is that you don't know which one to use.. 
>   
Sorry to be a bother, but why is that relevant? Except semantics, they 
are the same, right? So what problem would it be if you send it a 
pointer? The '!' uses the same "argument/reason" when given a pointer ;).
> But this isn't new discussion (nor something I'm changing at all); this has come
> up since way back in 2005 :)
> If you feel strongly of changing this, feel free to post a patch; for now I much
> rather leave things as they are right now.
>   
Oh o-well, in such case I may come back and do a larger patching 
someday. Just though since you were in the neighborhood...

Have a good evening
Richard Knutsson

Re: [patch 2/5] move WARN_ON() out of line

[Olof Johansson]

On Sat, Jan 05, 2008 at 07:09:59PM -0800, Arjan van de Ven wrote:
> From: Arjan van de Ven <arjan@linux.intel.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Olof Johansson <olof@lixom.net>
> Acked-by: Matt Meckall <mpm@selenic.com>
> 
> A quick grep shows that there are currently 1145 instances of WARN_ON
> in the kernel. Currently, WARN_ON is pretty much entirely inlined,
> which makes it hard to enhance it without growing the size of the kernel
> (and getting Andrew unhappy).
> 
> This patch build on top of Olof's patch that introduces __WARN,
> and places the slowpath out of line. It also uses Ingo's suggestion
> to not use __FUNCTION__ but to use kallsyms to do the lookup;
> this saves a ton of extra space since gcc doesn't need to store the function
> string twice now:
> 
> 3936367  833603  624736 5394706  525112 vmlinux.before
> 3917508  833603  624736 5375847  520767 vmlinux-slowpath
> 
> 15Kb savings...
> 
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

Acked-by: Olof Johansson <olof@lixom.net>

[PATCH] Add bug/warn marker to generic report_bug()

[Olof Johansson]

Powerpc uses the generic report_bug() from lib/bug.c to report warnings,
and I'm guessing other arches do as well.

Add the module list as well as the end-of-trace marker to the output. This
required making print_oops_end_marker() nonstatic.


Signed-off-by: Olof Johansson <olof@lixom.net>


---

On Sat, Jan 05, 2008 at 07:07:13PM -0800, Arjan van de Ven wrote:
> 3rd try for this patch series; now with a split up patch for __WARN_ON
> 
> This series has the goal of extending the usefulness of the WARN_ON() information,
> at least on architectures that use the generic WARN_ON() infrastructure. (Those who
> do their own thing either already have the extra information, or could consider
> switching to the generic code). In order to do that, WARN_ON() first needs to 
> be uninlined since there's like 1200 callsites and adding code to each of those
> isn't pretty.
> 
> As part of this, I had to split the __WARN_ON patch in -mm into 2 pieces, one to 
> introduce __WARN_ON, and a separate one to do the ifdef cleanup. 

Looks good. The following patch takes care of the warning printout from
powerpc as well. Unfortunately I had to non-staticfy
print_oops_end_marker().


-Olof

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 94bc996..88d1aa3 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -133,6 +133,7 @@ NORET_TYPE void panic(const char * fmt, ...)
 extern void oops_enter(void);
 extern void oops_exit(void);
 extern int oops_may_print(void);
+extern void print_oops_end_marker(void);
 fastcall NORET_TYPE void do_exit(long error_code)
 	ATTRIB_NORET;
 NORET_TYPE void complete_and_exit(struct completion *, long)
diff --git a/kernel/panic.c b/kernel/panic.c
index d9e90cf..0269a7f 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -281,7 +281,7 @@ static int init_oops_id(void)
 }
 late_initcall(init_oops_id);
 
-static void print_oops_end_marker(void)
+void print_oops_end_marker(void)
 {
 	init_oops_id();
 	printk(KERN_WARNING "---[ end trace %016llx ]---\n",
diff --git a/lib/bug.c b/lib/bug.c
index 530f38f..3aa60a5 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -148,7 +148,9 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
 			       "[verbose debug info unavailable]\n",
 			       (void *)bugaddr);
 
+		print_modules();
 		show_regs(regs);
+		print_oops_end_marker();
 		return BUG_TRAP_TYPE_WARN;
 	}
 

Re: [PATCH] Add bug/warn marker to generic report_bug()

[Arjan van de Ven]

On Sun, 6 Jan 2008 14:22:23 -0600
Olof Johansson <olof@lixom.net> wrote:

> Powerpc uses the generic report_bug() from lib/bug.c to report
> warnings, and I'm guessing other arches do as well.
> 
> Add the module list as well as the end-of-trace marker to the output.
> This required making print_oops_end_marker() nonstatic.
> 
> 

this is the wrong approach...
powerpc and such should just use oops_enter() / oops_exit() to signal the start/end of such 
a trace, that gives them all the other behavior of oopsing as well (such as the "slow oops scrolling down" etc)

Re: [PATCH] Add bug/warn marker to generic report_bug()

[Olof Johansson]

On Sun, Jan 06, 2008 at 01:38:17PM -0800, Arjan van de Ven wrote:
> On Sun, 6 Jan 2008 14:22:23 -0600
> Olof Johansson <olof@lixom.net> wrote:
> 
> > Powerpc uses the generic report_bug() from lib/bug.c to report
> > warnings, and I'm guessing other arches do as well.
> > 
> > Add the module list as well as the end-of-trace marker to the output.
> > This required making print_oops_end_marker() nonstatic.
> > 
> > 
> 
> this is the wrong approach...
> powerpc and such should just use oops_enter() / oops_exit() to signal the start/end of such 
> a trace, that gives them all the other behavior of oopsing as well (such as the "slow oops scrolling down" etc)

Note that this is for warnings, not oopses.

This comment in oops_enter threw me off of using it:

        debug_locks_off(); /* can't trust the integrity of the kernel
	anymore */

Since we can very well depend on the integrity of the kernel when it's
just doing a __WARN().

do_warn_slowpath() doesn't use oops_enter() either.


-Olof

Re: [PATCH] Add bug/warn marker to generic report_bug()

[Arjan van de Ven]

On Sun, 6 Jan 2008 19:22:37 -0600
Olof Johansson <olof@lixom.net> wrote:


> This comment in oops_enter threw me off of using it:
> 
>         debug_locks_off(); /* can't trust the integrity of the kernel
> 	anymore */
> 
> Since we can very well depend on the integrity of the kernel when it's
> just doing a __WARN().

ok fair enough; objection withdrawn

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch 3/5] Add the end-of-trace marker and the module list to WARN_ON()

[Valdis.Kletnieks]

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

On Sun, 06 Jan 2008 10:04:06 GMT, David Woodhouse said:
> 
> On Sat, 2008-01-05 at 19:10 -0800, Arjan van de Ven wrote:
> > Another issue is that, unlike oopses, WARN_ON() doesn't currently
> > printk the helpful "cut here" line, 
> 
> I'd rather see the 'cut here' line disappear altogether. Often, the
> line(s) which come immediately before it are extremely relevant to the
> problem. Cutting them is bad.

How about "--- cut 10 lines above this line ---"?

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]