linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family
@ 2014-02-26  3:48 Josh Triplett
  2014-02-26  3:48 ` [PATCH v2 2/5] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/ Josh Triplett
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Josh Triplett @ 2014-02-26  3:48 UTC (permalink / raw)
  To: Andrew Morton, Arnd Bergmann, linux-arch, linux-kernel

When !CONFIG_BUG, WARN_ON and family become simple passthroughs of their
condition argument; however, WARN_ON_ONCE and family still have
conditions and a boolean to detect one-time invocation, even though the
warning they'd emit doesn't exist.  Make the existing definitions
conditional on CONFIG_BUG, and add definitions for !CONFIG_BUG that map
to the passthrough versions of WARN and WARN_ON.

This saves 4.4k on a minimized configuration (smaller than
allnoconfig), and 20.6k with defconfig plus CONFIG_BUG=n.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
v2: Incorporate feedback from Arnd Bergmann: make the WARN_* variants with
    format strings and arguments call WARN and pass along those arguments,
    rather than calling WARN_ON.  Used by the new patch 3, which makes the stub
    version of WARN call no_printk.

 include/asm-generic/bug.h | 57 +++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 7d10f96..7ecd398 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -106,33 +106,6 @@ extern void warn_slowpath_null(const char *file, const int line);
 	unlikely(__ret_warn_on);					\
 })
 
-#else /* !CONFIG_BUG */
-#ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
-#endif
-
-#ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while(0)
-#endif
-
-#ifndef HAVE_ARCH_WARN_ON
-#define WARN_ON(condition) ({						\
-	int __ret_warn_on = !!(condition);				\
-	unlikely(__ret_warn_on);					\
-})
-#endif
-
-#ifndef WARN
-#define WARN(condition, format...) ({					\
-	int __ret_warn_on = !!(condition);				\
-	unlikely(__ret_warn_on);					\
-})
-#endif
-
-#define WARN_TAINT(condition, taint, format...) WARN_ON(condition)
-
-#endif
-
 #define WARN_ON_ONCE(condition)	({				\
 	static bool __section(.data.unlikely) __warned;		\
 	int __ret_warn_once = !!(condition);			\
@@ -163,6 +136,36 @@ extern void warn_slowpath_null(const char *file, const int line);
 	unlikely(__ret_warn_once);				\
 })
 
+#else /* !CONFIG_BUG */
+#ifndef HAVE_ARCH_BUG
+#define BUG() do {} while(0)
+#endif
+
+#ifndef HAVE_ARCH_BUG_ON
+#define BUG_ON(condition) do { if (condition) ; } while(0)
+#endif
+
+#ifndef HAVE_ARCH_WARN_ON
+#define WARN_ON(condition) ({						\
+	int __ret_warn_on = !!(condition);				\
+	unlikely(__ret_warn_on);					\
+})
+#endif
+
+#ifndef WARN
+#define WARN(condition, format...) ({					\
+	int __ret_warn_on = !!(condition);				\
+	unlikely(__ret_warn_on);					\
+})
+#endif
+
+#define WARN_ON_ONCE(condition) WARN_ON(condition)
+#define WARN_ONCE(condition, format...) WARN(condition, format)
+#define WARN_TAINT(condition, taint, format...) WARN(condition, format)
+#define WARN_TAINT_ONCE(condition, taint, format...) WARN(condition, format)
+
+#endif
+
 /*
  * WARN_ON_SMP() is for cases that the warning is either
  * meaningless for !SMP or may even cause failures.
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 2/5] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/
  2014-02-26  3:48 [PATCH v2 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Josh Triplett
@ 2014-02-26  3:48 ` Josh Triplett
  2014-02-26 13:25   ` Arnd Bergmann
  2014-02-26  3:49 ` [PATCH v2 3/5] bug: When !CONFIG_BUG, make WARN call no_printk to check format and args Josh Triplett
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Josh Triplett @ 2014-02-26  3:48 UTC (permalink / raw)
  To: Andrew Morton, Arnd Bergmann, linux-arch, linux-kernel

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 include/asm-generic/bug.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 7ecd398..2d54d8d 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -52,7 +52,7 @@ struct bug_entry {
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
+#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
 #endif
 
 /*
@@ -138,11 +138,11 @@ extern void warn_slowpath_null(const char *file, const int line);
 
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
+#define BUG() do {} while (0)
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while(0)
+#define BUG_ON(condition) do { if (condition) ; } while (0)
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 3/5] bug: When !CONFIG_BUG, make WARN call no_printk to check format and args
  2014-02-26  3:48 [PATCH v2 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Josh Triplett
  2014-02-26  3:48 ` [PATCH v2 2/5] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/ Josh Triplett
@ 2014-02-26  3:49 ` Josh Triplett
  2014-02-26 13:25   ` Arnd Bergmann
  2014-02-26  3:49 ` [PATCH v2 4/5] bug: Use a common definition of BUG_ON regardless of CONFIG_BUG Josh Triplett
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Josh Triplett @ 2014-02-26  3:49 UTC (permalink / raw)
  To: Andrew Morton, Arnd Bergmann, linux-arch, linux-kernel

The stub version of WARN for !CONFIG_BUG completely ignored its format
string and subsequent arguments; make it check them instead, using
no_printk.

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 include/asm-generic/bug.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 2d54d8d..a97fa11 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -155,6 +155,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 #ifndef WARN
 #define WARN(condition, format...) ({					\
 	int __ret_warn_on = !!(condition);				\
+	no_printk(format);						\
 	unlikely(__ret_warn_on);					\
 })
 #endif
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 4/5] bug: Use a common definition of BUG_ON regardless of CONFIG_BUG
  2014-02-26  3:48 [PATCH v2 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Josh Triplett
  2014-02-26  3:48 ` [PATCH v2 2/5] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/ Josh Triplett
  2014-02-26  3:49 ` [PATCH v2 3/5] bug: When !CONFIG_BUG, make WARN call no_printk to check format and args Josh Triplett
@ 2014-02-26  3:49 ` Josh Triplett
  2014-02-26 13:26   ` Arnd Bergmann
  2014-02-26  3:49 ` [PATCH v2 5/5] bug: Make BUG() call unreachable() Josh Triplett
  2014-02-26 13:24 ` [PATCH v2 " Arnd Bergmann
  4 siblings, 1 reply; 21+ messages in thread
From: Josh Triplett @ 2014-02-26  3:49 UTC (permalink / raw)
  To: Andrew Morton, Arnd Bergmann, linux-arch, linux-kernel

include/asm-generic/bug.h defines BUG_ON to call BUG() if CONFIG_BUG=y,
or as a no-op if !CONFIG_BUG.  However, BUG() is already a no-op if
!CONFIG_BUG, making this pointless.  Use a common definition that always
calls BUG().

This does not change the compiled code at all.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 include/asm-generic/bug.h | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index a97fa11..653c44a 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -51,10 +51,6 @@ struct bug_entry {
 } while (0)
 #endif
 
-#ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
-#endif
-
 /*
  * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
  * significant issues that need prompt attention if they should ever
@@ -141,10 +137,6 @@ extern void warn_slowpath_null(const char *file, const int line);
 #define BUG() do {} while (0)
 #endif
 
-#ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while (0)
-#endif
-
 #ifndef HAVE_ARCH_WARN_ON
 #define WARN_ON(condition) ({						\
 	int __ret_warn_on = !!(condition);				\
@@ -167,6 +159,10 @@ extern void warn_slowpath_null(const char *file, const int line);
 
 #endif
 
+#ifndef HAVE_ARCH_BUG_ON
+#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
+#endif
+
 /*
  * WARN_ON_SMP() is for cases that the warning is either
  * meaningless for !SMP or may even cause failures.
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 5/5] bug: Make BUG() call unreachable()
  2014-02-26  3:48 [PATCH v2 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Josh Triplett
                   ` (2 preceding siblings ...)
  2014-02-26  3:49 ` [PATCH v2 4/5] bug: Use a common definition of BUG_ON regardless of CONFIG_BUG Josh Triplett
@ 2014-02-26  3:49 ` Josh Triplett
  2014-02-26 13:29   ` Arnd Bergmann
  2014-02-26 13:24 ` [PATCH v2 " Arnd Bergmann
  4 siblings, 1 reply; 21+ messages in thread
From: Josh Triplett @ 2014-02-26  3:49 UTC (permalink / raw)
  To: Andrew Morton, Arnd Bergmann, linux-arch, linux-kernel

This effectively causes BUG() to act like a function with the noreturn
attribute, which prevents GCC from warning about the code that follows
BUG() (for instance, warning about not returning a value in a non-void
function after calling BUG()).

This actually makes the kernel smaller; bloat-o-meter summary:
add/remove: 2/7 grow/shrink: 34/57 up/down: 475/-1233 (-758)

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 include/asm-generic/bug.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 653c44a..5f69248 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -134,7 +134,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while (0)
+#define BUG() do { unreachable(); } while (0)
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family
  2014-02-26  3:48 [PATCH v2 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Josh Triplett
                   ` (3 preceding siblings ...)
  2014-02-26  3:49 ` [PATCH v2 5/5] bug: Make BUG() call unreachable() Josh Triplett
@ 2014-02-26 13:24 ` Arnd Bergmann
  4 siblings, 0 replies; 21+ messages in thread
From: Arnd Bergmann @ 2014-02-26 13:24 UTC (permalink / raw)
  To: Josh Triplett; +Cc: Andrew Morton, linux-arch, linux-kernel

On Wednesday 26 February 2014, Josh Triplett wrote:
> When !CONFIG_BUG, WARN_ON and family become simple passthroughs of their
> condition argument; however, WARN_ON_ONCE and family still have
> conditions and a boolean to detect one-time invocation, even though the
> warning they'd emit doesn't exist.  Make the existing definitions
> conditional on CONFIG_BUG, and add definitions for !CONFIG_BUG that map
> to the passthrough versions of WARN and WARN_ON.
> 
> This saves 4.4k on a minimized configuration (smaller than
> allnoconfig), and 20.6k with defconfig plus CONFIG_BUG=n.
> 
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/5] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/
  2014-02-26  3:48 ` [PATCH v2 2/5] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/ Josh Triplett
@ 2014-02-26 13:25   ` Arnd Bergmann
  0 siblings, 0 replies; 21+ messages in thread
From: Arnd Bergmann @ 2014-02-26 13:25 UTC (permalink / raw)
  To: Josh Triplett; +Cc: Andrew Morton, linux-arch, linux-kernel

On Wednesday 26 February 2014, Josh Triplett wrote:
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 3/5] bug: When !CONFIG_BUG, make WARN call no_printk to check format and args
  2014-02-26  3:49 ` [PATCH v2 3/5] bug: When !CONFIG_BUG, make WARN call no_printk to check format and args Josh Triplett
@ 2014-02-26 13:25   ` Arnd Bergmann
  0 siblings, 0 replies; 21+ messages in thread
From: Arnd Bergmann @ 2014-02-26 13:25 UTC (permalink / raw)
  To: Josh Triplett; +Cc: Andrew Morton, linux-arch, linux-kernel

On Wednesday 26 February 2014, Josh Triplett wrote:
> The stub version of WARN for !CONFIG_BUG completely ignored its format
> string and subsequent arguments; make it check them instead, using
> no_printk.
> 
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/5] bug: Use a common definition of BUG_ON regardless of CONFIG_BUG
  2014-02-26  3:49 ` [PATCH v2 4/5] bug: Use a common definition of BUG_ON regardless of CONFIG_BUG Josh Triplett
@ 2014-02-26 13:26   ` Arnd Bergmann
  0 siblings, 0 replies; 21+ messages in thread
From: Arnd Bergmann @ 2014-02-26 13:26 UTC (permalink / raw)
  To: Josh Triplett; +Cc: Andrew Morton, linux-arch, linux-kernel

On Wednesday 26 February 2014, Josh Triplett wrote:
> 
> include/asm-generic/bug.h defines BUG_ON to call BUG() if CONFIG_BUG=y,
> or as a no-op if !CONFIG_BUG.  However, BUG() is already a no-op if
> !CONFIG_BUG, making this pointless.  Use a common definition that always
> calls BUG().
> 
> This does not change the compiled code at all.
> 
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 5/5] bug: Make BUG() call unreachable()
  2014-02-26  3:49 ` [PATCH v2 5/5] bug: Make BUG() call unreachable() Josh Triplett
@ 2014-02-26 13:29   ` Arnd Bergmann
  2014-02-26 14:58     ` Josh Triplett
  0 siblings, 1 reply; 21+ messages in thread
From: Arnd Bergmann @ 2014-02-26 13:29 UTC (permalink / raw)
  To: Josh Triplett; +Cc: Andrew Morton, linux-arch, linux-kernel

On Wednesday 26 February 2014, Josh Triplett wrote:
> @@ -134,7 +134,7 @@ extern void warn_slowpath_null(const char *file, const int line);
>  
>  #else /* !CONFIG_BUG */
>  #ifndef HAVE_ARCH_BUG
> -#define BUG() do {} while (0)
> +#define BUG() do { unreachable(); } while (0)
>  #endif

I disagree with this one. As Alan said, we really want to use an
arch specific BUG() even in the !CONFIG_BUG case.

For the cases where this is not yet possible, I'd suggest using

#define BUG() do { } while (1)


For older gcc versions, this is actually the same as unreachable(), but
it is not the same as __builtin_unreachable, because it causes undefined
behavior.

	Arnd

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 5/5] bug: Make BUG() call unreachable()
  2014-02-26 13:29   ` Arnd Bergmann
@ 2014-02-26 14:58     ` Josh Triplett
  2014-02-27 19:19       ` Arnd Bergmann
  0 siblings, 1 reply; 21+ messages in thread
From: Josh Triplett @ 2014-02-26 14:58 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: Andrew Morton, linux-arch, linux-kernel

On Wed, Feb 26, 2014 at 02:29:06PM +0100, Arnd Bergmann wrote:
> On Wednesday 26 February 2014, Josh Triplett wrote:
> > @@ -134,7 +134,7 @@ extern void warn_slowpath_null(const char *file, const int line);
> >  
> >  #else /* !CONFIG_BUG */
> >  #ifndef HAVE_ARCH_BUG
> > -#define BUG() do {} while (0)
> > +#define BUG() do { unreachable(); } while (0)
> >  #endif
> 
> I disagree with this one. As Alan said, we really want to use an
> arch specific BUG() even in the !CONFIG_BUG case.

Possibly, but when doing so on an arch-by-arch basis, we'd need to make
sure that either there's a sensible trap handler for whatever trap it
invokes (for instance, ud2), or that there's some kind of useful
behavior otherwise (for instance, a reboot).  Hence why I didn't make
any attempt to add architecture-specific patches in this series.

In any case, I tried a quick test of that on x86 below, along with the
generic equivalent.

> For the cases where this is not yet possible, I'd suggest using
> 
> #define BUG() do { } while (1)

I just tested this, and:

$ scripts/bloat-o-meter vmlinux-nobug-base vmlinux-nobug-loop
add/remove: 2/1 grow/shrink: 247/33 up/down: 5461/-604 (4857)

In particular:

$ scripts/bloat-o-meter vmlinux-nobug-unreachable vmlinux-nobug-loop
add/remove: 6/0 grow/shrink: 261/9 up/down: 5679/-64 (5615)

So, some functions do get the optimizations from GCC treating the code
after an infinite loop as unreachable, but overall the infinite loops
themselves (and the conditionals around them in the more common case of
BUG_ON) are non-trivially large.

I tried a quick hack that used:
#define BUG() do { asm("ud2"); unreachable(); } while (0)
in place of the infinite loop, and got:

$ scripts/bloat-o-meter vmlinux-nobug-base vmlinux-nobug-ud2-unreachable
add/remove: 2/1 grow/shrink: 250/37 up/down: 4874/-606 (4268)

So, very little savings there compared to the infinite loop (unsurprising,
since ud2 is two bytes, and so is "1: jmp 1b").

This doesn't seem any different than compiling out assert() at runtime
in a userspace program, given how the kernel uses BUG() and BUG_ON().
I'd argue that adding unreachable() doesn't seem like it makes the
current implementation of BUG() any worse; either way if you reach it
you have a problem.

- Josh Triplett

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 5/5] bug: Make BUG() call unreachable()
  2014-02-26 14:58     ` Josh Triplett
