bug.h: Introduce HAVE_ARCH_WARN
PATCH 1/2 bug.h: Introduce HAVE_ARCH_WARN
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).
include/asm-generic/bug.h +10 -5
@@ -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
include/asm-parisc/bug.h +1 -0
@@ -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. */
include/asm-powerpc/bug.h +1 -0
@@ -110,6 +110,7 @@ })#define HAVE_ARCH_BUG +#define HAVE_ARCH_WARN #define HAVE_ARCH_BUG_ON #define HAVE_ARCH_WARN_ON #endif /* __ASSEMBLY __ */
include/asm-sh/bug.h +1 -0
@@ -5,6 +5,7 @@#ifdef CONFIG_BUG #define HAVE_ARCH_BUG +#define HAVE_ARCH_WARN #define HAVE_ARCH_WARN_ON /**
PATCH 2/2 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.
include/asm-powerpc/bug.h +0 -39
@@ -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 /* CONFIG_BUG */
Re: [PATCH 2/2] powerpc: Switch to generic WARN_ON()/BUG_ON()
Quoted 4 lines · Olof Johansson
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.
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
Re: [PATCH 2/2] powerpc: Switch to generic WARN_ON()/BUG_ON()
In reply to Olof JohanssonQuoted 4 lines · Olof Johansson
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.
It might be more instructions but it takes fewer cycles, I would expect. Do you have the actual instruction sequences to compare?
Paul.
Re: [PATCH 2/2] powerpc: Switch to generic WARN_ON()/BUG_ON()
Quoted 10 lines · Paul Mackerras
Quoted 4 lines · Olof Johansson
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.It might be more instructions but it takes fewer cycles, I would expect. Do you have the actual instruction sequences to compare?
I really hope WARN_ON/BUG_ON aren't hotpaths on powerpc. ;-)
Cheers,
Kyle
Re: [PATCH 2/2] powerpc: Switch to generic WARN_ON()/BUG_ON()
Quoted 8 lines · Kyle McMartin
Quoted 7 lines · Paul Mackerras
Quoted 4 lines · Olof Johansson
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.
Quoted 5 lines
Quoted 3 lines
It might be more instructions but it takes fewer cycles, I would expect. Do you have the actual instruction sequences to compare?
I really hope WARN_ON/BUG_ON aren't hotpaths on powerpc. ;-)
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
Re: [PATCH 2/2] powerpc: Switch to generic WARN_ON()/BUG_ON()
In reply to Paul MackerrasQuoted 9 lines · Paul Mackerras
Quoted 4 lines · Olof Johansson
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.It might be more instructions but it takes fewer cycles, I would expect. Do you have the actual instruction sequences to compare?
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: 7d 63 22 14 add r11,r3,r4 c00000000000b380: 80 09 00 00 lwz r0,0(r9) c00000000000b384: 7f 8b 00 40 cmplw cr7,r11,r0 c00000000000b388: 7c 00 00 26 mfcr r0 c00000000000b38c: 54 00 f7 fe rlwinm r0,r0,30,31,31 c00000000000b390: 0b 00 00 00 tdnei r0,0 c00000000000b394: e8 62 80 f8 ld r3,-32520(r2) c00000000000b398: 48 54 4c b1 bl c000000000550048 <._spin_lock_irqsave> c00000000000b39c: 60 00 00 00 nop
With the patch:
c00000000000b0b0 <.irq_free_virt>:
c00000000000b0b0: 7c 08 02 a6 mflr r0 c00000000000b0b4: 2b 83 00 0f cmplwi cr7,r3,15 c00000000000b0b8: fb c1 ff f0 std r30,-16(r1) c00000000000b0bc: fb e1 ff f8 std r31,-8(r1) c00000000000b0c0: 7c 9e 23 78 mr r30,r4 c00000000000b0c4: 7c 7f 1b 78 mr r31,r3 c00000000000b0c8: f8 01 00 10 std r0,16(r1) c00000000000b0cc: f8 21 ff 81 stdu r1,-128(r1) c00000000000b0d0: 41 bd 00 08 bgt cr7,c00000000000b0d8 <.irq_free_virt+0x28> c00000000000b0d4: 0f e0 00 00 twi 31,r0,0 c00000000000b0d8: 2f be 00 00 cmpdi cr7,r30,0 c00000000000b0dc: 41 9e 00 18 beq cr7,c00000000000b0f4 <.irq_free_virt+0x44> c00000000000b0e0: e9 22 80 d8 ld r9,-32552(r2) c00000000000b0e4: 7d 7f f2 14 add r11,r31,r30 c00000000000b0e8: 80 09 00 00 lwz r0,0(r9) c00000000000b0ec: 7f 8b 00 40 cmplw cr7,r11,r0 c00000000000b0f0: 40 bd 00 08 ble cr7,c00000000000b0f8 <.irq_free_virt+0x48> c00000000000b0f4: 0f e0 00 00 twi 31,r0,0 c00000000000b0f8: e8 62 80 f8 ld r3,-32520(r2) c00000000000b0fc: 48 54 3f 75 bl c00000000054f070 <._spin_lock_irqsave> c00000000000b100: 60 00 00 00 nop
PATCH v2 2/2 powerpc: Switch to generic WARN_ON()/BUG_ON()
In reply to Olof Johansson[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.
include/asm-powerpc/bug.h +0 -37
@@ -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 */
Quoted 8 lines · Olof Johansson
HAVE_ARCH_WARNis 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 <[email protected]>
ack parisc hunk (obviously :)
cheers,
kyle
Quoted 8 lines · Olof Johansson
HAVE_ARCH_WARNis 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 <[email protected]>
sh bits are fine.
Quoted 6 lines · On Thu, 11 Oct 2007 12:12:11 -0500 Olof Johansson <[email protected]> wrote:
HAVE_ARCH_WARNis 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).
Those HAVE_ARCH_foo things are unpleasant.
Quoted 6 lines
include/asm-generic/bug.h +0
-0
include/asm-generic/bug.h struct bug_entry { · line 40
Quoted 10 lines
include/asm-generic/bug.h +7
-0@@ -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
Can't we just do #ifndef __WARN?
Quoted 11 lines · Andrew Morton
Quoted 6 lines · On Thu, 11 Oct 2007 12:12:11 -0500 Olof Johansson <[email protected]> wrote:
HAVE_ARCH_WARNis 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).Those
HAVE_ARCH_foothings are unpleasant.
[...]
Quoted 1 line
Can't we just do #ifndef
__WARN?
Yep, good idea. I'll respin.
-Olof
PATCH v2 1/2 bug.h: Remove HAVE_ARCH_BUG.* / HAVE_ARCH_WARN.*
In reply to Olof JohanssonNo 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.
include/asm-generic/bug.h +16 -11
@@ -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); \
include/asm-parisc/bug.h +0 -2
@@ -7,8 +7,6 @@*/ #ifdef CONFIG_BUG -#define HAVE_ARCH_BUG -#define HAVE_ARCH_WARN_ON /* the break instruction is used as BUG() marker. */ #define PARISC_BUG_BREAK_ASM "break 0x1f, 0x1fff"
include/asm-powerpc/bug.h +0 -3
@@ -109,9 +109,6 @@unlikely (__ret_warn_on); \ }) -#define HAVE_ARCH_BUG -#define HAVE_ARCH_BUG_ON -#define HAVE_ARCH_WARN_ON #endif /* __ASSEMBLY __ */ #endif /* CONFIG_BUG */
include/asm-sh/bug.h +0 -2
@@ -4,8 +4,6 @@#define TRAPA_BUG_OPCODE 0xc33e/* trapa #0x3e */ #ifdef CONFIG_BUG -#define HAVE_ARCH_BUG -#define HAVE_ARCH_WARN_ON /** * _EMIT_BUG_ENTRY
include/asm-alpha/bug.h +0 -1
@@ -10,7 +10,6 @@ __asm__ __volatile__("call_pal %0 # bugchk\n\t"".long %1\n\t.8byte %2" \ : : "i" (PAL_bugchk), "i"(__LINE__), "i"(__FILE__)) -#define HAVE_ARCH_BUG #endif #include <asm-generic/bug.h>
include/asm-arm/bug.h +0 -1
@@ -16,7 +16,6 @@ extern void __bug(const char *file, int#endif -#define HAVE_ARCH_BUG #endif #include <asm-generic/bug.h>
include/asm-avr32/bug.h +0 -3
@@ -63,9 +63,6 @@unlikely (__ret_warn_on); \ }) -#define HAVE_ARCH_BUG -#define HAVE_ARCH_WARN_ON -#endif /* CONFIG_BUG */ #include <asm-generic/bug.h>
include/asm-frv/bug.h +0 -1
@@ -32,7 +32,6 @@ do { \ asmvolatile ("nop" ); \ } while(0) -#define HAVE_ARCH_BUG #define BUG () \ do { \ _debug_bug_printk(); \
include/asm-ia64/bug.h +0 -1
@@ -6,7 +6,6 @@#define BUG () do { printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__); ia64_abort(); } while (0)/* should this BUG be made generic? */ -#define HAVE_ARCH_BUG #endif #include <asm-generic/bug.h>
include/asm-m68k/bug.h +0 -1
@@ -21,7 +21,6 @@ } while (0)#endif -#define HAVE_ARCH_BUG #endif #include <asm-generic/bug.h>
include/asm-mips/bug.h +0 -4
@@ -12,8 +12,6 @@ do { \ __asm__ __volatile__("break %0" : :"i" (BRK_BUG )); \ } while (0) -#define HAVE_ARCH_BUG -#if (_MIPS_ISA > _MIPS_ISA_MIPS1)#define BUG_ON (condition) \
@@ -22,8 +20,6 @@ do { \ : : "r" (condition), "i" (BRK_BUG)); \ } while (0) -#define HAVE_ARCH_BUG_ON -#endif /* _MIPS_ISA > _MIPS_ISA_MIPS1 */ #endif
include/asm-s390/bug.h +0 -2
@@ -61,8 +61,6 @@unlikely (__ret_warn_on); \ }) -#define HAVE_ARCH_BUG -#define HAVE_ARCH_WARN_ON #endif /* CONFIG_BUG */ #include <asm-generic/bug.h>
include/asm-sh64/bug.h +0 -1
@@ -11,7 +11,6 @@ *(volatile int *)0 =0 ; \ } while (0) -#define HAVE_ARCH_BUG #endif #include <asm-generic/bug.h>
include/asm-sparc/bug.h +0 -1
@@ -26,7 +26,6 @@ extern void do_BUG(const char *file, int#define BUG () __bug_trap()#endif -#define HAVE_ARCH_BUG #endif #include <asm-generic/bug.h>
include/asm-sparc64/bug.h +0 -1
@@ -14,7 +14,6 @@ extern void do_BUG(const char *file, int#define BUG () __builtin_trap()#endif -#define HAVE_ARCH_BUG #endif #include <asm-generic/bug.h>
include/asm-v850/bug.h +0 -1
@@ -17,7 +17,6 @@#ifdef CONFIG_BUG extern void __bug (void ) __attribute__ ((noreturn));#define BUG () __bug() -#define HAVE_ARCH_BUG #endif #include <asm-generic/bug.h>
include/asm-x86/bug.h +0 -1
@@ -2,7 +2,6 @@#define _ASM_X86_BUG_H#ifdef CONFIG_BUG -#define HAVE_ARCH_BUG #ifdef CONFIG_DEBUG_BUGVERBOSE