[PATCH, re-send] Always trap on BUG()

[PATCH, re-send] Always trap on BUG()

[Arnd Bergmann]

I've run some size analyis using the ARM 'multi_v7_defconfig'
and gcc-4.8, using various definitions for BUG() and BUG_ON(), to
see how big the size improvement actually gets

1. Baseline: normal bug plus CONFIG_BUG_VERBOSE
   text    data     bss     dec     hex filename
3743196  224396  206812 4174404  3fb244 vmlinux-bugverbose

2. disabling CONFIG_BUG_VERBOSE, saving 0.6%
3716920  224380  206812 4148112  3f4b90 vmlinux-nobugverbose

3. #define BUG() panic(__func__), #define BUG_ON(c) if(c) BUG(), saving 1.0%
3701076  224384  206812 4132272  3f0db0 vmlinux-bug-panicfunc

3. #define BUG() panic(__func__), #define BUG_ON(c) if(c) BUG(), saving 1.5%
3678884  224400  206812 4110096  3eb710 vmlinux-bug-panic

4. #define BUG() unreachable(), saving 2.1%
3652636  224384  206812 4083832  3e5078 vmlinux-bug-unreachable

5. as 4, plus #define BUG_ON(c) if(c) unreachable(), saving 2.2%
3651108  224380  206812 4082300  3e4a7c vmlinux-bugon-unreachable

6. #define BUG() do{}while(0), saving 2.1%
3654264  224380  206812 4085456  3e56d0 vmlinux-nobug

7. as 6, plus #define BUG_ON if(0 && (c)) unreachable, saving 2.2%
3648392  223996  206748 4079136  3e3e20 vmlinux-no-bugon

8. my patch below, saving 1.8%
3666532  224380  206812 4097724  3e86bc obj-tmp/vmlinux

The gain of doing unreachable and letting the code run off whereever
is minimal compared to the current state of doing nothing, but it
avoids the warnings.

Same test using x86_defconfig:

1. CONFIG_BUG=y, CONFIG_BUGVERBOSE=n
10797859        1395648 1175552 13369059         cbfee3 vmlinux-x86-bug

2. CONFIG_BUG=n, saves 1.0%
10658553        1395584 1175552 13229689         c9de79 vmlinux-x86-nobug

3. with my patch, saves 0.8%
10684186        1395584 1175552 13255322         ca429a vmlinux-x86-archbug

Getting 1-2% savings in kernel size seems absolutely worth keeping the
option, but 0.2-0.4% left for getting reproducible behavior also seems
worthwhile. The result in the patch below.

This basically loses any of the BUG() reporting, but leaves the
logic to trap and kill the task in place when CONFIG_BUG is disabled.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: linux-arch@vger.kernel.org
Cc: Chen Gang <gang.chen@asianux.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>

diff --git a/arch/arm/include/asm/bug.h b/arch/arm/include/asm/bug.h
index 7af5c6c..4a5b85d 100644
--- a/arch/arm/include/asm/bug.h
+++ b/arch/arm/include/asm/bug.h
@@ -3,8 +3,6 @@
 
 #include <linux/linkage.h>
 
-#ifdef CONFIG_BUG
-
 /*
  * Use a suitable undefined instruction to use for ARM/Thumb2 bug handling.
  * We need to be careful not to conflict with those used by other modules and
@@ -54,7 +52,6 @@ do {								\
 #endif  /* CONFIG_DEBUG_BUGVERBOSE */
 
 #define HAVE_ARCH_BUG
-#endif  /* CONFIG_BUG */
 
 #include <asm-generic/bug.h>
 
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index 2f03ff0..ba38ebb 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -1,7 +1,6 @@
 #ifndef _ASM_X86_BUG_H
 #define _ASM_X86_BUG_H
 
-#ifdef CONFIG_BUG
 #define HAVE_ARCH_BUG
 
 #ifdef CONFIG_DEBUG_BUGVERBOSE
@@ -33,8 +32,6 @@ do {								\
 } while (0)
 #endif
 
-#endif /* !CONFIG_BUG */
-
 #include <asm-generic/bug.h>
 
 #endif /* _ASM_X86_BUG_H */
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 7d10f96..bbd2872 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -112,12 +112,13 @@ extern void warn_slowpath_null(const char *file, const int line);
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while(0)
+#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
 #define WARN_ON(condition) ({						\
 	int __ret_warn_on = !!(condition);				\
+	(void)__ret_warn_on;						\
 	unlikely(__ret_warn_on);					\
 })
 #endif
@@ -125,6 +126,8 @@ extern void warn_slowpath_null(const char *file, const int line);
 #ifndef WARN
 #define WARN(condition, format...) ({					\
 	int __ret_warn_on = !!(condition);				\
+	if (0 && (__ret_warn_on))					\
+		printk(format);						\
 	unlikely(__ret_warn_on);					\
 })
 #endif

[PATCH, re-send] Always trap on BUG()

[Arnd Bergmann]

I've run some size analyis using the ARM 'multi_v7_defconfig'
and gcc-4.8, using various definitions for BUG() and BUG_ON(), to
see how big the size improvement actually gets

1. Baseline: normal bug plus CONFIG_BUG_VERBOSE
   text    data     bss     dec     hex filename
3743196  224396  206812 4174404  3fb244 vmlinux-bugverbose

2. disabling CONFIG_BUG_VERBOSE, saving 0.6%
3716920  224380  206812 4148112  3f4b90 vmlinux-nobugverbose

