Home » Mailing list archives » linux-kernel » 2007 » October » 11

Re: [PATCH 2/2] powerpc: Switch to generic WARN_ON()/BUG_ON()

Previous thread: [PATCH try #3] Blackfin BF54x Input Keypad controller driver by Bryan Wu on Thursday, October 11, 2007 - 9:28 am. (2 messages)

Next thread: [RFC] mm - background_writeout exits when pages_skipped ? by richard kennedy on Thursday, October 11, 2007 - 10:19 am. (2 messages)
[view original message]
From: Olof Johansson
Subject: [PATCH 1/2] bug.h: Introduce HAVE_ARCH_WARN
Date: Thursday, October 11, 2007 - 10:12 am

HAVE_ARCH_WARN is used to determine if an arch already has a __WARN()
macro, or if a generic one is needed.

With this, some of the arch-specific WARN_ON() implementations can be
made common instead (see follow-up patch for powerpc).

Signed-off-by: Olof Johansson <olof@lixom.net>

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index d56fedb..c6b8386 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -31,14 +31,19 @@ struct bug_entry {
 #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
 #endif
 
+#ifndef HAVE_ARCH_WARN
+#define __WARN() do {							\
+	printk("WARNING: at %s:%d %s()\n", __FILE__,			\
+		__LINE__, __FUNCTION__);				\
+	dump_stack();							\
+} while (0)
+#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();						\
-	}								\
+	if (unlikely(__ret_warn_on))					\
+		__WARN();						\
 	unlikely(__ret_warn_on);					\
 })
 #endif
diff --git a/include/asm-parisc/bug.h b/include/asm-parisc/bug.h
index 8cfc553..3f68100 100644
--- a/include/asm-parisc/bug.h
+++ b/include/asm-parisc/bug.h
@@ -8,6 +8,7 @@
 
 #ifdef CONFIG_BUG
 #define HAVE_ARCH_BUG
+#define HAVE_ARCH_WARN
 #define HAVE_ARCH_WARN_ON
 
 /* the break instruction is used as BUG() marker.  */
diff --git a/include/asm-powerpc/bug.h b/include/asm-powerpc/bug.h
index e55d1f6..02e171c 100644
--- a/include/asm-powerpc/bug.h
+++ b/include/asm-powerpc/bug.h
@@ -110,6 +110,7 @@
 })
 
 #define HAVE_ARCH_BUG
+#define HAVE_ARCH_WARN
 #define HAVE_ARCH_BUG_ON
 #define HAVE_ARCH_WARN_ON
 #endif /* __ASSEMBLY __ */
diff --git a/include/asm-sh/bug.h b/include/asm-sh/bug.h
index a78d482..4ea415b 100644
--- a/include/asm-sh/bug.h
+++ b/include/asm-sh/bug.h
@@ -5,6 +5,7 @@
 
 #ifdef CONFIG_BUG
 #define HAVE_ARCH_BUG
+#define ...
[ message continues ]
[view original message]
From: Olof Johansson
Subject: [PATCH 2/2] powerpc: Switch to generic WARN_ON()/BUG_ON()
Date: Thursday, October 11, 2007 - 10:14 am

Not using the ppc-specific WARN_ON/BUG_ON constructs actually saves about
4K text on a ppc64_defconfig. The main reason seems to be that prepping
the arguments to the conditional trap instructions is more work than
just doing a compare and branch.

Signed-off-by: Olof Johansson <olof@lixom.net>

diff --git a/include/asm-powerpc/bug.h b/include/asm-powerpc/bug.h
index 02e171c..966de9b 100644
--- a/include/asm-powerpc/bug.h
+++ b/include/asm-powerpc/bug.h
@@ -54,12 +54,6 @@
 	".previous\n"
 #endif
 
-/*
- * BUG_ON() and WARN_ON() do their best to cooperate with compile-time
- * optimisations. However depending on the complexity of the condition
- * some compiler versions may not produce optimal results.
- */
-
 #define BUG() do {						\
 	__asm__ __volatile__(					\
 		"1:	twi 31,0,0\n"				\
@@ -69,20 +63,6 @@
 	for(;;) ;						\
 } while (0)
 
