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(-)
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,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
===================================================================
--- 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,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
On Thu, 2008-01-03 at 22:26 +0100, Arjan van de Ven wrote:
> 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
Nice!
Acked-by: Matt Mackall <[email protected]>
--
Mathematics is the supreme nostalgia of our time.
* Arjan van de Ven <[email protected]> wrote:
> 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
On Fri, Jan 04, 2008 at 08:41:49AM +0100, Ingo Molnar wrote:
>
> * Arjan van de Ven <[email protected]> wrote:
>
> > 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?
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
Hi,
On Thu, Jan 03, 2008 at 10:26:05PM +0100, Arjan van de Ven wrote:
> 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:
> 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,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
>
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
On Fri, Jan 04, 2008 at 08:41:49AM +0100, Ingo Molnar wrote:
>...
> 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/.
> Ingo
cu
Adrian
--
"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
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).
>
> 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 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]>
> 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
> ===================================================================
> --- linux-2.6.24-rc6.orig/include/asm-generic/bug.h
> +++ 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
> 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,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
> ===================================================================
> --- linux-2.6.24-rc6.orig/include/asm-generic/bug.h
> +++ 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
> 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,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