3. #define BUG() panic(__func__), #define BUG_ON(c) if(c) BUG(), saving 1.0%
3701076  224384  206812 4132272  3f0db0 vmlinux-bug-panicfunc

3. #define BUG() panic(__func__), #define BUG_ON(c) if(c) BUG(), saving 1.5%
3678884  224400  206812 4110096  3eb710 vmlinux-bug-panic

4. #define BUG() unreachable(), saving 2.1%
3652636  224384  206812 4083832  3e5078 vmlinux-bug-unreachable

5. as 4, plus #define BUG_ON(c) if(c) unreachable(), saving 2.2%
3651108  224380  206812 4082300  3e4a7c vmlinux-bugon-unreachable

6. #define BUG() do{}while(0), saving 2.1%
3654264  224380  206812 4085456  3e56d0 vmlinux-nobug

7. as 6, plus #define BUG_ON if(0 && (c)) unreachable, saving 2.2%
3648392  223996  206748 4079136  3e3e20 vmlinux-no-bugon

8. my patch below, saving 1.8%
3666532  224380  206812 4097724  3e86bc obj-tmp/vmlinux

The gain of doing unreachable and letting the code run off whereever
is minimal compared to the current state of doing nothing, but it
avoids the warnings.

Same test using x86_defconfig:

1. CONFIG_BUG=y, CONFIG_BUGVERBOSE=n
10797859        1395648 1175552 13369059         cbfee3 vmlinux-x86-bug

2. CONFIG_BUG=n, saves 1.0%
10658553        1395584 1175552 13229689         c9de79 vmlinux-x86-nobug

3. with my patch, saves 0.8%
10684186        1395584 1175552 13255322         ca429a vmlinux-x86-archbug

Getting 1-2% savings in kernel size seems absolutely worth keeping the
option, but 0.2-0.4% left for getting reproducible behavior also seems
worthwhile. The result in the patch below.

This basically loses any of the BUG() reporting, but leaves the
logic to trap and kill the task in place when CONFIG_BUG is disabled.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: linux-arch at vger.kernel.org
Cc: Chen Gang <gang.chen@asianux.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>

diff --git a/arch/arm/include/asm/bug.h b/arch/arm/include/asm/bug.h
index 7af5c6c..4a5b85d 100644
--- a/arch/arm/include/asm/bug.h
+++ b/arch/arm/include/asm/bug.h
@@ -3,8 +3,6 @@
 
 #include <linux/linkage.h>
 
-#ifdef CONFIG_BUG
-
 /*
  * Use a suitable undefined instruction to use for ARM/Thumb2 bug handling.
  * We need to be careful not to conflict with those used by other modules and
@@ -54,7 +52,6 @@ do {								\
 #endif  /* CONFIG_DEBUG_BUGVERBOSE */
 
 #define HAVE_ARCH_BUG
-#endif  /* CONFIG_BUG */
 
 #include <asm-generic/bug.h>
 
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index 2f03ff0..ba38ebb 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -1,7 +1,6 @@
 #ifndef _ASM_X86_BUG_H
 #define _ASM_X86_BUG_H
 
-#ifdef CONFIG_BUG
 #define HAVE_ARCH_BUG
 
 #ifdef CONFIG_DEBUG_BUGVERBOSE
@@ -33,8 +32,6 @@ do {								\
 } while (0)
 #endif
 
-#endif /* !CONFIG_BUG */
-
 #include <asm-generic/bug.h>
 
 #endif /* _ASM_X86_BUG_H */
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 7d10f96..bbd2872 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -112,12 +112,13 @@ extern void warn_slowpath_null(const char *file, const int line);
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while(0)
+#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
 #define WARN_ON(condition) ({						\
 	int __ret_warn_on = !!(condition);				\
+	(void)__ret_warn_on;						\
 	unlikely(__ret_warn_on);					\
 })
 #endif
@@ -125,6 +126,8 @@ extern void warn_slowpath_null(const char *file, const int line);
 #ifndef WARN
 #define WARN(condition, format...) ({					\
 	int __ret_warn_on = !!(condition);				\
+	if (0 && (__ret_warn_on))					\
+		printk(format);						\
 	unlikely(__ret_warn_on);					\
 })
 #endif

Re: [PATCH, re-send] Always trap on BUG()

[Ingo Molnar]

* Arnd Bergmann <arnd@arndb.de> wrote:

