move WARN_ON() out of line

PATCH In review Arjan van de Ven 6 participants 7 messages Jan 3, 2008 – Jan 5, 2008 Generic include/asm header files

Share & tools

Arjan van de Ven

author

2008-01-03 21:28 UTC #0 lore raw

PATCH 1/2 move WARN_ON() out of line

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]>
CC: 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 rightfully 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 |    7 ++-----
 kernel/panic.c            |   14 ++++++++++++++
 2 files changed, 16 insertions(+), 5 deletions(-)

include/asm-generic/bug.h +2 -5

Index: linux-2.6.24-rc6/include/asm-generic/bug.h
===================================================================
@@ -32,11 +32,8 @@ struct bug_entry {
 #endif
 
 #ifndef __WARN
-#define __WARN() do {							\
-	printk("WARNING: at %s:%d %s()\n", __FILE__,			\
-		__LINE__, __FUNCTION__);				\
-	dump_stack();							\
-} while (0)
+extern void warn_on_slowpath(const char *file, const int line);
+#define __WARN() warn_on_slowpath(__FILE__, __LINE__)
 #endif
 
 #ifndef WARN_ON

kernel/panic.c +14 -0

Index: 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,19 @@ void oops_exit(void)
 		(unsigned long long)oops_id);
 }
 
