login
Search
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
--
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>
---
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
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
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
--
i've added the first 3 patches to x86.git, for further testing - and
would expect -mm to pick up #4/#5.Ingo
--
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_w...
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) \...
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
--
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...
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--
How about "--- cut 10 lines above this line ---"?
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-slowpath15Kb 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 @@
#...
Acked-by: Olof Johansson <olof@lixom.net>
--
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
--
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--
On Sun, 06 Jan 2008 12:44:56 +0100
is iffy.. like what happens if an u64 is cast to a boolean?
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
--
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
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
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
--
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
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--
| Tarkan Erimer | Re: Dual-Licensing Linux Kernel with GPL V2 and GPL V3 |
| Greg Kroah-Hartman | [PATCH 001/196] Chinese: Add the known_regression URI to the HOWTO |
| Bart Van Assche | Integration of SCST in the mainstream Linux kernel |
| Heiko Carstens | Re: -mm merge plans for 2.6.23 -- sys_fallocate |
git: | |
| Gerrit Renker | [PATCH 27/37] dccp: Integration of dynamic feature activation - part 2 (server side) |
| Jarek Poplawski | Re: [PATCH] pkt_sched: Destroy gen estimators under rtnl_lock(). |
| David Miller | Re: [GIT]: Networking |
| Jarek Poplawski | [PATCH iproute2] Re: HTB accuracy for high speed |