> I've run some size analyis using the ARM 'multi_v7_defconfig'
> and gcc-4.8, using various definitions for BUG() and BUG_ON(), to
> see how big the size improvement actually gets
> 
> 1. Baseline: normal bug plus CONFIG_BUG_VERBOSE
>    text    data     bss     dec     hex filename
> 3743196  224396  206812 4174404  3fb244 vmlinux-bugverbose
> 
> 2. disabling CONFIG_BUG_VERBOSE, saving 0.6%
> 3716920  224380  206812 4148112  3f4b90 vmlinux-nobugverbose
> 
> 3. #define BUG() panic(__func__), #define BUG_ON(c) if(c) BUG(), saving 1.0%
> 3701076  224384  206812 4132272  3f0db0 vmlinux-bug-panicfunc
> 
> 3. #define BUG() panic(__func__), #define BUG_ON(c) if(c) BUG(), saving 1.5%
> 3678884  224400  206812 4110096  3eb710 vmlinux-bug-panic
> 
> 4. #define BUG() unreachable(), saving 2.1%
> 3652636  224384  206812 4083832  3e5078 vmlinux-bug-unreachable
> 
> 5. as 4, plus #define BUG_ON(c) if(c) unreachable(), saving 2.2%
> 3651108  224380  206812 4082300  3e4a7c vmlinux-bugon-unreachable
> 
> 6. #define BUG() do{}while(0), saving 2.1%
> 3654264  224380  206812 4085456  3e56d0 vmlinux-nobug
> 
> 7. as 6, plus #define BUG_ON if(0 && (c)) unreachable, saving 2.2%
> 3648392  223996  206748 4079136  3e3e20 vmlinux-no-bugon
> 
> 8. my patch below, saving 1.8%
> 3666532  224380  206812 4097724  3e86bc obj-tmp/vmlinux
> 
> The gain of doing unreachable and letting the code run off whereever
> is minimal compared to the current state of doing nothing, but it
> avoids the warnings.
> 
> Same test using x86_defconfig:
> 
> 1. CONFIG_BUG=y, CONFIG_BUGVERBOSE=n
> 10797859        1395648 1175552 13369059         cbfee3 vmlinux-x86-bug
> 
> 2. CONFIG_BUG=n, saves 1.0%
> 10658553        1395584 1175552 13229689         c9de79 vmlinux-x86-nobug
> 
> 3. with my patch, saves 0.8%
> 10684186        1395584 1175552 13255322         ca429a vmlinux-x86-archbug
> 
> Getting 1-2% savings in kernel size seems absolutely worth keeping the
> option, but 0.2-0.4% left for getting reproducible behavior also seems
> worthwhile. The result in the patch below.
> 
> This basically loses any of the BUG() reporting, but leaves the
> logic to trap and kill the task in place when CONFIG_BUG is disabled.

This also kills a boatload of compiler warnings when CONFIG_BUG is 
disabled.

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

[PATCH, re-send] Always trap on BUG()

[Ingo Molnar]

* Arnd Bergmann <arnd@arndb.de> wrote:

> I've run some size analyis using the ARM 'multi_v7_defconfig'
> and gcc-4.8, using various definitions for BUG() and BUG_ON(), to
> see how big the size improvement actually gets
> 
> 1. Baseline: normal bug plus CONFIG_BUG_VERBOSE
>    text    data     bss     dec     hex filename
> 3743196  224396  206812 4174404  3fb244 vmlinux-bugverbose
> 
> 2. disabling CONFIG_BUG_VERBOSE, saving 0.6%
> 3716920  224380  206812 4148112  3f4b90 vmlinux-nobugverbose
> 
> 3. #define BUG() panic(__func__), #define BUG_ON(c) if(c) BUG(), saving 1.0%
> 3701076  224384  206812 4132272  3f0db0 vmlinux-bug-panicfunc
> 
> 3. #define BUG() panic(__func__), #define BUG_ON(c) if(c) BUG(), saving 1.5%
> 3678884  224400  206812 4110096  3eb710 vmlinux-bug-panic
> 
> 4. #define BUG() unreachable(), saving 2.1%
> 3652636  224384  206812 4083832  3e5078 vmlinux-bug-unreachable
> 
> 5. as 4, plus #define BUG_ON(c) if(c) unreachable(), saving 2.2%
> 3651108  224380  206812 4082300  3e4a7c vmlinux-bugon-unreachable
> 
> 6. #define BUG() do{}while(0), saving 2.1%
> 3654264  224380  206812 4085456  3e56d0 vmlinux-nobug
> 
> 7. as 6, plus #define BUG_ON if(0 && (c)) unreachable, saving 2.2%
> 3648392  223996  206748 4079136  3e3e20 vmlinux-no-bugon
> 
> 8. my patch below, saving 1.8%
> 3666532  224380  206812 4097724  3e86bc obj-tmp/vmlinux
> 
> The gain of doing unreachable and letting the code run off whereever
> is minimal compared to the current state of doing nothing, but it
> avoids the warnings.
> 
> Same test using x86_defconfig:
> 
> 1. CONFIG_BUG=y, CONFIG_BUGVERBOSE=n
> 10797859        1395648 1175552 13369059         cbfee3 vmlinux-x86-bug
> 
> 2. CONFIG_BUG=n, saves 1.0%
> 10658553        1395584 1175552 13229689         c9de79 vmlinux-x86-nobug
> 
> 3. with my patch, saves 0.8%
> 10684186        1395584 1175552 13255322         ca429a vmlinux-x86-archbug
> 
> Getting 1-2% savings in kernel size seems absolutely worth keeping the
> option, but 0.2-0.4% left for getting reproducible behavior also seems
> worthwhile. The result in the patch below.
> 
> This basically loses any of the BUG() reporting, but leaves the
> logic to trap and kill the task in place when CONFIG_BUG is disabled.

This also kills a boatload of compiler warnings when CONFIG_BUG is 
disabled.

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH, re-send] Always trap on BUG()

[Andrew Morton]