@ 2014-02-27 19:19       ` Arnd Bergmann
  2014-02-28  0:16         ` Josh Triplett
  0 siblings, 1 reply; 21+ messages in thread
From: Arnd Bergmann @ 2014-02-27 19:19 UTC (permalink / raw)
  To: Josh Triplett; +Cc: Andrew Morton, linux-arch, linux-kernel

On Wednesday 26 February 2014, Josh Triplett wrote:
> This doesn't seem any different than compiling out assert() at runtime
> in a userspace program, given how the kernel uses BUG() and BUG_ON().
> I'd argue that adding unreachable() doesn't seem like it makes the
> current implementation of BUG() any worse; either way if you reach it
> you have a problem.

I think it's better to get a warning about undefined behavior than
to suppress that warning.

	Arnd

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 5/5] bug: Make BUG() call unreachable()
  2014-02-27 19:19       ` Arnd Bergmann
@ 2014-02-28  0:16         ` Josh Triplett
  2014-02-28  8:55           ` Arnd Bergmann
  0 siblings, 1 reply; 21+ messages in thread
From: Josh Triplett @ 2014-02-28  0:16 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: Andrew Morton, linux-arch, linux-kernel

On Thu, Feb 27, 2014 at 08:19:47PM +0100, Arnd Bergmann wrote:
> On Wednesday 26 February 2014, Josh Triplett wrote:
> > This doesn't seem any different than compiling out assert() at runtime
> > in a userspace program, given how the kernel uses BUG() and BUG_ON().
> > I'd argue that adding unreachable() doesn't seem like it makes the
> > current implementation of BUG() any worse; either way if you reach it
> > you have a problem.
> 
> I think it's better to get a warning about undefined behavior than
> to suppress that warning.

Then at this point I'm going to suggest that you go ahead and submit the
patch you want on top of the first four patches of this series.  Please
keep in mind the value and code size savings of !CONFIG_BUG, versus
CONFIG_BUG=y and !CONFIG_DEBUG_BUGVERBOSE; those mean two different
things.

Meanwhile: Andrew, could you go ahead and apply the first four patches?

- Josh Triplett

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 5/5] bug: Make BUG() call unreachable()
  2014-02-28  0:16         ` Josh Triplett
