enhance WARN_ON series
Subsystems (2)
cover enhance WARN_ON series
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.
Signature · 3 lines
If you want to reach me at my work email, use [email protected] For development, discussion and tips for power savings, visit http://www.lesswatts.org
PATCH 1/5 Introduce __WARN()
From: Olof Johansson <[email protected]>
Introduce __WARN() in the generic case, so the generic WARN_ON() can use arch-specific code for when the condition is true.
---
include/asm-generic/bug.h | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
include/asm-generic/bug.h +11 -6
@@ -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
Signature · 3 lines
If you want to reach me at my work email, use [email protected] For development, discussion and tips for power savings, visit http://www.lesswatts.org
Re: [patch 1/5] Introduce __WARN()
Quoted 24 lines · Arjan van de Ven
From: Olof Johansson <[email protected]>
Introduce
__WARN()in the generic case, so the genericWARN_ON()can use arch-specific code for when the condition is true.Signed-off-by Olof Johansson <[email protected]>Cc <[email protected]>Signed-off-by Andrew Morton <[email protected]>---
include/asm-generic/bug.h | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)Index:
linux-2.6.24-rc6/include/asm-generic/bug.h
===================================================================include/asm-generic/bug.h +2
-1@@ -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 { \
include/asm-generic/bug.h struct bug_entry { · line 45
Quoted 10 lines
include/asm-generic/bug.h +7
-0+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?)
Quoted 12 lines
include/asm-generic/bug.h +2
-5-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 1/5] Introduce __WARN()
Quoted 33 lines · On Sun, 06 Jan 2008 12:44:56 +0100 Richard Knutsson <[email protected]>…
Quoted 32 lines · Arjan van de Ven
From: Olof Johansson <[email protected]>
Introduce
__WARN()in the generic case, so the genericWARN_ON()can use arch-specific code for when the condition is true.Signed-off-by Olof Johansson <[email protected]>Cc <[email protected]>Signed-off-by Andrew Morton <[email protected]>---
include/asm-generic/bug.h | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)Index:
linux-2.6.24-rc6/include/asm-generic/bug.h
===================================================================include/asm-generic/bug.h +0
-0@@ -31,14 +31,19 @@ struct bug_entry {#define BUG_ON (condition) do { if (unlikely(condition)) BUG(); }while(0) #endif
include/asm-generic/bug.h +2
-1-#ifndef HAVE_ARCH_WARN_ON +#ifndef __WARN +#define __WARN () do{ \
include/asm-generic/bug.h +1
-0+ printk("WARNING: at %s:%d %s()\n",
__FILE__, \include/asm-generic/bug.h +1
-0+ __LINE__,
__FUNCTION__); \
+dump_stack();
\ +} while (0) +#endif
Quoted 8 lines
Quoted 5 lines
include/asm-generic/bug.h +2
-0+ +#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
Quoted 2 lines
(btw, wouldn't 'var != 0' actually be the proper semantic instead of playing with '!'s?)
no because var could be a pointer for example...
Signature · 3 lines
If you want to reach me at my work email, use [email protected] For development, discussion and tips for power savings, visit http://www.lesswatts.org
Re: [patch 1/5] Introduce __WARN()
Quoted 50 lines · Arjan van de Ven
Quoted 36 lines · On Sun, 06 Jan 2008 12:44:56 +0100 Richard Knutsson <[email protected]>…
Quoted 34 lines · Arjan van de Ven
From: Olof Johansson <[email protected]>
Introduce
__WARN()in the generic case, so the genericWARN_ON()can use arch-specific code for when the condition is true.Signed-off-by Olof Johansson <[email protected]>Cc <[email protected]>Signed-off-by Andrew Morton <[email protected]>---
include/asm-generic/bug.h | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)Index:
linux-2.6.24-rc6/include/asm-generic/bug.h
===================================================================include/asm-generic/bug.h +0
-0@@ -31,14 +31,19 @@ struct bug_entry {#define BUG_ON (condition) do { if (unlikely(condition)) BUG(); }while(0) #endif
include/asm-generic/bug.h +2
-1-#ifndef HAVE_ARCH_WARN_ON +#ifndef __WARN +#define __WARN () do{ \
include/asm-generic/bug.h +1
-0+ printk("WARNING: at %s:%d %s()\n",
__FILE__, \include/asm-generic/bug.h +1
-0+ __LINE__,
__FUNCTION__); \
+dump_stack();
\ +} while (0) +#endifinclude/asm-generic/bug.h +2
-0+ +#ifndef WARN_ON Quoted 9 lines
Quoted 4 lines
#define
WARN_ON(condition)
({ \ int__ret_warn_on= !!(condition); \What about using a boolean for
__ret_warn_on, which then let us remove the '!!'?
Quoted 3 lines
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'".)
Quoted 6 lines
Quoted 3 lines
(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()
Quoted 8 lines · On Sun, 06 Jan 2008 17:09:44 +0100 Richard Knutsson <[email protected]>…
Quoted 6 lines
Quoted 3 lines
(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.
Signature · 3 lines
If you want to reach me at my work email, use [email protected] For development, discussion and tips for power savings, visit http://www.lesswatts.org
Re: [patch 1/5] Introduce __WARN()
Quoted 8 lines · Arjan van de Ven
Quoted 4 lines · On Sun, 06 Jan 2008 17:09:44 +0100 Richard Knutsson <[email protected]>…
Quoted 4 lines
Quoted 4 lines
(btw, wouldn't 'var != 0' actually be the proper semantic instead of playing with '!'s?)
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
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 ;).
Quoted 5 lines
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
PATCH 2/5 move WARN_ON() out of line
In reply to Arjan van de VenSubject: move WARN_ON() out of line
From: Arjan van de Ven <[email protected]>
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...
--- include/asm-generic/bug.h | 10 +++++----- kernel/panic.c | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 5 deletions(-)
include/asm-generic/bug.h +5 -5
@@ -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
kernel/panic.c +15 -0
@@ -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 [email protected] For development, discussion and tips for power savings, visit http://www.lesswatts.org
Re: [patch 2/5] move WARN_ON() out of line
Quoted 17 lines · Arjan van de Ven
From: Arjan van de Ven <[email protected]>
CC Ingo Molnar <[email protected]>CC Andrew Morton <[email protected]>CC Olof Johansson <[email protected]>Acked-by Matt Meckall <[email protected]>A quick grep shows that there are currently 1145 instances of
WARN_ONin the kernel. Currently,WARN_ONis 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:
Quoted 6 lines
3936367 833603 624736 5394706 525112 vmlinux.before 3917508 833603 624736 5375847 520767 vmlinux-slowpath15Kb savings...
Signed-off-by Arjan van de Ven <[email protected]>
PATCH 3/5 Add the end-of-trace marker and the module list to WARN_ON()
In reply to Arjan van de VenSubject: Add the end-of-trace marker and the module list to WARN_ON()
From: Arjan van de Ven <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Andrew Morton <[email protected]>
CC: Olof Johansson <[email protected]>
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.
--- kernel/panic.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
kernel/panic.c +12 -4
@@ -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
Signature · 3 lines
If you want to reach me at my work email, use [email protected] 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()
Quoted 2 lines · Arjan van de Ven
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.
Signature · 1 line
dwmw2
Re: [patch 3/5] Add the end-of-trace marker and the module list to WARN_ON()
Quoted 8 lines · David Woodhouse
Quoted 2 lines · Arjan van de Ven
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 ---"?
PATCH 4/5 bugh-remove-have_arch_bug--have_arch_warn
In reply to Arjan van de VenFrom: Olof Johansson <[email protected]>
No need to have the HAVE_ARCH_BUG.* / HAVE_ARCH_WARN.* defines, when the generic implementation can just use #ifndef on the macros themselves.
---
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(-)
include/asm-alpha/bug.h +0 -1
@@ -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>
include/asm-arm/bug.h +0 -1
@@ -16,7 +16,6 @@ extern void __bug(const char *file, int#endif -#define HAVE_ARCH_BUG #endif #include <asm-generic/bug.h>
include/asm-avr32/bug.h +0 -3
@@ -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>
include/asm-frv/bug.h +0 -1
@@ -32,7 +32,6 @@ do { \ asmvolatile ("nop" ); \ } while(0) -#define HAVE_ARCH_BUG #define BUG () \ do { \ _debug_bug_printk(); \
include/asm-generic/bug.h +5 -5
@@ -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); \
include/asm-ia64/bug.h +0 -1
@@ -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>
include/asm-m68k/bug.h +0 -1
@@ -21,7 +21,6 @@ } while (0)#endif -#define HAVE_ARCH_BUG #endif #include <asm-generic/bug.h>
include/asm-mips/bug.h +0 -4
@@ -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
include/asm-parisc/bug.h +0 -2
@@ -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"
include/asm-powerpc/bug.h +0 -3
@@ -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 */
include/asm-s390/bug.h +0 -2
@@ -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>
include/asm-sparc/bug.h +0 -1
@@ -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>
include/asm-sparc64/bug.h +0 -1
@@ -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>
include/asm-v850/bug.h +0 -1
@@ -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>
include/asm-x86/bug.h +0 -1
@@ -2,7 +2,6 @@#define _ASM_X86_BUG_H#ifdef CONFIG_BUG -#define HAVE_ARCH_BUG #ifdef CONFIG_DEBUG_BUGVERBOSE
Signature · 3 lines
If you want to reach me at my work email, use [email protected] For development, discussion and tips for power savings, visit http://www.lesswatts.org
PATCH 5/5 PowerPC: switch to generic WARN_ON / BUG_ON
In reply to Arjan van de VenFrom: Olof Johansson <[email protected]>
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.
---
include/asm-powerpc/bug.h | 37 ------------------------------------- 1 file changed, 37 deletions(-)
include/asm-powerpc/bug.h +0 -37
@@ -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 */
Signature · 3 lines
If you want to reach me at my work email, use [email protected] For development, discussion and tips for power savings, visit http://www.lesswatts.org
Re: [patch 5/5] PowerPC: switch to generic WARN_ON / BUG_ON
Quoted 6 lines · Arjan van de Ven
From: Olof Johansson <[email protected]>
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.
Quoted 75 lines
Signed-off-by Olof Johansson <[email protected]>Cc <[email protected]>Cc Scott Wood <[email protected]>Cc Benjamin Herrenschmidt <[email protected]>Cc Paul Mackerras <[email protected]>,Signed-off-by Andrew Morton <[email protected]>---
include/asm-powerpc/bug.h | 37 ------------------------------------- 1 file changed, 37 deletions(-)Index:
linux-2.6.24-rc6/include/asm-powerpc/bug.h
===================================================================include/asm-powerpc/bug.h +0
-37@@ -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 5/5] PowerPC: switch to generic WARN_ON / BUG_ON
Quoted 4 lines · Benjamin Herrenschmidt
Quoted 2 lines · Arjan van de Ven
From: Olof Johansson <[email protected]>
Quoted 8 lines
Quoted 4 lines
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
Quoted 10 lines · Arjan van de Ven
3rd try for this patch series; now with a split up patch for
__WARN_ONThis series has the goal of extending the usefulness of the
WARN_ON()information, at least on architectures that use the genericWARN_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.
Quoted 3 lines
As part of this, I had to split the
__WARN_ONpatch 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
PATCH 1/1 Add bug/warn marker to generic report_bug()
In reply to Arjan van de VenPowerpc 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.
---
Quoted 11 lines · Arjan van de Ven
3rd try for this patch series; now with a split up patch for
__WARN_ONThis series has the goal of extending the usefulness of the
WARN_ON()information, at least on architectures that use the genericWARN_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_ONpatch 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
include/linux/kernel.h +1 -0
@@ -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)
kernel/panic.c +1 -1
@@ -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",
lib/bug.c +2 -0
@@ -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()
Quoted 7 lines · On Sun, 6 Jan 2008 14:22:23 -0600 Olof Johansson <[email protected]> wrote:
Powerpc uses the generic
report_bug()fromlib/bug.cto 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()
Quoted 6 lines · Arjan van de Ven
Quoted 3 lines · On Sun, 6 Jan 2008 14:22:23 -0600 Olof Johansson <[email protected]> wrote:
Powerpc uses the generic
report_bug()fromlib/bug.cto report warnings, and I'm guessing other arches do as well.
Quoted 8 lines
Quoted 4 lines
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 useoops_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()
Quoted 7 lines · On Sun, 6 Jan 2008 19:22:37 -0600 Olof Johansson <[email protected]> wrote:
This comment in
oops_enterthrew 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
Signature · 3 lines
If you want to reach me at my work email, use [email protected] For development, discussion and tips for power savings, visit http://www.lesswatts.org