On Fri, 5 Jul 2013 17:38:35 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> I've run some size analyis using the ARM 'multi_v7_defconfig'
> and gcc-4.8, using various definitions for BUG() and BUG_ON(), to
> see how big the size improvement actually gets
> 
> 1. Baseline: normal bug plus CONFIG_BUG_VERBOSE
>    text    data     bss     dec     hex filename
> 3743196  224396  206812 4174404  3fb244 vmlinux-bugverbose
> 
> 2. disabling CONFIG_BUG_VERBOSE, saving 0.6%
> 3716920  224380  206812 4148112  3f4b90 vmlinux-nobugverbose
> 
> 3. #define BUG() panic(__func__), #define BUG_ON(c) if(c) BUG(), saving 1.0%
> 3701076  224384  206812 4132272  3f0db0 vmlinux-bug-panicfunc
> 
> 3. #define BUG() panic(__func__), #define BUG_ON(c) if(c) BUG(), saving 1.5%
> 3678884  224400  206812 4110096  3eb710 vmlinux-bug-panic
> 
> 4. #define BUG() unreachable(), saving 2.1%
> 3652636  224384  206812 4083832  3e5078 vmlinux-bug-unreachable
> 
> 5. as 4, plus #define BUG_ON(c) if(c) unreachable(), saving 2.2%
> 3651108  224380  206812 4082300  3e4a7c vmlinux-bugon-unreachable
> 
> 6. #define BUG() do{}while(0), saving 2.1%
> 3654264  224380  206812 4085456  3e56d0 vmlinux-nobug
> 
> 7. as 6, plus #define BUG_ON if(0 && (c)) unreachable, saving 2.2%
> 3648392  223996  206748 4079136  3e3e20 vmlinux-no-bugon
> 
> 8. my patch below, saving 1.8%
> 3666532  224380  206812 4097724  3e86bc obj-tmp/vmlinux
> 
> The gain of doing unreachable and letting the code run off whereever
> is minimal compared to the current state of doing nothing, but it
> avoids the warnings.
> 
> Same test using x86_defconfig:
> 
> 1. CONFIG_BUG=y, CONFIG_BUGVERBOSE=n
> 10797859        1395648 1175552 13369059         cbfee3 vmlinux-x86-bug
> 
> 2. CONFIG_BUG=n, saves 1.0%
> 10658553        1395584 1175552 13229689         c9de79 vmlinux-x86-nobug
> 
> 3. with my patch, saves 0.8%
> 10684186        1395584 1175552 13255322         ca429a vmlinux-x86-archbug
> 
> Getting 1-2% savings in kernel size seems absolutely worth keeping the
> option, but 0.2-0.4% left for getting reproducible behavior also seems
> worthwhile. The result in the patch below.
> 
> This basically loses any of the BUG() reporting, but leaves the
> logic to trap and kill the task in place when CONFIG_BUG is disabled.

um, nice changelog, but it didn't tell us what the patch actually does.

AFACIT it arranges for the kernel to execute the trap instruction even
when CONFIG_BUG=n?  Can't be bothered fully working it out, really.

It also makes mystery changes to WARN_ON() and WARN().  What are those?

Also, you say "it avoids the warnings".  To what warnings do you refer?
The ones which are already emitted by CONFIG_BUG=n, or new ones which
would have been added had this patch been implemented differently?

Finally, code-size decreases are nice, but what are the runtime effects
of this patch?

I've been thinking for a while that CONFIG_BUG=n is a pretty dumb thing
to do, and that maintaining it (and trying to fix the warnings it
produces) aren't worth the effort and that we should remove the whole
thing.  Perhaps your patch changes that calculus, dunno.  Please discuss.

[PATCH, re-send] Always trap on BUG()

[Andrew Morton]

On Fri, 5 Jul 2013 17:38:35 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> I've run some size analyis using the ARM 'multi_v7_defconfig'
> and gcc-4.8, using various definitions for BUG() and BUG_ON(), to
> see how big the size improvement actually gets
> 
> 1. Baseline: normal bug plus CONFIG_BUG_VERBOSE
>    text    data     bss     dec     hex filename
> 3743196  224396  206812 4174404  3fb244 vmlinux-bugverbose
> 
> 2. disabling CONFIG_BUG_VERBOSE, saving 0.6%
> 3716920  224380  206812 4148112  3f4b90 vmlinux-nobugverbose
> 
> 3. #define BUG() panic(__func__), #define BUG_ON(c) if(c) BUG(), saving 1.0%
> 3701076  224384  206812 4132272  3f0db0 vmlinux-bug-panicfunc
> 
> 3. #define BUG() panic(__func__), #define BUG_ON(c) if(c) BUG(), saving 1.5%
> 3678884  224400  206812 4110096  3eb710 vmlinux-bug-panic
> 
> 4. #define BUG() unreachable(), saving 2.1%
> 3652636  224384  206812 4083832  3e5078 vmlinux-bug-unreachable
> 
> 5. as 4, plus #define BUG_ON(c) if(c) unreachable(), saving 2.2%
> 3651108  224380  206812 4082300  3e4a7c vmlinux-bugon-unreachable
> 
> 6. #define BUG() do{}while(0), saving 2.1%
> 3654264  224380  206812 4085456  3e56d0 vmlinux-nobug
> 
> 7. as 6, plus #define BUG_ON if(0 && (c)) unreachable, saving 2.2%
> 3648392  223996  206748 4079136  3e3e20 vmlinux-no-bugon
> 
> 8. my patch below, saving 1.8%
> 3666532  224380  206812 4097724  3e86bc obj-tmp/vmlinux
> 
> The gain of doing unreachable and letting the code run off whereever
> is minimal compared to the current state of doing nothing, but it
> avoids the warnings.
> 
> Same test using x86_defconfig:
> 
> 1. CONFIG_BUG=y, CONFIG_BUGVERBOSE=n
> 10797859        1395648 1175552 13369059         cbfee3 vmlinux-x86-bug
> 
> 2. CONFIG_BUG=n, saves 1.0%
> 10658553        1395584 1175552 13229689         c9de79 vmlinux-x86-nobug
> 
> 3. with my patch, saves 0.8%
> 10684186        1395584 1175552 13255322         ca429a vmlinux-x86-archbug
> 
> Getting 1-2% savings in kernel size seems absolutely worth keeping the
> option, but 0.2-0.4% left for getting reproducible behavior also seems
> worthwhile. The result in the patch below.
> 
> This basically loses any of the BUG() reporting, but leaves the
> logic to trap and kill the task in place when CONFIG_BUG is disabled.