-#define BUG_ON(x) do {						\
-	if (__builtin_constant_p(x)) {				\
-		if (x)						\
-			BUG();					\
-	} else {						\
-		__asm__ __volatile__(				\
-		"1:	"PPC_TLNEI"	%4,0\n"			\
-		_EMIT_BUG_ENTRY					\
-		: : "i" (__FILE__), "i" (__LINE__), "i" (0),	\
-		  "i" (sizeof(struct bug_entry)),		\
-		  "r" ((__force long)(x)));			\
-	}							\
-} while (0)
-
 #define __WARN() do {						\
 	__asm__ __volatile__(					\
 		"1:	twi 31,0,0\n"				\
@@ -92,27 +72,8 @@
 		  "i" (sizeof(struct bug_entry)));		\
 } while (0)
 
-#define WARN_ON(x) ({						\
-	int __ret_warn_on = !!(x);				\
-	if (__builtin_constant_p(__ret_warn_on)) {		\
-		if (__ret_warn_on)				\
-			__WARN();				\
-	} else {						\
-		__asm__ __volatile__(				\
-		"1:	"PPC_TLNEI"	%4,0\n"			\
-		_EMIT_BUG_ENTRY					\
-		: : "i" (__FILE__), "i" (__LINE__),		\
-		  "i" (BUGFLAG_WARNING),			\
-		  "i" (sizeof(struct bug_entry)),		\
-		  "r" (__ret_warn_on));				\
-	}							\
-	unlikely(__ret_warn_on);				\
-})
-
 #define HAVE_ARCH_BUG
 #define HAVE_ARCH_WARN
-#define HAVE_ARCH_BUG_ON
-#define HAVE_ARCH_WARN_ON
 #endif /* __ASSEMBLY __ */
 #endif /* ...
[ message continues ]
[view original message]
From: Scott Wood
Subject: Re: [PATCH 2/2] powerpc: Switch to generic WARN_ON()/BUG_ON()
Date: Thursday, October 11, 2007 - 12:55 pm

It'd be nice if we could get GCC to generate bug table entries for
__builtin_trap(); that way we could use GCC's ability to put arbitrary
conditions in the trap instruction.

-Scott
-


[ message continues ]
[view original message]
From: Paul Mackerras
Subject: Re: [PATCH 2/2] powerpc: Switch to generic WARN_ON()/BUG_ON()
Date: Thursday, October 11, 2007 - 6:23 pm

It might be more instructions but it takes fewer cycles, I would
expect.  Do you have the actual instruction sequences to compare?

Paul.
-


[ message continues ]
[view original message]
From: Kyle McMartin
Subject: Re: [PATCH 2/2] powerpc: Switch to generic WARN_ON()/BUG_ON()
Date: Thursday, October 11, 2007 - 7:04 pm

I really hope WARN_ON/BUG_ON aren't hotpaths on powerpc. ;-)

Cheers,
	Kyle
-


[ message continues ]
[view original message]
From: Olof Johansson
Subject: Re: [PATCH 2/2] powerpc: Switch to generic WARN_ON()/BUG_ON()
Date: Thursday, October 11, 2007 - 7:41 pm

Not the taken branch of them, no. :)  But making it past them as fast
as possible when they're not tripping is always good.


-Olof
-


[ message continues ]
[view original message]
From: Olof Johansson
Subject: Re: [PATCH 2/2] powerpc: Switch to generic WARN_ON()/BUG_ON()
Date: Thursday, October 11, 2007 - 7:40 pm

Sure. Just looking at a couple of cases:

On range comparisons such as (a < CONST):
* without this patch, you end up with a comparison, then a cr-to-cr op +
mfcr + mask operation + tdnei.
* with the patch, you get a comparison and a conditional branch with
twi out of line.

On multiple comparisons like WARN_ON( a || b), it seems that there's an
added li to make the tdnei always hit for the first case. There it uses
a regular conditional branch, so no real difference in generated code
besides that.

Code examples. This was with a ppc64_defconfig, so CONFIG_POWER4_ONLY
wasn't enabled. It uses mfocrf when it is, which is alot cheaper. Still,
the rest of the code sequence is the same:

