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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
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.
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
===================================================================
--- 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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
Subject: move WARN_ON() out of line
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_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 <[email protected]>
---
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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
Subject: 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.
Signed-off-by: Arjan van de Ven <[email protected]>
---
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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
From: 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.
Signed-off-by: Olof Johansson <[email protected]>
Cc: <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
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.
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
===================================================================
--- 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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
* Arjan van de Ven <[email protected]> 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
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
On Sat, 2008-01-05 at 19:12 -0800, Arjan van de Ven wrote:
> 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.
> 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
> ===================================================================
> --- 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 */
>
>
>
Arjan van de Ven wrote:
> 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.
>
> 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
> ===================================================================
> --- 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
>
>
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 <[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...
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
On Sun, 06 Jan 2008 12:44:56 +0100
Richard Knutsson <[email protected]> wrote:
> Arjan van de Ven wrote:
> > 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.
> >
> > 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
> > ===================================================================
> > --- 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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
Arjan van de Ven wrote:
> On Sun, 06 Jan 2008 12:44:56 +0100
> Richard Knutsson <[email protected]> wrote:
>
>
>> Arjan van de Ven wrote:
>>
>>> 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.
>>>
>>> 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
>>> ===================================================================
>>> --- 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
On Sun, 06 Jan 2008 17:09:44 +0100
Richard Knutsson <[email protected]> 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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
Arjan van de Ven wrote:
> On Sun, 06 Jan 2008 17:09:44 +0100
> Richard Knutsson <[email protected]> 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
On Sat, Jan 05, 2008 at 07:09:59PM -0800, Arjan van de Ven wrote:
> 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_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 <[email protected]>
Acked-by: Olof Johansson <[email protected]>
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 <[email protected]>
---
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;
}
On Sun, 6 Jan 2008 14:22:23 -0600
Olof Johansson <[email protected]> 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)
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 <[email protected]> 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
On Sun, 6 Jan 2008 19:22:37 -0600
Olof Johansson <[email protected]> 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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
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 ---"?