um, nice changelog, but it didn't tell us what the patch actually does.

AFACIT it arranges for the kernel to execute the trap instruction even
when CONFIG_BUG=n?  Can't be bothered fully working it out, really.

It also makes mystery changes to WARN_ON() and WARN().  What are those?

Also, you say "it avoids the warnings".  To what warnings do you refer?
The ones which are already emitted by CONFIG_BUG=n, or new ones which
would have been added had this patch been implemented differently?

Finally, code-size decreases are nice, but what are the runtime effects
of this patch?

I've been thinking for a while that CONFIG_BUG=n is a pretty dumb thing
to do, and that maintaining it (and trying to fix the warnings it
produces) aren't worth the effort and that we should remove the whole
thing.  Perhaps your patch changes that calculus, dunno.  Please discuss.

Re: [PATCH, re-send] Always trap on BUG()

[Russell King - ARM Linux]

On Mon, Jul 15, 2013 at 03:16:12PM -0700, Andrew Morton wrote:
> I've been thinking for a while that CONFIG_BUG=n is a pretty dumb thing
> to do, and that maintaining it (and trying to fix the warnings it
> produces) aren't worth the effort and that we should remove the whole
> thing.  Perhaps your patch changes that calculus, dunno.  Please discuss.

This isn't about introducing "CONFIG_BUG=n" - this is about making a
kernel with CONFIG_BUG=n build without producing tonnes and tonnes of
warnings, as it does today.  It makes building randconfig pretty
useless to find what could be more important warnings.

For example, this is a small extract from one such build log that
randconfig decided to set CONFIG_BUG to n:

fs/ext4/extents.c: In function 'ext4_inode_journal_mode':
fs/ext4/ext4_jbd2.h:415:1: warning: control reaches end of non-void function
fs/ext4/ext4_jbd2.c: In function 'ext4_inode_journal_mode':
fs/ext4/ext4_jbd2.h:415:1: warning: control reaches end of non-void function
fs/ext4/mballoc.c: In function 'ext4_inode_journal_mode':
fs/ext4/ext4_jbd2.h:415:1: warning: control reaches end of non-void function
fs/ext4/move_extent.c: In function 'ext4_inode_journal_mode':
fs/ext4/ext4_jbd2.h:415:1: warning: control reaches end of non-void function
fs/ext4/indirect.c: In function 'ext4_inode_journal_mode':
fs/ext4/ext4_jbd2.h:415:1: warning: control reaches end of non-void function
fs/ext4/inline.c: In function 'ext4_inode_journal_mode':
fs/ext4/ext4_jbd2.h:415:1: warning: control reaches end of non-void function
fs/ext4/acl.c: In function 'acl_by_type':
include/linux/posix_acl.h:108:1: warning: control reaches end of non-void function
fs/ext4/acl.c: In function 'ext4_get_acl':
fs/ext4/acl.c:150:6: warning: 'name_index' may be used uninitialized in this funtion

[PATCH, re-send] Always trap on BUG()

[Russell King - ARM Linux]

On Mon, Jul 15, 2013 at 03:16:12PM -0700, Andrew Morton wrote:
> I've been thinking for a while that CONFIG_BUG=n is a pretty dumb thing
> to do, and that maintaining it (and trying to fix the warnings it
> produces) aren't worth the effort and that we should remove the whole
> thing.  Perhaps your patch changes that calculus, dunno.  Please discuss.

This isn't about introducing "CONFIG_BUG=n" - this is about making a
kernel with CONFIG_BUG=n build without producing tonnes and tonnes of
warnings, as it does today.  It makes building randconfig pretty
useless to find what could be more important warnings.

For example, this is a small extract from one such build log that
randconfig decided to set CONFIG_BUG to n:

fs/ext4/extents.c: In function 'ext4_inode_journal_mode':
fs/ext4/ext4_jbd2.h:415:1: warning: control reaches end of non-void function
fs/ext4/ext4_jbd2.c: In function 'ext4_inode_journal_mode':
fs/ext4/ext4_jbd2.h:415:1: warning: control reaches end of non-void function
fs/ext4/mballoc.c: In function 'ext4_inode_journal_mode':
fs/ext4/ext4_jbd2.h:415:1: warning: control reaches end of non-void function
fs/ext4/move_extent.c: In function 'ext4_inode_journal_mode':
fs/ext4/ext4_jbd2.h:415:1: warning: control reaches end of non-void function
fs/ext4/indirect.c: In function 'ext4_inode_journal_mode':
fs/ext4/ext4_jbd2.h:415:1: warning: control reaches end of non-void function
fs/ext4/inline.c: In function 'ext4_inode_journal_mode':
fs/ext4/ext4_jbd2.h:415:1: warning: control reaches end of non-void function
fs/ext4/acl.c: In function 'acl_by_type':
include/linux/posix_acl.h:108:1: warning: control reaches end of non-void function
fs/ext4/acl.c: In function 'ext4_get_acl':
fs/ext4/acl.c:150:6: warning: 'name_index' may be used uninitialized in this funtion

