[patch 1/2] move WARN_ON() out of line

[patch 1/2] move WARN_ON() out of line

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]

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>
CC: 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 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 <arjan@linux.intel.com>


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

[-- Attachment #2: out_of_line_WARN_ON.patch --]
[-- Type: text/x-patch, Size: 2537 bytes --]

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>
CC: 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-slowpath

15Kb savings...

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>


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

Re: [patch 1/2] move WARN_ON() out of line

[Matt Mackall]

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 <arjan@linux.intel.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Olof Johansson <olof@lixom.net>
> CC: 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 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 <mpm@selenic.com>

-- 
Mathematics is the supreme nostalgia of our time.

Re: [patch 1/2] move WARN_ON() out of line

[Ingo Molnar]

* Arjan van de Ven <arjan@linux.intel.com> 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 <mingo@elte.hu>

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

Re: [patch 1/2] move WARN_ON() out of line

[Olof Johansson]

On Fri, Jan 04, 2008 at 08:41:49AM +0100, Ingo Molnar wrote:
> 
> * Arjan van de Ven <arjan@linux.intel.com> 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 <mingo@elte.hu>
> 
> 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

Re: [patch 1/2] move WARN_ON() out of line

[Olof Johansson]

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

Re: [patch 1/2] move WARN_ON() out of line

[Adrian Bunk]

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

Re: [patch 1/2] move WARN_ON() out of line

[Dmitri Vorobiev]

Arjan van de Ven пишет:
> 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>
> CC: 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 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 <dmitri.vorobiev@gmail.com>

> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> 
> 
> ---
>  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 <arjan@linux.intel.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Olof Johansson <olof@lixom.net>
> CC: 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-slowpath
> 
> 15Kb savings...
> 
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> 
> 
> ---
>  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