void irq_free_virt(unsigned int virq, unsigned int count)
{
        unsigned long flags;
        unsigned int i;

        WARN_ON (virq < NUM_ISA_INTERRUPTS);
        WARN_ON (count == 0 || (virq + count) > irq_virq_count);

        spin_lock_irqsave(&irq_big_lock, flags);

Without the patch:

c00000000000b33c <.irq_free_virt>:
c00000000000b33c:       7c 08 02 a6     mflr    r0
c00000000000b340:       2b 83 00 0f     cmplwi  cr7,r3,15
c00000000000b344:       fb c1 ff f0     std     r30,-16(r1)
c00000000000b348:       fb e1 ff f8     std     r31,-8(r1)
c00000000000b34c:       7c 9e 23 78     mr      r30,r4
c00000000000b350:       7c 7f 1b 78     mr      r31,r3
c00000000000b354:       4f dd e8 42     crnot   4*cr7+eq,4*cr7+gt
c00000000000b358:       f8 01 00 10     std     r0,16(r1)
c00000000000b35c:       f8 21 ff 81     stdu    r1,-128(r1)
c00000000000b360:       7c 00 00 26     mfcr    r0
c00000000000b364:       54 00 ff fe     rlwinm  r0,r0,31,31,31
c00000000000b368:       0b 00 00 00     tdnei   r0,0
c00000000000b36c:       2f a4 00 00     cmpdi   cr7,r4,0
c00000000000b370:       38 00 00 01     li      r0,1
c00000000000b374:       41 9e 00 1c     beq     cr7,c00000000000b390 <.irq_free_virt+0x54>
c00000000000b378:       e9 22 80 d8     ld      r9,-32552(r2)
c00000000000b37c:       ...
[ message continues ]
[view original message]
From: Olof Johansson
Subject: [PATCH v2] [2/2] powerpc: Switch to generic WARN_ON()/BUG_ON()
Date: Thursday, October 18, 2007 - 7:03 pm

[POWERPC] Switch to generic WARN_ON()/BUG_ON()

Not using the ppc-specific WARN_ON/BUG_ON constructs actually saves about
4K text on a ppc64_defconfig. The main reason seems to be that prepping
the arguments to the conditional trap instructions is more work than
just doing a compare and branch.

Signed-off-by: Olof Johansson <olof@lixom.net>

Index: k.org/include/asm-powerpc/bug.h
===================================================================
--- k.org.orig/include/asm-powerpc/bug.h
+++ k.org/include/asm-powerpc/bug.h
@@ -54,12 +54,6 @@
 	".previous\n"
 #endif
 
-/*
- * BUG_ON() and WARN_ON() do their best to cooperate with compile-time
- * optimisations. However depending on the complexity of the condition
- * some compiler versions may not produce optimal results.
- */
-
 #define BUG() do {						\
 	__asm__ __volatile__(					\
 		"1:	twi 31,0,0\n"				\
@@ -69,20 +63,6 @@
 	for(;;) ;						\
 } while (0)
 
-#define BUG_ON(x) do {						\
-	if (__builtin_constant_p(x)) {				\
-		if (x)						\
-			BUG();					\
-	} else {						\
-		__asm__ __volatile__(				\
-		"1:	"PPC_TLNEI"	%4,0\n"			\
-		_EMIT_BUG_ENTRY					\
-		: : "i" (__FILE__), "i" (__LINE__), "i" (0),	\
-		  "i" (sizeof(struct bug_entry)),		\
-		  "r" ((__force long)(x)));			\
-	}							\
-} while (0)
-
 #define __WARN() do {						\
 	__asm__ __volatile__(					\
 		"1:	twi 31,0,0\n"				\
@@ -92,23 +72,6 @@
 		  "i" (sizeof(struct bug_entry)));		\
 } while (0)
 
-#define WARN_ON(x) ({						\
-	int __ret_warn_on = !!(x);				\
-	if (__builtin_constant_p(__ret_warn_on)) {		\
-		if (__ret_warn_on)				\
-			__WARN();				\
-	} else {						\
-		__asm__ __volatile__(				\
-		"1:	"PPC_TLNEI"	%4,0\n"			\
-		_EMIT_BUG_ENTRY					\
-		: : "i" (__FILE__), "i" (__LINE__),		\
-		  "i" (BUGFLAG_WARNING),			\
-		  "i" (sizeof(struct bug_entry)),		\
-		  "r" (__ret_warn_on));				\
-	}							\
-	unlikely(__ret_warn_on);				\
-})
-
 #endif /* __ASSEMBLY __ */
 #endif /* CONFIG_BUG */
 