Re: [PATCH, re-send] Always trap on BUG()

[H. Peter Anvin]

On 07/15/2013 03:27 PM, Russell King - ARM Linux wrote:
> On Mon, Jul 15, 2013 at 03:16:12PM -0700, Andrew Morton wrote:
>> I've been thinking for a while that CONFIG_BUG=n is a pretty dumb thing
>> to do, and that maintaining it (and trying to fix the warnings it
>> produces) aren't worth the effort and that we should remove the whole
>> thing.  Perhaps your patch changes that calculus, dunno.  Please discuss.
> 
> This isn't about introducing "CONFIG_BUG=n" - this is about making a
> kernel with CONFIG_BUG=n build without producing tonnes and tonnes of
> warnings, as it does today.  It makes building randconfig pretty
> useless to find what could be more important warnings.
> 

Well, there are three alternatives here, right:

1. We can use unreachable(), which means that the compiler can assume it
never happens.

2. We can trap without metadata.

3. We can trap with metadata (current CONFIG_BUG=y).

I am *guessing* this does 2, but it isn't clear.

	-hpa

Re: [PATCH, re-send] Always trap on BUG()

[H. Peter Anvin]

On 07/15/2013 03:27 PM, Russell King - ARM Linux wrote:
> On Mon, Jul 15, 2013 at 03:16:12PM -0700, Andrew Morton wrote:
>> I've been thinking for a while that CONFIG_BUG=n is a pretty dumb thing
>> to do, and that maintaining it (and trying to fix the warnings it
>> produces) aren't worth the effort and that we should remove the whole
>> thing.  Perhaps your patch changes that calculus, dunno.  Please discuss.
> 
> This isn't about introducing "CONFIG_BUG=n" - this is about making a
> kernel with CONFIG_BUG=n build without producing tonnes and tonnes of
> warnings, as it does today.  It makes building randconfig pretty
> useless to find what could be more important warnings.
> 

Well, there are three alternatives here, right:

1. We can use unreachable(), which means that the compiler can assume it
never happens.

2. We can trap without metadata.

3. We can trap with metadata (current CONFIG_BUG=y).

I am *guessing* this does 2, but it isn't clear.

	-hpa

[PATCH, re-send] Always trap on BUG()

[H. Peter Anvin]

On 07/15/2013 03:27 PM, Russell King - ARM Linux wrote:
> On Mon, Jul 15, 2013 at 03:16:12PM -0700, Andrew Morton wrote:
>> I've been thinking for a while that CONFIG_BUG=n is a pretty dumb thing
>> to do, and that maintaining it (and trying to fix the warnings it
>> produces) aren't worth the effort and that we should remove the whole
>> thing.  Perhaps your patch changes that calculus, dunno.  Please discuss.
> 
> This isn't about introducing "CONFIG_BUG=n" - this is about making a
> kernel with CONFIG_BUG=n build without producing tonnes and tonnes of
> warnings, as it does today.  It makes building randconfig pretty
> useless to find what could be more important warnings.
> 

Well, there are three alternatives here, right:

1. We can use unreachable(), which means that the compiler can assume it
never happens.

2. We can trap without metadata.

3. We can trap with metadata (current CONFIG_BUG=y).

I am *guessing* this does 2, but it isn't clear.

	-hpa

Re: [PATCH, re-send] Always trap on BUG()

[Russell King - ARM Linux]

On Mon, Jul 15, 2013 at 03:35:16PM -0700, H. Peter Anvin wrote:
> On 07/15/2013 03:27 PM, Russell King - ARM Linux wrote:
> > On Mon, Jul 15, 2013 at 03:16:12PM -0700, Andrew Morton wrote:
> >> I've been thinking for a while that CONFIG_BUG=n is a pretty dumb thing
> >> to do, and that maintaining it (and trying to fix the warnings it
> >> produces) aren't worth the effort and that we should remove the whole
> >> thing.  Perhaps your patch changes that calculus, dunno.  Please discuss.
> > 
> > This isn't about introducing "CONFIG_BUG=n" - this is about making a
> > kernel with CONFIG_BUG=n build without producing tonnes and tonnes of
> > warnings, as it does today.  It makes building randconfig pretty
> > useless to find what could be more important warnings.
> > 
> 
> Well, there are three alternatives here, right:

There's many more than three alternatives - some of them may not pass
your taste filter.

> 1. We can use unreachable(), which means that the compiler can assume it
> never happens.

Arnd tried that... see the commit message for the build results from
that.  Arnd included a whole boat-load of other solutions to this
problem and documented the build results there too.

What this actually means in reality is that you're giving permission for
the CPU to run off the end of a function and start executing whatever it
finds there - which could be literal tables.  That might cause some
memory to be corrupted but otherwise go by unnoticed until very much
later.

Infinite while loops on the other hand... probably a single instruction,
but if we're using a single instruction why not make it trap with an
exception anyway so that the system can panic() and maybe reboot without
needing to wait for the watchdog?

> 2. We can trap without metadata.
> 
> 3. We can trap with metadata (current CONFIG_BUG=y).
> 
> I am *guessing* this does 2, but it isn't clear.