@ 2014-02-28  8:55           ` Arnd Bergmann
  2014-03-10  1:00             ` [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Josh Triplett
  0 siblings, 1 reply; 21+ messages in thread
From: Arnd Bergmann @ 2014-02-28  8:55 UTC (permalink / raw)
  To: Josh Triplett; +Cc: Andrew Morton, linux-arch, linux-kernel

On Thursday 27 February 2014 16:16:45 Josh Triplett wrote:
> On Thu, Feb 27, 2014 at 08:19:47PM +0100, Arnd Bergmann wrote:
> > On Wednesday 26 February 2014, Josh Triplett wrote:
> > > This doesn't seem any different than compiling out assert() at runtime
> > > in a userspace program, given how the kernel uses BUG() and BUG_ON().
> > > I'd argue that adding unreachable() doesn't seem like it makes the
> > > current implementation of BUG() any worse; either way if you reach it
> > > you have a problem.
> > 
> > I think it's better to get a warning about undefined behavior than
> > to suppress that warning.
> 
> Then at this point I'm going to suggest that you go ahead and submit the
> patch you want on top of the first four patches of this series.

Sure, no problem. I'll wait for your patches to show up in linux-next
and then do a patch on top. I'll be traveling for the next week, so
it may get delayed another few days.

> Please keep in mind the value and code size savings of !CONFIG_BUG, versus
> CONFIG_BUG=y and !CONFIG_DEBUG_BUGVERBOSE; those mean two different
> things.

I think I compared all the options before in the patch I cited,
https://lkml.org/lkml/2013/7/5/222 but I agree that the list
is a bit confusing.

> Meanwhile: Andrew, could you go ahead and apply the first four patches?

Yes please.

	Arnd

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family
  2014-02-28  8:55           ` Arnd Bergmann
@ 2014-03-10  1:00             ` Josh Triplett
  2014-03-10  1:01               ` [PATCH v3 2/5] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/ Josh Triplett
                                 ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Josh Triplett @ 2014-03-10  1:00 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Arnd Bergmann, linux-arch, linux-kernel

When !CONFIG_BUG, WARN_ON and family become simple passthroughs of their
condition argument; however, WARN_ON_ONCE and family still have
conditions and a boolean to detect one-time invocation, even though the
warning they'd emit doesn't exist.  Make the existing definitions
conditional on CONFIG_BUG, and add definitions for !CONFIG_BUG that map
to the passthrough versions of WARN and WARN_ON.

This saves 4.4k on a minimized configuration (smaller than
allnoconfig), and 20.6k with defconfig plus CONFIG_BUG=n.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
v3: Patch unchanged from v2.

Andrew, can you please replace the entire v2 series currently in -mm
with this new series?

 include/asm-generic/bug.h | 57 +++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 7d10f96..7ecd398 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -106,33 +106,6 @@ extern void warn_slowpath_null(const char *file, const int line);
 	unlikely(__ret_warn_on);					\
 })
 
-#else /* !CONFIG_BUG */
-#ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
-#endif
-
-#ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while(0)
-#endif
-
-#ifndef HAVE_ARCH_WARN_ON
-#define WARN_ON(condition) ({						\
-	int __ret_warn_on = !!(condition);				\
-	unlikely(__ret_warn_on);					\
-})
-#endif
-
-#ifndef WARN
-#define WARN(condition, format...) ({					\
-	int __ret_warn_on = !!(condition);				\
-	unlikely(__ret_warn_on);					\
-})
-#endif
-
-#define WARN_TAINT(condition, taint, format...) WARN_ON(condition)
-
-#endif
-
 #define WARN_ON_ONCE(condition)	({				\
 	static bool __section(.data.unlikely) __warned;		\
 	int __ret_warn_once = !!(condition);			\
@@ -163,6 +136,36 @@ extern void warn_slowpath_null(const char *file, const int line);
 	unlikely(__ret_warn_once);				\
 })
 
+#else /* !CONFIG_BUG */
+#ifndef HAVE_ARCH_BUG
+#define BUG() do {} while(0)
+#endif
+
+#ifndef HAVE_ARCH_BUG_ON
+#define BUG_ON(condition) do { if (condition) ; } while(0)
+#endif
+
+#ifndef HAVE_ARCH_WARN_ON
+#define WARN_ON(condition) ({						\
+	int __ret_warn_on = !!(condition);				\
+	unlikely(__ret_warn_on);					\
+})
+#endif
+
+#ifndef WARN
+#define WARN(condition, format...) ({					\
+	int __ret_warn_on = !!(condition);				\
+	unlikely(__ret_warn_on);					\
+})
+#endif
+
+#define WARN_ON_ONCE(condition) WARN_ON(condition)
+#define WARN_ONCE(condition, format...) WARN(condition, format)
+#define WARN_TAINT(condition, taint, format...) WARN(condition, format)
+#define WARN_TAINT_ONCE(condition, taint, format...) WARN(condition, format)
+
+#endif
+
 /*
  * WARN_ON_SMP() is for cases that the warning is either
  * meaningless for !SMP or may even cause failures.
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 2/5] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/
  2014-03-10  1:00             ` [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Josh Triplett
@ 2014-03-10  1:01               ` Josh Triplett
  2014-03-10  1:02               ` [PATCH v3 3/5] bug: When !CONFIG_BUG, make WARN call no_printk to check format and args Josh Triplett
                                 ` (3 subsequent siblings)
  4 siblings, 0 replies; 21+ messages in thread
From: Josh Triplett @ 2014-03-10  1:01 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Arnd Bergmann, linux-arch, linux-kernel

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
v3: Patch unchanged from v2.
 include/asm-generic/bug.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 7ecd398..2d54d8d 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -52,7 +52,7 @@ struct bug_entry {
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
+#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
 #endif
 
 /*
@@ -138,11 +138,11 @@ extern void warn_slowpath_null(const char *file, const int line);
 
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
+#define BUG() do {} while (0)
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while(0)
+#define BUG_ON(condition) do { if (condition) ; } while (0)
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 3/5] bug: When !CONFIG_BUG, make WARN call no_printk to check format and args
  2014-03-10  1:00             ` [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Josh Triplett
  2014-03-10  1:01               ` [PATCH v3 2/5] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/ Josh Triplett
@ 2014-03-10  1:02               ` Josh Triplett
  2014-03-10  1:02               ` [PATCH v3 4/5] bug: Make BUG() always stop the machine Josh Triplett
                                 ` (2 subsequent siblings)
  4 siblings, 0 replies; 21+ messages in thread
From: Josh Triplett @ 2014-03-10  1:02 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Arnd Bergmann, linux-arch, linux-kernel

The stub version of WARN for !CONFIG_BUG completely ignored its format
string and subsequent arguments; make it check them instead, using
no_printk.

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
v3: Patch unchanged from v2.
 include/asm-generic/bug.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 2d54d8d..a97fa11 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -155,6 +155,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 #ifndef WARN
 #define WARN(condition, format...) ({					\
 	int __ret_warn_on = !!(condition);				\
+	no_printk(format);						\
 	unlikely(__ret_warn_on);					\
 })
 #endif
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 4/5] bug: Make BUG() always stop the machine
  2014-03-10  1:00             ` [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Josh Triplett
  2014-03-10  1:01               ` [PATCH v3 2/5] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/ Josh Triplett
  2014-03-10  1:02               ` [PATCH v3 3/5] bug: When !CONFIG_BUG, make WARN call no_printk to check format and args Josh Triplett
@ 2014-03-10  1:02               ` Josh Triplett
  2014-03-10  1:03               ` [PATCH v3 5/5] x86: Always define BUG() and HAVE_ARCH_BUG, even with !CONFIG_BUG Josh Triplett
  2014-03-11 16:40               ` [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Arnd Bergmann
  4 siblings, 0 replies; 21+ messages in thread
From: Josh Triplett @ 2014-03-10  1:02 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Arnd Bergmann, linux-arch, linux-kernel

When !CONFIG_BUG and !HAVE_ARCH_BUG, define the generic BUG() as an
infinite loop rather than a no-op.  This avoids undefined behavior if
execution ever actually reaches BUG(), and avoids warnings about code
after BUG() (such as on non-void functions calling BUG() and then
not returning).

bloat-o-meter results:

add/remove: 0/0 grow/shrink: 43/10 up/down: 235/-98 (137)
function                                     old     new   delta
umount_collect                               119     138     +19
notify_change                                306     324     +18
xstate_enable_boot_cpu                       252     269     +17
kunmap                                        54      70     +16
balloon_page_dequeue                         112     126     +14
mm_take_all_locks                            223     233     +10
list_lru_walk_node                           143     152      +9
vma_adjust                                  1059    1067      +8
pcpu_setup_first_chunk                      1130    1138      +8
mm_drop_all_locks                            143     151      +8
ns_capable                                    55      62      +7
anon_transport_class_unregister                8      15      +7
srcu_init_notifier_head                       35      41      +6
shrink_dcache_for_umount                     174     180      +6
kunmap_high                                   99     105      +6
end_page_writeback                            43      49      +6
do_exit                                     1339    1345      +6
__kfifo_dma_out_prepare_r                     86      92      +6
__kfifo_dma_in_prepare_r                      90      96      +6
fixup_user_fault                             120     125      +5
repair_env_string                             73      77      +4
read_cache_pages_invalidate_page              56      60      +4
isolate_lru_pages.isra                       142     146      +4
do_notify_parent_cldstop                     255     259      +4
cpu_init                                     370     374      +4
utimes_common                                270     272      +2
tasklet_hi_action                             91      93      +2
tasklet_action                                91      93      +2
set_pte_vaddr                                 46      48      +2
find_get_pages_tag                           202     204      +2
early_iounmap                                185     187      +2
__native_set_fixmap                           36      38      +2
__get_user_pages                             822     824      +2
__early_ioremap                              299     301      +2
yield_task_stop                                1       2      +1
tick_resume                                   37      38      +1
switched_to_stop                               1       2      +1
switched_to_idle                               1       2      +1
prio_changed_stop                              1       2      +1
prio_changed_idle                              1       2      +1
pm_qos_power_read                            111     112      +1
arch_cpu_idle_dead                             1       2      +1
__insert_vmap_area                           140     141      +1
sys_renameat                                 614     612      -2
mm_fault_error                               297     295      -2
SyS_renameat                                 614     612      -2
sys_linkat                                   416     413      -3
SyS_linkat                                   416     413      -3
chmod_common                                 129     122      -7
proc_cap_handler                             240     225     -15
__schedule                                   849     831     -18
sys_madvise                                 1077    1054     -23
SyS_madvise                                 1077    1054     -23

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
v3: New patch in this series, incorporating Arnd's suggestion to make BUG()
always stop execution.  This eliminates the new warnings currently appearing in
allnoconfig due to !CONFIG_BUG; unlike v2, this avoids using unreachable(), and
thus avoids the possibility of undefined behvior if execution actually reaches
a call to BUG().

 include/asm-generic/bug.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index a97fa11..630dd23 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -138,7 +138,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while (0)
+#define BUG() do {} while (1)
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 5/5] x86: Always define BUG() and HAVE_ARCH_BUG, even with !CONFIG_BUG
  2014-03-10  1:00             ` [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Josh Triplett
                                 ` (2 preceding siblings ...)
  2014-03-10  1:02               ` [PATCH v3 4/5] bug: Make BUG() always stop the machine Josh Triplett
@ 2014-03-10  1:03               ` Josh Triplett
  2014-03-11 16:40               ` [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Arnd Bergmann
  4 siblings, 0 replies; 21+ messages in thread
From: Josh Triplett @ 2014-03-10  1:03 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Arnd Bergmann, linux-arch, linux-kernel

This ensures that BUG() always has a definition that causes a trap (via
an undefined instruction), and that the compiler still recognizes the
code following BUG() as unreachable, avoiding warnings that would
otherwise appear (such as on non-void functions that don't return a
value after BUG()).

In addition to saving a few bytes over the generic infinite-loop
implementation, this implementation traps rather than looping, which
potentially allows for better error-recovery behavior (such as by
rebooting).

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
v3: New patch for x86-optimized implementation of a simple bug for
!CONFIG_BUG, based on Arnd Bergmann's proposal.

 arch/x86/include/asm/bug.h | 3 ---
 1 file changed, 3 deletions(-)

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 */
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family
  2014-03-10  1:00             ` [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Josh Triplett
                                 ` (3 preceding siblings ...)
  2014-03-10  1:03               ` [PATCH v3 5/5] x86: Always define BUG() and HAVE_ARCH_BUG, even with !CONFIG_BUG Josh Triplett
@ 2014-03-11 16:40               ` Arnd Bergmann
  2014-03-11 17:49                 ` Josh Triplett
  4 siblings, 1 reply; 21+ messages in thread
From: Arnd Bergmann @ 2014-03-11 16:40 UTC (permalink / raw)
  To: Josh Triplett; +Cc: Andrew Morton, linux-arch, linux-kernel

On Monday 10 March 2014, Josh Triplett wrote:
> 
> When !CONFIG_BUG, WARN_ON and family become simple passthroughs of their
> condition argument; however, WARN_ON_ONCE and family still have
> conditions and a boolean to detect one-time invocation, even though the
> warning they'd emit doesn't exist.  Make the existing definitions
> conditional on CONFIG_BUG, and add definitions for !CONFIG_BUG that map
> to the passthrough versions of WARN and WARN_ON.
> 
> This saves 4.4k on a minimized configuration (smaller than
> allnoconfig), and 20.6k with defconfig plus CONFIG_BUG=n.
> 
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
> v3: Patch unchanged from v2.
> 
> Andrew, can you please replace the entire v2 series currently in -mm
> with this new series?


[all patches]
Acked-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family
  2014-03-11 16:40               ` [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Arnd Bergmann
@ 2014-03-11 17:49                 ` Josh Triplett
  0 siblings, 0 replies; 21+ messages in thread
From: Josh Triplett @ 2014-03-11 17:49 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: Andrew Morton, linux-arch, linux-kernel

On Tue, Mar 11, 2014 at 05:40:25PM +0100, Arnd Bergmann wrote:
> On Monday 10 March 2014, Josh Triplett wrote:
> > 
> > When !CONFIG_BUG, WARN_ON and family become simple passthroughs of their
> > condition argument; however, WARN_ON_ONCE and family still have
> > conditions and a boolean to detect one-time invocation, even though the
> > warning they'd emit doesn't exist.  Make the existing definitions
> > conditional on CONFIG_BUG, and add definitions for !CONFIG_BUG that map
> > to the passthrough versions of WARN and WARN_ON.
> > 
> > This saves 4.4k on a minimized configuration (smaller than
> > allnoconfig), and 20.6k with defconfig plus CONFIG_BUG=n.
> > 
> > Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> > ---
> > v3: Patch unchanged from v2.
> > 
> > Andrew, can you please replace the entire v2 series currently in -mm
> > with this new series?
> 
> 
> [all patches]
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Thanks!

- Josh Triplett

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2014-03-11 17:49 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-26  3:48 [PATCH v2 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Josh Triplett
2014-02-26  3:48 ` [PATCH v2 2/5] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/ Josh Triplett
2014-02-26 13:25   ` Arnd Bergmann
2014-02-26  3:49 ` [PATCH v2 3/5] bug: When !CONFIG_BUG, make WARN call no_printk to check format and args Josh Triplett
2014-02-26 13:25   ` Arnd Bergmann
2014-02-26  3:49 ` [PATCH v2 4/5] bug: Use a common definition of BUG_ON regardless of CONFIG_BUG Josh Triplett
2014-02-26 13:26   ` Arnd Bergmann
2014-02-26  3:49 ` [PATCH v2 5/5] bug: Make BUG() call unreachable() Josh Triplett
2014-02-26 13:29   ` Arnd Bergmann
2014-02-26 14:58     ` Josh Triplett
2014-02-27 19:19       ` Arnd Bergmann
2014-02-28  0:16         ` Josh Triplett
2014-02-28  8:55           ` Arnd Bergmann
2014-03-10  1:00             ` [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Josh Triplett
2014-03-10  1:01               ` [PATCH v3 2/5] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/ Josh Triplett
2014-03-10  1:02               ` [PATCH v3 3/5] bug: When !CONFIG_BUG, make WARN call no_printk to check format and args Josh Triplett
2014-03-10  1:02               ` [PATCH v3 4/5] bug: Make BUG() always stop the machine Josh Triplett
2014-03-10  1:03               ` [PATCH v3 5/5] x86: Always define BUG() and HAVE_ARCH_BUG, even with !CONFIG_BUG Josh Triplett
2014-03-11 16:40               ` [PATCH v3 1/5] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family Arnd Bergmann
2014-03-11 17:49                 ` Josh Triplett
2014-02-26 13:24 ` [PATCH v2 " Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).