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

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

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

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 moves WARN_ON() out of line entirely. I've considered keeping
the test inline and moving only the slowpath out of line, but I decided
against that: an out of line test reduces the pressure on the CPUs
branch predictor logic and gives smaller code, while a function call
to a fixed location is quite fast. Likewise I've considered doing something
similar to BUG() (eg use a trapping instruction) but that's not really
better (it needs the test inline again and recovering from an invalid
instruction isn't quite fun).

The code size reduction of this patch was about 6.5Kb (on a distro style
.config):

    text	   data	    bss	    dec	    hex	filename
3096493	 293455	2760704	6150652	 5dd9fc	vmlinux.before
3090006	 293455	2760704	6144165	 5dc0a5	vmlinux.after

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

---
  include/asm-generic/bug.h |   13 ++++---------
  kernel/panic.c            |   13 +++++++++++++
  2 files changed, 17 insertions(+), 9 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,15 +32,10 @@ struct bug_entry {
  #endif

  #ifndef HAVE_ARCH_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();						\
-	}								\
-	unlikely(__ret_warn_on);					\
-})
+extern int do_warn_on(const unsigned long condition, const char *file,
+	const int line, const char *function);
+#define WARN_ON(condition) do_warn_on((unsigned long)(condition), __FILE__, \
+					 __LINE__, __FUNCTION__)
  #endif

  #else /* !CONFIG_BUG */
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 <asm/bug.h>

  int panic_on_oops;
  int tainted;
@@ -292,6 +293,18 @@ void oops_exit(void)
  		(unsigned long long)oops_id);
  }