That is what this paragraph in Arnd's mail conveys to me:

  This basically loses any of the BUG() reporting, but leaves the
  logic to trap and kill the task in place when CONFIG_BUG is disabled.

[PATCH, re-send] Always trap on BUG()

[Russell King - ARM Linux]

On Mon, Jul 15, 2013 at 03:35:16PM -0700, H. Peter Anvin wrote:
> On 07/15/2013 03:27 PM, Russell King - ARM Linux wrote:
> > On Mon, Jul 15, 2013 at 03:16:12PM -0700, Andrew Morton wrote:
> >> I've been thinking for a while that CONFIG_BUG=n is a pretty dumb thing
> >> to do, and that maintaining it (and trying to fix the warnings it
> >> produces) aren't worth the effort and that we should remove the whole
> >> thing.  Perhaps your patch changes that calculus, dunno.  Please discuss.
> > 
> > This isn't about introducing "CONFIG_BUG=n" - this is about making a
> > kernel with CONFIG_BUG=n build without producing tonnes and tonnes of
> > warnings, as it does today.  It makes building randconfig pretty
> > useless to find what could be more important warnings.
> > 
> 
> Well, there are three alternatives here, right:

There's many more than three alternatives - some of them may not pass
your taste filter.

> 1. We can use unreachable(), which means that the compiler can assume it
> never happens.

Arnd tried that... see the commit message for the build results from
that.  Arnd included a whole boat-load of other solutions to this
problem and documented the build results there too.

What this actually means in reality is that you're giving permission for
the CPU to run off the end of a function and start executing whatever it
finds there - which could be literal tables.  That might cause some
memory to be corrupted but otherwise go by unnoticed until very much
later.

Infinite while loops on the other hand... probably a single instruction,
but if we're using a single instruction why not make it trap with an
exception anyway so that the system can panic() and maybe reboot without
needing to wait for the watchdog?

> 2. We can trap without metadata.
> 
> 3. We can trap with metadata (current CONFIG_BUG=y).
> 
> I am *guessing* this does 2, but it isn't clear.

That is what this paragraph in Arnd's mail conveys to me:

  This basically loses any of the BUG() reporting, but leaves the
  logic to trap and kill the task in place when CONFIG_BUG is disabled.

Re: [PATCH, re-send] Always trap on BUG()

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> On 07/15/2013 03:27 PM, Russell King - ARM Linux wrote:
> > On Mon, Jul 15, 2013 at 03:16:12PM -0700, Andrew Morton wrote:
> >> I've been thinking for a while that CONFIG_BUG=n is a pretty dumb thing
> >> to do, and that maintaining it (and trying to fix the warnings it
> >> produces) aren't worth the effort and that we should remove the whole
> >> thing.  Perhaps your patch changes that calculus, dunno.  Please discuss.
> > 
> > This isn't about introducing "CONFIG_BUG=n" - this is about making a
> > kernel with CONFIG_BUG=n build without producing tonnes and tonnes of
> > warnings, as it does today.  It makes building randconfig pretty
> > useless to find what could be more important warnings.
> > 
> 
> Well, there are three alternatives here, right:
> 
> 1. We can use unreachable(), which means that the compiler can assume it
> never happens.

AFAICS this is dangerous as it loses warnings and moves execution into 
la-la-land without any obvious sign at the C level.

> 2. We can trap without metadata.

This is what the patch does.

> 3. We can trap with metadata (current CONFIG_BUG=y).

That is still kept with the patch.

> I am *guessing* this does 2, but it isn't clear.

Yes, that's what it does - and I think it's the best of all worlds:

Acked-by: Ingo Molnar <mingo@kernel.org>

(the crazies can keep a separate patch to remove even more of BUG() to win 
a K or two.)

Thanks,

	Ingo

[PATCH, re-send] Always trap on BUG()

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> On 07/15/2013 03:27 PM, Russell King - ARM Linux wrote:
> > On Mon, Jul 15, 2013 at 03:16:12PM -0700, Andrew Morton wrote:
> >> I've been thinking for a while that CONFIG_BUG=n is a pretty dumb thing
> >> to do, and that maintaining it (and trying to fix the warnings it
> >> produces) aren't worth the effort and that we should remove the whole
> >> thing.  Perhaps your patch changes that calculus, dunno.  Please discuss.
> > 
> > This isn't about introducing "CONFIG_BUG=n" - this is about making a
> > kernel with CONFIG_BUG=n build without producing tonnes and tonnes of
> > warnings, as it does today.  It makes building randconfig pretty
> > useless to find what could be more important warnings.
> > 
> 
> Well, there are three alternatives here, right:
> 
> 1. We can use unreachable(), which means that the compiler can assume it
> never happens.

AFAICS this is dangerous as it loses warnings and moves execution into 
la-la-land without any obvious sign at the C level.

> 2. We can trap without metadata.

This is what the patch does.

> 3. We can trap with metadata (current CONFIG_BUG=y).

That is still kept with the patch.

> I am *guessing* this does 2, but it isn't clear.

Yes, that's what it does - and I think it's the best of all worlds:

Acked-by: Ingo Molnar <mingo@kernel.org>

(the crazies can keep a separate patch to remove even more of BUG() to win 
a K or two.)

Thanks,

	Ingo

Re: [PATCH, re-send] Always trap on BUG()

[H. Peter Anvin]