-


[ message continues ]
[view original message]
From: Kyle McMartin
Subject: Re: [PATCH 1/2] bug.h: Introduce HAVE_ARCH_WARN
Date: Thursday, October 11, 2007 - 10:20 am

ack parisc hunk (obviously :)

cheers,
	kyle
-


[ message continues ]
[view original message]
From: Paul Mundt
Subject: Re: [PATCH 1/2] bug.h: Introduce HAVE_ARCH_WARN
Date: Thursday, October 11, 2007 - 7:53 pm

sh bits are fine.

Acked-by: Paul Mundt <lethal@linux-sh.org>
-


[ message continues ]
[view original message]
From: Andrew Morton
Subject: Re: [PATCH 1/2] bug.h: Introduce HAVE_ARCH_WARN
Date: Thursday, October 18, 2007 - 3:17 pm

On Thu, 11 Oct 2007 12:12:11 -0500


Can't we just do #ifndef __WARN?
-


[ message continues ]
[view original message]
From: Olof Johansson
Subject: Re: [PATCH 1/2] bug.h: Introduce HAVE_ARCH_WARN
Date: Thursday, October 18, 2007 - 3:34 pm

Yep, good idea. I'll respin.


-Olof
-


[ message continues ]
[view original message]
From: Olof Johansson
Subject: [PATCH v2] [1/2] bug.h: Remove HAVE_ARCH_BUG.* / HAVE_ARCH_WARN.*
Date: Thursday, October 18, 2007 - 7:03 pm

No need to have the HAVE_ARCH_BUG.* / HAVE_ARCH_WARN.* defines, when
the generic implementation can just use #ifndef on the macros themselves.

Also, introduce __WARN() in the generic case, so the generic WARN_ON()
can use arch-specific code for when the condition is true.

Built on powerpc, i386, sh and sparc64.


Signed-off-by: Olof Johansson <olof@lixom.net>

Index: k.org/include/asm-generic/bug.h
===================================================================
--- k.org.orig/include/asm-generic/bug.h
+++ k.org/include/asm-generic/bug.h
@@ -20,39 +20,44 @@ struct bug_entry {
 #define BUGFLAG_WARNING	(1<<0)
 #endif	/* CONFIG_GENERIC_BUG */
 
-#ifndef HAVE_ARCH_BUG
+#ifndef BUG
 #define BUG() do { \
 	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __FUNCTION__); \
 	panic("BUG!"); \
 } while (0)
 #endif
 
-#ifndef HAVE_ARCH_BUG_ON
+#ifndef BUG_ON
 #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
 #endif
 
-#ifndef HAVE_ARCH_WARN_ON
+#ifndef __WARN
+#define __WARN() do {							\
+	printk("WARNING: at %s:%d %s()\n", __FILE__,			\
+		__LINE__, __FUNCTION__);				\
+	dump_stack();							\
+} while (0)
+#endif
+
+#ifndef 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();						\
-	}								\
+	if (unlikely(__ret_warn_on))					\
+		__WARN();						\
 	unlikely(__ret_warn_on);					\
 })
 #endif
 
 #else /* !CONFIG_BUG */
-#ifndef HAVE_ARCH_BUG
+#ifndef BUG
 #define BUG()
 #endif
 
-#ifndef HAVE_ARCH_BUG_ON
+#ifndef BUG_ON
 #define BUG_ON(condition) do { if (condition) ; } while(0)
 #endif
 
-#ifndef HAVE_ARCH_WARN_ON
+#ifndef WARN_ON
 #define WARN_ON(condition) ({						\
 	int __ret_warn_on = !!(condition);				\
 	unlikely(__ret_warn_on);					\
Index: ...
[ message continues ]
Previous thread: [PATCH try #3] Blackfin BF54x Input Keypad controller driver by Bryan Wu on Thursday, October 11, 2007 - 9:28 am. (2 messages)

Next thread: [RFC] mm - background_writeout exits when pages_skipped ? by richard kennedy on Thursday, October 11, 2007 - 10:19 am. (2 messages)