+int do_warn_on(const unsigned long condition, const char *file,
+			const int line, const char *function)
+{
+	if (unlikely(condition)) {
+		printk(KERN_WARNING "WARNING: at %s:%d %s()\n",
+			__FILE__, __LINE__, __FUNCTION__);
+		dump_stack();
+	}
+	return !!condition;
+}
+EXPORT_SYMBOL(do_warn_on);
+
  #ifdef CONFIG_CC_STACKPROTECTOR
  /*
   * Called when gcc's -fstack-protector feature is used, and

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

[Matt Mackall]

On Thu, 2008-01-03 at 01:56 +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>
> 
> 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 moves WARN_ON() out of line entirely. I've considered keeping
> the test inline and moving only the slowpath out of line, but I decided
> against that: an out of line test reduces the pressure on the CPUs
> branch predictor logic and gives smaller code, while a function call
> to a fixed location is quite fast. Likewise I've considered doing something
> similar to BUG() (eg use a trapping instruction) but that's not really
> better (it needs the test inline again and recovering from an invalid
> instruction isn't quite fun).
> 
> The code size reduction of this patch was about 6.5Kb (on a distro style
> .config):
> 
>     text	   data	    bss	    dec	    hex	filename
> 3096493	 293455	2760704	6150652	 5dd9fc	vmlinux.before
> 3090006	 293455	2760704	6144165	 5dc0a5	vmlinux.after
> 
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

I hate the do_foo naming scheme (how about __warn_on?), but otherwise:

Acked-by: Matt Mackall <mpm@selenic.com>

> +		printk(KERN_WARNING "WARNING: at %s:%d %s()\n",
> +			__FILE__, __LINE__, __FUNCTION__);
> +		dump_stack();

While we're here, I'll mention that dump_stack probably ought to take a
severity level argument.

-- 
Mathematics is the supreme nostalgia of our time.

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

[Arjan van de Ven]

Matt Mackall wrote:
> 
> I hate the do_foo naming scheme (how about __warn_on?), but otherwise:
> 
> Acked-by: Matt Mackall <mpm@selenic.com>

after I moved it around based on Olof's work, I've now ended up with

warn_on_slowpath()

> 
>> +		printk(KERN_WARNING "WARNING: at %s:%d %s()\n",
>> +			__FILE__, __LINE__, __FUNCTION__);
>> +		dump_stack();
> 
> While we're here, I'll mention that dump_stack probably ought to take a
> severity level argument.

125 files changed, 202 insertions(+), 199 deletions(-)
just to get the api change done.
I can hear akpm cringe from here...

> 

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

[Herbert Xu]

Arjan van de Ven <arjan@linux.intel.com> wrote:
>
>> While we're here, I'll mention that dump_stack probably ought to take a
>> severity level argument.
> 
> 125 files changed, 202 insertions(+), 199 deletions(-)
> just to get the api change done.
> I can hear akpm cringe from here...

You don't have to change all of them at once.  Just create a new function
that does take a level and make the old dump_stack and WARN_ON call that
then slowly convert everything else across if so desired.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Arjan van de Ven]

Herbert Xu wrote:
> Arjan van de Ven <arjan@linux.intel.com> wrote:
>>> While we're here, I'll mention that dump_stack probably ought to take a
>>> severity level argument.
>> 125 files changed, 202 insertions(+), 199 deletions(-)
>> just to get the api change done.
>> I can hear akpm cringe from here...
> 
> You don't have to change all of them at once. 

Having 2 is just silly though, and it'll take a long time, if ever,
to do this sort of thing slowly. Better to do it all at once.
The amount of change doesn't get smaller if you spread it out;
it's still a question if the total amount of change is worth
the added functionality.

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

[Olof Johansson]

On Thu, Jan 03, 2008 at 01:56:58AM +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>
>
> 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 moves WARN_ON() out of line entirely. I've considered keeping
> the test inline and moving only the slowpath out of line, but I decided
> against that: an out of line test reduces the pressure on the CPUs
> branch predictor logic and gives smaller code, while a function call
> to a fixed location is quite fast. Likewise I've considered doing something
> similar to BUG() (eg use a trapping instruction) but that's not really
> better (it needs the test inline again and recovering from an invalid
> instruction isn't quite fun).

Hi Arjan,

I've got a couple of patches in -mm at the moment that introduces __WARN()
and uses that (and lets architectures override __WARN, since for example
powerpc does use trapping instructions similarly to BUG()).

The two patches in question are:

bugh-remove-have_arch_bug--have_arch_warn.patch
powerpc-switch-to-generic-warn_on-bug_on.patch

Care to do this incrementally on top of that instead? I.e. call
do_warn_on() from the asm-generic/bug.h __WARN() instead.


-Olof

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

[Arjan van de Ven]

Olof Johansson wrote:
> On Thu, Jan 03, 2008 at 01:56:58AM +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>
>>
>> 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 moves WARN_ON() out of line entirely. I've considered keeping
>> the test inline and moving only the slowpath out of line, but I decided
>> against that: an out of line test reduces the pressure on the CPUs
>> branch predictor logic and gives smaller code, while a function call
>> to a fixed location is quite fast. Likewise I've considered doing something
>> similar to BUG() (eg use a trapping instruction) but that's not really
>> better (it needs the test inline again and recovering from an invalid
>> instruction isn't quite fun).
> 
> Hi Arjan,
> 
> I've got a couple of patches in -mm at the moment that introduces __WARN()
> and uses that (and lets architectures override __WARN, since for example
> powerpc does use trapping instructions similarly to BUG()).
> 
> The two patches in question are:
> 
> bugh-remove-have_arch_bug--have_arch_warn.patch
> powerpc-switch-to-generic-warn_on-bug_on.patch
> 
> Care to do this incrementally on top of that instead? I.e. call
> do_warn_on() from the asm-generic/bug.h __WARN() instead.
> 
> 
ok just did that; will post shortly

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

[Ingo Molnar]

* Arjan van de Ven <arjan@linux.intel.com> wrote:

> +#define WARN_ON(condition) do_warn_on((unsigned long)(condition), __FILE__, \
> +					 __LINE__, __FUNCTION__)

hm. This passes in 4 arguments to do_warn_on().

i think we could get away with no arguments (!), by using section 
tricks. Firstly, we can get rid of __FUNCTION__ and replace it with a 
ksyms lookup - that is fine enough. Secondly, we could put __FILE__ and 
__LINE__ into a text section and key it up to the return address from 
do_warn_on().

the condition code should not be passed in at all i think - it creates 
extra function calls to do_warn_on() all the time.

	Ingo

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

[Arjan van de Ven]

Ingo Molnar wrote:
> * Arjan van de Ven <arjan@linux.intel.com> wrote:
> 
>> +#define WARN_ON(condition) do_warn_on((unsigned long)(condition), __FILE__, \
>> +					 __LINE__, __FUNCTION__)
> 
> hm. This passes in 4 arguments to do_warn_on().
> 
> i think we could get away with no arguments (!), by using section 
> tricks. Firstly, we can get rid of __FUNCTION__ and replace it with a 
> ksyms lookup - that is fine enough.

I can see that; I'll play with that

> Secondly, we could put __FILE__ and 
> __LINE__ into a text section and key it up to the return address from 
> do_warn_on().

the asm generated for this is 2 movl instructions for immediate to register.
Doing fancy tricks ... it may well end up bigger and gain nothing.
> 
> the condition code should not be passed in at all i think - it creates 
> extra function calls to do_warn_on() all the time.

function calls are *CHEAP*.

passing the condition is actually near free (remember we have regparm!), it's likely to be
in a register already anyway.

Doing the test inline makes stuff bigger, and also spreads the branch prediction pain around
rather than having one nicely predictable place...

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

[Jeremy Fitzhardinge]

Ingo Molnar wrote:
> * Arjan van de Ven <arjan@linux.intel.com> wrote:
>
>   
>> +#define WARN_ON(condition) do_warn_on((unsigned long)(condition), __FILE__, \
>> +					 __LINE__, __FUNCTION__)
>>     
>
> hm. This passes in 4 arguments to do_warn_on().
>
> i think we could get away with no arguments (!), by using section 
> tricks. Firstly, we can get rid of __FUNCTION__ and replace it with a 
> ksyms lookup - that is fine enough. Secondly, we could put __FILE__ and 
> __LINE__ into a text section and key it up to the return address from 
> do_warn_on().
>   

BUG_ON already does this, and WARN_ON can reuse all the same machinery.

    J

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

[Pekka Enberg]

Hi Arjan,

On Jan 3, 2008 2:56 AM, Arjan van de Ven <arjan@linux.intel.com> wrote:
> +int do_warn_on(const unsigned long condition, const char *file,
> +                       const int line, const char *function)
> +{
> +       if (unlikely(condition)) {
> +               printk(KERN_WARNING "WARNING: at %s:%d %s()\n",
> +                       __FILE__, __LINE__, __FUNCTION__);

I don't think you want to use __FILE__ and friends here...

Re: [patch 1/3] 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>
> 
> 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 moves WARN_ON() out of line entirely. I've considered keeping
> the test inline and moving only the slowpath out of line, but I decided
> against that: an out of line test reduces the pressure on the CPUs
> branch predictor logic and gives smaller code, while a function call
> to a fixed location is quite fast. Likewise I've considered doing something
> similar to BUG() (eg use a trapping instruction) but that's not really
> better (it needs the test inline again and recovering from an invalid
> instruction isn't quite fun).
> 
> The code size reduction of this patch was about 6.5Kb (on a distro style
> .config):
> 
>    text       data        bss        dec        hex    filename
> 3096493     293455    2760704    6150652     5dd9fc    vmlinux.before
> 3090006     293455    2760704    6144165     5dc0a5    vmlinux.after
> 
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> 
> ---
>  include/asm-generic/bug.h |   13 ++++---------
>  kernel/panic.c            |   13 +++++++++++++
>  2 files changed, 17 insertions(+), 9 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,15 +32,10 @@ struct bug_entry {
>  #endif
> 
>  #ifndef HAVE_ARCH_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();                        \
> -    }                                \
> -    unlikely(__ret_warn_on);                    \
> -})
> +extern int do_warn_on(const unsigned long condition, const char *file,
> +    const int line, const char *function);
> +#define WARN_ON(condition) do_warn_on((unsigned long)(condition),
> __FILE__, \
> +                     __LINE__, __FUNCTION__)
>  #endif
> 
>  #else /* !CONFIG_BUG */
> 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 <asm/bug.h>
> 
>  int panic_on_oops;
>  int tainted;
> @@ -292,6 +293,18 @@ void oops_exit(void)
>          (unsigned long long)oops_id);
>  }
> 
> +int do_warn_on(const unsigned long condition, const char *file,
> +            const int line, const char *function)
> +{
> +    if (unlikely(condition)) {
> +        printk(KERN_WARNING "WARNING: at %s:%d %s()\n",
> +            __FILE__, __LINE__, __FUNCTION__);

Isn't this going to print "panic.c" for __FILE__, "do_warn_on" for __FUNCTION__
and whatever line number you have there for __LINE__? I put up a userspace equivalent
of what you're doing here, which confirms my suspision:

dmvo@cipher:/tmp$ cat c.c
#include <stdio.h>

#define WARN_ON(condition) do_warn_on((unsigned long)(condition), __FILE__, \
                        __LINE__, __FUNCTION__)

int do_warn_on(const unsigned long condition, const char *file,
                const int line, const char *function)
{
        if (condition) {
                printf("WARNING: at %s:%d %s()\n",
                                __FILE__, __LINE__, __FUNCTION__);
        }
        return !!condition;
}

int main()
{
        WARN_ON(1);
        return 0;
}
dmvo@cipher:/tmp$ cc c.c
dmvo@cipher:/tmp$ ./a.out
WARNING: at c.c:11 do_warn_on()
dmvo@cipher:/tmp$

I think that your intention was to propagate the parameters to the do_warn_on() routine
to the printk() call, right?

> +        dump_stack();
> +    }
> +    return !!condition;
> +}
> +EXPORT_SYMBOL(do_warn_on);
> +
>  #ifdef CONFIG_CC_STACKPROTECTOR
>  /*
>   * Called when gcc's -fstack-protector feature is used, and
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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

[Jeremy Fitzhardinge]

Arjan van de Ven wrote:
> This patch moves WARN_ON() out of line entirely. I've considered keeping
> the test inline and moving only the slowpath out of line, but I decided
> against that: an out of line test reduces the pressure on the CPUs
> branch predictor logic and gives smaller code, while a function call
> to a fixed location is quite fast. Likewise I've considered doing
> something
> similar to BUG() (eg use a trapping instruction) but that's not really
> better (it needs the test inline again and recovering from an invalid
> instruction isn't quite fun).

Power implements WARN_ON this way, and all the machinery is in place to
generically implement WARN_ON that way if you want.  It does generate
denser code than the call (since its just a single trapping instruction
with no need for argument setup), and the performance cost of the trap
shouldn't matter if warnings are rare (which one would hope).

    J

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

[Arjan van de Ven]

Jeremy Fitzhardinge wrote:
> Arjan van de Ven wrote:
>> This patch moves WARN_ON() out of line entirely. I've considered keeping
>> the test inline and moving only the slowpath out of line, but I decided
>> against that: an out of line test reduces the pressure on the CPUs
>> branch predictor logic and gives smaller code, while a function call
>> to a fixed location is quite fast. Likewise I've considered doing
>> something
>> similar to BUG() (eg use a trapping instruction) but that's not really
>> better (it needs the test inline again and recovering from an invalid
>> instruction isn't quite fun).
> 
> Power implements WARN_ON this way, and all the machinery is in place to
> generically implement WARN_ON that way if you want.  It does generate
> denser code than the call (since its just a single trapping instruction
> with no need for argument setup), and the performance cost of the trap
> shouldn't matter if warnings are rare (which one would hope).

I just did an experiment with this to see how much is on the table. I made
a file with 1024 WARN_ON()'s (new style, eg the out of line call) and 1024 BUG_ON()'s,
which on i386 already use the trap.
This shows that the BUG_ON() case is 2Kb shorter in generated code. From this 2Kb you
need to subtract all the code size that is needed to deal with the trap and the module
merging/unmerging of trap points etc etc, so lets say that a total of 1Kb is left on the table.
HOWEVER, if you have a module with, say, only 4 WARN_ON()/BUG_ON()'s, you actually LOOSE
48 bytes, because of the extra overhead of how the trap data is stored.

So... call me unconvinced for now. There's 30 Kb on the table with the easy, obviously safe
transform, and maybe another 1Kb with the much more tricky trapping scenario, but only
for the vmlinux case; the module case seems to be a loss instead.

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

[Arjan van de Ven]

Arjan van de Ven wrote:
> Jeremy Fitzhardinge wrote:
>> Arjan van de Ven wrote:
>>> This patch moves WARN_ON() out of line entirely. I've considered keeping
>>> the test inline and moving only the slowpath out of line, but I decided
>>> against that: an out of line test reduces the pressure on the CPUs
>>> branch predictor logic and gives smaller code, while a function call
>>> to a fixed location is quite fast. Likewise I've considered doing
>>> something
>>> similar to BUG() (eg use a trapping instruction) but that's not really
>>> better (it needs the test inline again and recovering from an invalid
>>> instruction isn't quite fun).
>>
>> Power implements WARN_ON this way, and all the machinery is in place to
>> generically implement WARN_ON that way if you want.  It does generate
>> denser code than the call (since its just a single trapping instruction
>> with no need for argument setup), and the performance cost of the trap
>> shouldn't matter if warnings are rare (which one would hope).
> 
> I just did an experiment with this to see how much is on the table. I made
> a file with 1024 WARN_ON()'s (new style, eg the out of line call) and 
> 1024 BUG_ON()'s,
> which on i386 already use the trap.
> This shows that the BUG_ON() case is 2Kb shorter in generated code. From 
> this 2Kb you
> need to subtract all the code size that is needed to deal with the trap 
> and the module
> merging/unmerging of trap points etc etc, so lets say that a total of 
> 1Kb is left on the table.
> HOWEVER, if you have a module with, say, only 4 WARN_ON()/BUG_ON()'s, 
> you actually LOOSE
> 48 bytes, because of the extra overhead of how the trap data is stored.
> 
> So... call me unconvinced for now. There's 30 Kb on the table with the 
> easy, obviously safe
> transform, and maybe another 1Kb with the much more tricky trapping 
> scenario, but only
> for the vmlinux case; the module case seems to be a loss instead.
> 
> 
Eh I have to retract my math here; I used a slightly older version of the WARN_ON patch series.
(before Ingo's suggestion)
In the new model, even at 1024 the out of line WARN_ON function call is smaller than the BUG_ON method.

So I think that at least for x86, it's a loss to do what you suggest....

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

[Arjan van de Ven]

Arjan van de Ven wrote:
>> So... call me unconvinced for now. There's 30 Kb on the table with the 
>> easy, obviously safe
>> transform, and maybe another 1Kb with the much more tricky trapping 
>> scenario, but only
>> for the vmlinux case; the module case seems to be a loss instead.
>>
>>
> Eh I have to retract my math here; I used a slightly older version of 
> the WARN_ON patch series.
> (before Ingo's suggestion)
> In the new model, even at 1024 the out of line WARN_ON function call is 
> smaller than the BUG_ON method.
> 
> So I think that at least for x86, it's a loss to do what you suggest....
> 


if people wonder where this comes from:
the BUG_ON code sequence is 13 bytes, the WARN_ON sequence
is 24 bytes, so 11 bytes longer. HOWEVER, the BUG_ON approach
also needs 12 bytes of data (20 on 64 bit) per bug, a nett loss
of 1 byte on 32 bit x86. (plus some general overhead for storing
sections as such, but that scales per ELF file, not per BUG_ON instance)

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

[Jeremy Fitzhardinge]

Arjan van de Ven wrote:
> Jeremy Fitzhardinge wrote:
>> Arjan van de Ven wrote:
>>> This patch moves WARN_ON() out of line entirely. I've considered
>>> keeping
>>> the test inline and moving only the slowpath out of line, but I decided
>>> against that: an out of line test reduces the pressure on the CPUs
>>> branch predictor logic and gives smaller code, while a function call
>>> to a fixed location is quite fast. Likewise I've considered doing
>>> something
>>> similar to BUG() (eg use a trapping instruction) but that's not really
>>> better (it needs the test inline again and recovering from an invalid
>>> instruction isn't quite fun).
>>
>> Power implements WARN_ON this way, and all the machinery is in place to
>> generically implement WARN_ON that way if you want.  It does generate
>> denser code than the call (since its just a single trapping instruction
>> with no need for argument setup), and the performance cost of the trap
>> shouldn't matter if warnings are rare (which one would hope).
>
> I just did an experiment with this to see how much is on the table. I
> made
> a file with 1024 WARN_ON()'s (new style, eg the out of line call) and
> 1024 BUG_ON()'s,
> which on i386 already use the trap.
> This shows that the BUG_ON() case is 2Kb shorter in generated code.
> From this 2Kb you
> need to subtract all the code size that is needed to deal with the
> trap and the module
> merging/unmerging of trap points etc etc, so lets say that a total of
> 1Kb is left on the table.
> HOWEVER, if you have a module with, say, only 4 WARN_ON()/BUG_ON()'s,
> you actually LOOSE
> 48 bytes, because of the extra overhead of how the trap data is stored.
>
> So... call me unconvinced for now. There's 30 Kb on the table with the
> easy, obviously safe
> transform, and maybe another 1Kb with the much more tricky trapping
> scenario, but only
> for the vmlinux case; the module case seems to be a loss instead. 

Yeah, that seems reasonable if you're optimising for overall size.  Did
you count the difference of including the function name?  We decided not
to include it for BUG because its usefulness/size tradeoff didn't seem
terribly important.

But my goal was actually to reduce icache pollution, so by my reckoning
code bytes were much more expensive than data ones, so putting all BUG
information in a separate section makes those bytes much less
significant than putting anything inline in code.  Also, the trap for
WARN_ON would be smaller than BUG, because it wouldn't need the spurious
infinite loop needed to make gcc understand the control flow of a BUG.

On the other hand, you could put the call to out of line warning
function in a separate section to achieve the same effect.

    J

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

[Arjan van de Ven]

Jeremy Fitzhardinge wrote:
> 
> Yeah, that seems reasonable if you're optimising for overall size.  Did
> you count the difference of including the function name?  We decided not
> to include it for BUG because its usefulness/size tradeoff didn't seem
> terribly important.

in the WARN_ON case it's not there either, based on Ingo's idea we do a kallsyms lookup
of __builtin_return_address(0) .. same data, less memory.


> But my goal was actually to reduce icache pollution, so by my reckoning
> code bytes were much more expensive than data ones, so putting all BUG
> information in a separate section makes those bytes much less
> significant than putting anything inline in code.  Also, the trap for
> WARN_ON would be smaller than BUG, because it wouldn't need the spurious
> infinite loop needed to make gcc understand the control flow of a BUG.
> 
> On the other hand, you could put the call to out of line warning
> function in a separate section to achieve the same effect.

yeah and gcc even has a compiler option for that. Doubt it's really worth it,
we're still talking a few bytes here ;)

> 
>     J