On 07/23/2013 02:36 AM, Ingo Molnar wrote:
>>
>> Well, there are three alternatives here, right:
>>
>> 1. We can use unreachable(), which means that the compiler can assume it
>> never happens.
> 
> AFAICS this is dangerous as it loses warnings and moves execution into 
> la-la-land without any obvious sign at the C level.
> 

Extremely dangerous.

	-hpa

Re: [PATCH, re-send] Always trap on BUG()

[H. Peter Anvin]

On 07/23/2013 02:36 AM, Ingo Molnar wrote:
>>
>> Well, there are three alternatives here, right:
>>
>> 1. We can use unreachable(), which means that the compiler can assume it
>> never happens.
> 
> AFAICS this is dangerous as it loses warnings and moves execution into 
> la-la-land without any obvious sign at the C level.
> 

Extremely dangerous.

	-hpa

[PATCH, re-send] Always trap on BUG()

[H. Peter Anvin]

On 07/23/2013 02:36 AM, Ingo Molnar wrote:
>>
>> Well, there are three alternatives here, right:
>>
>> 1. We can use unreachable(), which means that the compiler can assume it
>> never happens.
> 
> AFAICS this is dangerous as it loses warnings and moves execution into 
> la-la-land without any obvious sign at the C level.
> 

Extremely dangerous.

	-hpa

Re: [PATCH, re-send] Always trap on BUG()

[Chen Gang]

On 07/23/2013 05:36 PM, Ingo Molnar wrote:
> 
> (the crazies can keep a separate patch to remove even more of BUG() to win 
> a K or two.)
> 

Excuse me, my English is not quite well, I do not quite understand your
meaning, could you please repeat again in details or say more clearly ?

Thanks.
-- 
Chen Gang

[PATCH, re-send] Always trap on BUG()

[Chen Gang]

On 07/23/2013 05:36 PM, Ingo Molnar wrote:
> 
> (the crazies can keep a separate patch to remove even more of BUG() to win 
> a K or two.)
> 

Excuse me, my English is not quite well, I do not quite understand your
meaning, could you please repeat again in details or say more clearly ?

Thanks.
-- 
Chen Gang

Re: [PATCH, re-send] Always trap on BUG()

[Ingo Molnar]

* Chen Gang <gang.chen@asianux.com> wrote:

> On 07/23/2013 05:36 PM, Ingo Molnar wrote:
> > 
> > (the crazies can keep a separate patch to remove even more of BUG() to win 
> > a K or two.)
> > 
> 
> Excuse me, my English is not quite well, I do not quite understand your
> meaning, could you please repeat again in details or say more clearly ?

I meant: those people who'd like to have the tiny 
additional space savings offered by !CONFIG_BUG (and are 
crazy enough to live with non-determinism in exchange for a 
measly payment of 1 KB), can apply a separate, out of tree 
patch just fine and don't need upstream kernel support.

Thanks,

	Ingo

[PATCH, re-send] Always trap on BUG()

[Ingo Molnar]

* Chen Gang <gang.chen@asianux.com> wrote:

> On 07/23/2013 05:36 PM, Ingo Molnar wrote:
> > 
> > (the crazies can keep a separate patch to remove even more of BUG() to win 
> > a K or two.)
> > 
> 
> Excuse me, my English is not quite well, I do not quite understand your
> meaning, could you please repeat again in details or say more clearly ?

I meant: those people who'd like to have the tiny 
additional space savings offered by !CONFIG_BUG (and are 
crazy enough to live with non-determinism in exchange for a 
measly payment of 1 KB), can apply a separate, out of tree 
patch just fine and don't need upstream kernel support.

Thanks,

	Ingo

Re: [PATCH, re-send] Always trap on BUG()

[Chen Gang]

On 07/26/2013 06:05 AM, Ingo Molnar wrote:
> * Chen Gang <gang.chen@asianux.com> wrote:
> 
>> On 07/23/2013 05:36 PM, Ingo Molnar wrote:
>>>
>>> (the crazies can keep a separate patch to remove even more of BUG() to win 
>>> a K or two.)
>>>
>>
>> Excuse me, my English is not quite well, I do not quite understand your
>> meaning, could you please repeat again in details or say more clearly ?
> 
> I meant: those people who'd like to have the tiny 
> additional space savings offered by !CONFIG_BUG (and are 
> crazy enough to live with non-determinism in exchange for a 
> measly payment of 1 KB), can apply a separate, out of tree 
> patch just fine and don't need upstream kernel support.
> 

Thank you for your reply in details.


Thanks
-- 
Chen Gang

[PATCH, re-send] Always trap on BUG()

[Chen Gang]

On 07/26/2013 06:05 AM, Ingo Molnar wrote:
> * Chen Gang <gang.chen@asianux.com> wrote:
> 
>> On 07/23/2013 05:36 PM, Ingo Molnar wrote:
>>>
>>> (the crazies can keep a separate patch to remove even more of BUG() to win 
>>> a K or two.)
>>>
>>
>> Excuse me, my English is not quite well, I do not quite understand your
>> meaning, could you please repeat again in details or say more clearly ?
> 
> I meant: those people who'd like to have the tiny 
> additional space savings offered by !CONFIG_BUG (and are 
> crazy enough to live with non-determinism in exchange for a 
> measly payment of 1 KB), can apply a separate, out of tree 
> patch just fine and don't need upstream kernel support.
> 

Thank you for your reply in details.


Thanks
-- 
Chen Gang