+
+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);
+
 #ifdef CONFIG_CC_STACKPROTECTOR
 /*
  * Called when gcc's -fstack-protector feature is used, and

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 |    7 ++-----
 kernel/panic.c            |   14 ++++++++++++++
 2 files changed, 16 insertions(+), 5 deletions(-)

include/asm-generic/bug.h +2 -5

Index: linux-2.6.24-rc6/include/asm-generic/bug.h
===================================================================
@@ -32,11 +32,8 @@ struct bug_entry {
 #endif
 
 #ifndef __WARN
-#define __WARN() do {							\
-	printk("WARNING: at %s:%d %s()\n", __FILE__,			\
-		__LINE__, __FUNCTION__);				\
-	dump_stack();							\
-} while (0)
+extern void warn_on_slowpath(const char *file, const int line);
+#define __WARN() warn_on_slowpath(__FILE__, __LINE__)
 #endif
 
 #ifndef WARN_ON

kernel/panic.c +14 -0

Index: 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,19 @@ void oops_exit(void)
 		(unsigned long long)oops_id);
 }
 
+
+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);
+
 #ifdef CONFIG_CC_STACKPROTECTOR
 /*
  * Called when gcc's -fstack-protector feature is used, and

Matt Mackall

Acked-by on v1 1/2

2008-01-03 22:29 UTC #1 lore raw

Quoted 12 lines · Arjan van de Ven

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]>
CC: 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 rightfully unhappy).

Quoted 8 lines

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

Nice!

Acked-by Matt Mackall <[email protected]>

Signature · 1 line

Mathematics is the supreme nostalgia of our time.

Ingo Molnar

Acked-by on v1 1/2

2008-01-04 07:42 UTC #2 lore raw

In reply to Arjan van de Ven

Quoted 10 lines · Arjan van de Ven

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...

hey, cool!

Acked-by Ingo Molnar <[email protected]>

i'm wondering how we could put this into x86.git to get it tested some more. Olof's patch touches other architectures so it's not really appropriate. Maybe a portion of Olof's patch could be applied to make your patch apply cleanly?

Ingo

Olof Johansson

2008-01-04 15:51 UTC #3 lore raw

Quoted 14 lines · Ingo Molnar

Quoted 10 lines · Arjan van de Ven
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...

Quoted 8 lines

hey, cool!
Acked-by Ingo Molnar <[email protected]>
i'm wondering how we could put this into x86.git to get it tested some more. Olof's patch touches other architectures so it's not really appropriate. Maybe a portion of Olof's patch could be applied to make your patch apply cleanly?

My base patch only touches other architectures to remove the no longer needed HAVE_ARCH_BUG. Given the new ifdef in bug.h, there's nothing functional that breaks if they're left in (i.e. split up in two patches).

The only problem with merging it up the x86 path is that the powerpc patch that builds on it would need to be ordered after the x86 merge (in mm etc), or it'll cause build breaks. Keeping it in -mm is nice in that way since Andrew could sequence them right when sending to Linus.

Easiest of all might be to get an Acked-by from Paul on the powerpc patch, and merge it all up through x86.git?

(Paul: This is powerpc-switch-to-generic-warn_on-bug_on.patch in -mm I'm talking about).

-Olof

Olof Johansson

2008-01-04 16:18 UTC #5 lore raw

In reply to Arjan van de Ven

Hi,

Quoted 10 lines · Arjan van de Ven

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...

This patch actually resulted in a slight size increase on PPC, most likely because of the new warn_on_slowpath() that isn't used:

text data bss dec hex filename

8059224 1820584  529064 10408872 9ed3a8 vmlinux.mainline
8058268 1820592  529064 10407924 9ecff4 vmlinux.mm
8058324 1820648  529064 10408036 9ed064 vmlinux.mm+arjanpatch

Also:

Quoted 7 lines

Index: linux-2.6.24-rc6/include/asm-generic/bug.h
===================================================================
include/asm-generic/bug.h +0 -0
@@ -32,11 +32,8 @@ struct bug_entry {
 #endif
 

include/asm-generic/bug.h struct bug_entry { · line 38

Quoted 10 lines

include/asm-generic/bug.h +2 -5

 #ifndef __WARN
-#define __WARN() do {							\
-	printk("WARNING: at %s:%d %s()\n", __FILE__,			\
-		__LINE__, __FUNCTION__);				\
-	dump_stack();							\
-} while (0)
+extern void warn_on_slowpath(const char *file, const int line);
+#define __WARN() warn_on_slowpath(__FILE__, __LINE__)
 #endif

This caused problems on powerpc, since bug.h ends up being pulled into some assembly files. The #defines in there have been fine, but the external declaration threw it off. (They need them to get to some of the include/asm-powerpc/bug.h contents, but it pulls in the generic one as well).

Easiest is probably to encapsulate the whole file in #ifndef __ASSEMBLY__.

-Olof

Adrian Bunk

2008-01-05 10:00 UTC #4 lore raw

In reply to Ingo Molnar

Quoted 4 lines · Ingo Molnar

...
i'm wondering how we could put this into x86.git to get it tested some more.
...

Please don't make x86.git a testing tree for not x86 specific stuff.

The right place for getting stuff tested is -mm, and when looking at git-x86.patch in -mm this tree already touches too many files outside of arch/x86/ and include/asm-x86/.

Quoted 1 line

Ingo

cu
Adrian

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Dmitri Vorobiev

Tested-by on v1 1/2

2008-01-05 13:17 UTC #6 lore raw

In reply to Arjan van de Ven

Arjan van de Ven пишет:

Quoted 19 lines

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]>
CC: 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 rightfully 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 5 lines

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

15Kb savings...

This looks twice as good for one of MIPS configs:

text data bss dec hex filename

4908276  204104 2846304 7958684  79709c vmlinux.after
4941596  204104 2846304 7992004  79f2c4 vmlinux.before
$ file vmlinux
vmlinux: ELF 32-bit MSB executable, MIPS, version 1 (SYSV), statically linked, not stripped

More than 30Kb gone, thanks a lot!

Tested-by Dmitri Vorobiev <[email protected]>

Quoted 143 lines

Signed-off-by Arjan van de Ven <[email protected]>
---
 include/asm-generic/bug.h |    7 ++-----
 kernel/panic.c            |   14 ++++++++++++++
 2 files changed, 16 insertions(+), 5 deletions(-)
Index: linux-2.6.24-rc6/include/asm-generic/bug.h
===================================================================
include/asm-generic/bug.h +2 -5
@@ -32,11 +32,8 @@ struct bug_entry {
 #endif
 
 #ifndef __WARN
-#define __WARN() do {                            \
-    printk("WARNING: at %s:%d %s()\n", __FILE__,            \
-        __LINE__, __FUNCTION__);                \
-    dump_stack();                            \
-} while (0)
+extern void warn_on_slowpath(const char *file, const int line);
+#define __WARN() warn_on_slowpath(__FILE__, __LINE__)
 #endif
 
 #ifndef WARN_ON
Index: linux-2.6.24-rc6/kernel/panic.c
===================================================================
kernel/panic.c +14 -1
@@ -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,19 @@ void oops_exit(void)
         (unsigned long long)oops_id);
 }
 
+
+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);
+
 #ifdef CONFIG_CC_STACKPROTECTOR
 /*
  * Called when gcc's -fstack-protector feature is used, and
 
 
------------------------------------------------------------------------
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]>
CC: 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 |    7 ++-----
 kernel/panic.c            |   14 ++++++++++++++
 2 files changed, 16 insertions(+), 5 deletions(-)
Index: linux-2.6.24-rc6/include/asm-generic/bug.h
===================================================================
include/asm-generic/bug.h +2 -5
@@ -32,11 +32,8 @@ struct bug_entry {
 #endif
 
 #ifndef __WARN
-#define __WARN() do {							\
-	printk("WARNING: at %s:%d %s()\n", __FILE__,			\
-		__LINE__, __FUNCTION__);				\
-	dump_stack();							\
-} while (0)
+extern void warn_on_slowpath(const char *file, const int line);
+#define __WARN() warn_on_slowpath(__FILE__, __LINE__)
 #endif
 
 #ifndef WARN_ON
Index: linux-2.6.24-rc6/kernel/panic.c
===================================================================
kernel/panic.c +14 -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,19 @@ void oops_exit(void)
 		(unsigned long long)oops_id);
 }
 
+
+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);
+
 #ifdef CONFIG_CC_STACKPROTECTOR
 /*
  * Called when gcc's -fstack-protector feature is used, and