linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* BUGed to death
@ 2003-04-14 20:19 Martin J. Bligh
  2003-04-14 20:40 ` Duncan Sands
                   ` (2 more replies)
  0 siblings, 3 replies; 35+ messages in thread
From: Martin J. Bligh @ 2003-04-14 20:19 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel

Seems all these bug checks are fairly expensive. I can get 1%
back on system time for kernel compiles by changing BUG to 
"do {} while (0)" to make them all compile away. Profiles aren't
very revealing though ... seems to be within experimental error ;-(

I was pondering CONFIG_RUN_WILD_NAKED_AND_FREE, but maybe we can
just nail a few of the hottest path ones instead (I think you did
a couple already recently). I guess that suggestion isn't much
use without more profile data though ;-)

M.

Kernbench: (make -j N vmlinux, where N = 2 x num_cpus)
                              Elapsed      System        User         CPU
              2.5.67-mjb2       43.34       76.24      563.55     1476.25
        2.5.67-mjb2-nobug       43.43       75.29      564.12     1471.75

Kernbench: (make -j N vmlinux, where N = 16 x num_cpus)
                              Elapsed      System        User         CPU
              2.5.67-mjb2       43.91       85.05      570.61     1493.50
        2.5.67-mjb2-nobug       44.12       84.80      571.10     1485.00

Kernbench: (make -j vmlinux, maximal tasks)
                              Elapsed      System        User         CPU
              2.5.67-mjb2       44.01       85.12      570.10     1488.25
        2.5.67-mjb2-nobug       44.03       83.93      570.37     1485.25


DISCLAIMER: SPEC(tm) and the benchmark name SDET(tm) are registered
trademarks of the Standard Performance Evaluation Corporation. This 
benchmarking was performed for research purposes only, and the run results
are non-compliant and not-comparable with any published results.

Results are shown as percentages of the first set displayed

SDET 1  (see disclaimer)
                           Throughput    Std. Dev
              2.5.67-mjb2       100.0%         1.9%
        2.5.67-mjb2-nobug       104.1%         0.0%

SDET 2  (see disclaimer)
                           Throughput    Std. Dev
              2.5.67-mjb2       100.0%         1.9%
        2.5.67-mjb2-nobug       106.4%         0.0%

SDET 4  (see disclaimer)
                           Throughput    Std. Dev
              2.5.67-mjb2       100.0%         3.8%
        2.5.67-mjb2-nobug        97.1%         1.3%

SDET 8  (see disclaimer)
                           Throughput    Std. Dev
              2.5.67-mjb2       100.0%         0.9%
        2.5.67-mjb2-nobug       100.7%         0.7%

SDET 16  (see disclaimer)
                           Throughput    Std. Dev
              2.5.67-mjb2       100.0%         1.3%
        2.5.67-mjb2-nobug       102.8%         0.7%

SDET 32  (see disclaimer)
                           Throughput    Std. Dev
              2.5.67-mjb2       100.0%         0.5%
        2.5.67-mjb2-nobug       100.8%         0.5%

SDET 64  (see disclaimer)
                           Throughput    Std. Dev
              2.5.67-mjb2       100.0%         0.4%
        2.5.67-mjb2-nobug       100.6%         0.3%

SDET 128  (see disclaimer)
                           Throughput    Std. Dev
              2.5.67-mjb2       100.0%         0.1%
        2.5.67-mjb2-nobug       100.9%         0.2%


RMAPBENCH

rmapbench: 100x100-linear
                              Elapsed      System        User         CPU
              2.5.67-mjb2       44.24      470.00      211.26     1527.67
        2.5.67-mjb2-nobug       51.20      579.11      218.33     1533.33

rmapbench: 100x100-random
                              Elapsed      System        User         CPU
              2.5.67-mjb2        2.99       26.50        0.44      895.67
        2.5.67-mjb2-nobug        3.03       28.02        0.33      892.00

rmapbench: 1x10000-linear
                              Elapsed      System        User         CPU
              2.5.67-mjb2        2.53        1.32        0.19       59.67
        2.5.67-mjb2-nobug        2.37        1.17        0.19       57.00


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

* Re: BUGed to death
  2003-04-14 20:19 BUGed to death Martin J. Bligh
@ 2003-04-14 20:40 ` Duncan Sands
  2003-04-14 21:02   ` Dave Jones
  2003-04-14 21:00 ` Dave Jones
  2003-04-15 12:01 ` Duncan Sands
  2 siblings, 1 reply; 35+ messages in thread
From: Duncan Sands @ 2003-04-14 20:40 UTC (permalink / raw)
  To: Martin J. Bligh, Andrew Morton; +Cc: linux-kernel

On Monday 14 April 2003 22:19, Martin J. Bligh wrote:
> Seems all these bug checks are fairly expensive. I can get 1%
> back on system time for kernel compiles by changing BUG to
> "do {} while (0)" to make them all compile away. Profiles aren't
> very revealing though ... seems to be within experimental error ;-(
>
> I was pondering CONFIG_RUN_WILD_NAKED_AND_FREE, but maybe we can
> just nail a few of the hottest path ones instead (I think you did
> a couple already recently). I guess that suggestion isn't much
> use without more profile data though ;-)

You would think that the compiler would consider a branch leading to
ud2 (i.e. BUG()) to be "unlikely", but it doesn't seem to.  Maybe some
improvement can be made there.

All the best,

Duncan.

PS: gcc 3.2.3

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

* Re: BUGed to death
  2003-04-14 21:00 ` Dave Jones
@ 2003-04-14 20:55   ` Martin J. Bligh
  2003-04-14 21:08     ` Dave Jones
  2003-04-15  0:23   ` H. Peter Anvin
  1 sibling, 1 reply; 35+ messages in thread
From: Martin J. Bligh @ 2003-04-14 20:55 UTC (permalink / raw)
  To: Dave Jones; +Cc: Andrew Morton, linux-kernel

>  > Seems all these bug checks are fairly expensive. I can get 1%
>  > back on system time for kernel compiles by changing BUG to 
>  > "do {} while (0)" to make them all compile away. Profiles aren't
>  > very revealing though ... seems to be within experimental error ;-(
>  > 
>  > I was pondering CONFIG_RUN_WILD_NAKED_AND_FREE
> 
> The sort of folks who would worry about that very last 1% are the
> sort of people that would more than likely hit these BUGs as they're
> really stressing things.
> 
> Losing a bunch of potential reports (and possibly doing bad things),
> in the name of a 1% performance boost doesn't sound too productive to me.

True - however I should have included some more info ... Andrew worked
out that some of the hottest ones lead to a null ptr dereference
immediately afterwards anyways, so they're actually pointless.

I wasn't seriously suggesting just removing all of them, was just a point
of interest for some things that would be worth looking at ;-)

I'd agree with you that an unreliable system is 100% slower than a working
one ;-)

M.

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

* Re: BUGed to death
  2003-04-14 20:19 BUGed to death Martin J. Bligh
  2003-04-14 20:40 ` Duncan Sands
@ 2003-04-14 21:00 ` Dave Jones
  2003-04-14 20:55   ` Martin J. Bligh
  2003-04-15  0:23   ` H. Peter Anvin
  2003-04-15 12:01 ` Duncan Sands
  2 siblings, 2 replies; 35+ messages in thread
From: Dave Jones @ 2003-04-14 21:00 UTC (permalink / raw)
  To: Martin J. Bligh; +Cc: Andrew Morton, linux-kernel

On Mon, Apr 14, 2003 at 01:19:58PM -0700, Martin J. Bligh wrote:
 > Seems all these bug checks are fairly expensive. I can get 1%
 > back on system time for kernel compiles by changing BUG to 
 > "do {} while (0)" to make them all compile away. Profiles aren't
 > very revealing though ... seems to be within experimental error ;-(
 > 
 > I was pondering CONFIG_RUN_WILD_NAKED_AND_FREE

The sort of folks who would worry about that very last 1% are the
sort of people that would more than likely hit these BUGs as they're
really stressing things.

Losing a bunch of potential reports (and possibly doing bad things),
in the name of a 1% performance boost doesn't sound too productive to me.

		Dave


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

* Re: BUGed to death
  2003-04-14 20:40 ` Duncan Sands
@ 2003-04-14 21:02   ` Dave Jones
  2003-04-14 21:10     ` Duncan Sands
  0 siblings, 1 reply; 35+ messages in thread
From: Dave Jones @ 2003-04-14 21:02 UTC (permalink / raw)
  To: Duncan Sands; +Cc: Martin J. Bligh, Andrew Morton, linux-kernel

On Mon, Apr 14, 2003 at 10:40:41PM +0200, Duncan Sands wrote:

 > You would think that the compiler would consider a branch leading to
 > ud2 (i.e. BUG()) to be "unlikely", but it doesn't seem to.  Maybe some
 > improvement can be made there.

BUG_ON is already marked unlikely.
See include/linux/kernel.h

The costs here are doing the actual checks, nothing to do with
the branch prediction.

		Dave


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

* Re: BUGed to death
  2003-04-14 20:55   ` Martin J. Bligh
@ 2003-04-14 21:08     ` Dave Jones
  2003-04-14 21:50       ` Andrew Morton
  0 siblings, 1 reply; 35+ messages in thread
From: Dave Jones @ 2003-04-14 21:08 UTC (permalink / raw)
  To: Martin J. Bligh; +Cc: Andrew Morton, linux-kernel

On Mon, Apr 14, 2003 at 01:55:40PM -0700, Martin J. Bligh wrote:

 > True - however I should have included some more info ... Andrew worked
 > out that some of the hottest ones lead to a null ptr dereference
 > immediately afterwards anyways, so they're actually pointless.

Erk, that doesn't sound good. Example ?

		Dave


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

* Re: BUGed to death
  2003-04-14 21:02   ` Dave Jones
@ 2003-04-14 21:10     ` Duncan Sands
  2003-04-14 21:17       ` Dave Jones
  0 siblings, 1 reply; 35+ messages in thread
From: Duncan Sands @ 2003-04-14 21:10 UTC (permalink / raw)
  To: Dave Jones; +Cc: Martin J. Bligh, Andrew Morton, linux-kernel

> BUG_ON is already marked unlikely.
> See include/linux/kernel.h
>
> The costs here are doing the actual checks, nothing to do with
> the branch prediction.

Some places don't seem to know about BUG_ON, for example
(from include/linux/skbuff.h):

static inline char *__skb_pull(struct sk_buff *skb, unsigned int len)
{
        skb->len -= len;
        if (skb->len < skb->data_len)
                BUG();
        return skb->data += len;
}

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

* Re: BUGed to death
  2003-04-14 21:10     ` Duncan Sands
@ 2003-04-14 21:17       ` Dave Jones
  2003-04-15 11:57         ` Duncan Sands
  0 siblings, 1 reply; 35+ messages in thread
From: Dave Jones @ 2003-04-14 21:17 UTC (permalink / raw)
  To: Duncan Sands; +Cc: Martin J. Bligh, Andrew Morton, linux-kernel

On Mon, Apr 14, 2003 at 11:10:05PM +0200, Duncan Sands wrote:

 > Some places don't seem to know about BUG_ON, for example
 > (from include/linux/skbuff.h):

Right, BUG_ON was added later (possibly for the purpose of
marking unlikely branches).  I see your point now about gcc
not recognising branches which are going to be unlikely, but
whether or not it should is questionable IMO.

		Dave


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

* Re: BUGed to death
  2003-04-14 21:08     ` Dave Jones
@ 2003-04-14 21:50       ` Andrew Morton
  2003-04-14 21:55         ` Dave Jones
  0 siblings, 1 reply; 35+ messages in thread
From: Andrew Morton @ 2003-04-14 21:50 UTC (permalink / raw)
  To: Dave Jones; +Cc: mbligh, linux-kernel

Dave Jones <davej@codemonkey.org.uk> wrote:
>
> On Mon, Apr 14, 2003 at 01:55:40PM -0700, Martin J. Bligh wrote:
> 
>  > True - however I should have included some more info ... Andrew worked
>  > out that some of the hottest ones lead to a null ptr dereference
>  > immediately afterwards anyways, so they're actually pointless.
> 
> Erk, that doesn't sound good. Example ?
> 

	if (foo == NULL)
		BUG();
	*foo = bar;

The BUG is a waste of space.


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

* Re: BUGed to death
  2003-04-14 21:50       ` Andrew Morton
@ 2003-04-14 21:55         ` Dave Jones
  0 siblings, 0 replies; 35+ messages in thread
From: Dave Jones @ 2003-04-14 21:55 UTC (permalink / raw)
  To: Andrew Morton; +Cc: mbligh, linux-kernel

On Mon, Apr 14, 2003 at 02:50:28PM -0700, Andrew Morton wrote:

 > 	if (foo == NULL)
 > 		BUG();
 > 	*foo = bar;
 > 
 > The BUG is a waste of space.

Agreed. That is somewhat silly.

		Dave


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

* Re: BUGed to death
  2003-04-14 21:00 ` Dave Jones
  2003-04-14 20:55   ` Martin J. Bligh
@ 2003-04-15  0:23   ` H. Peter Anvin
  1 sibling, 0 replies; 35+ messages in thread
From: H. Peter Anvin @ 2003-04-15  0:23 UTC (permalink / raw)
  To: linux-kernel

Followup to:  <20030414210006.GA7831@suse.de>
By author:    Dave Jones <davej@codemonkey.org.uk>
In newsgroup: linux.dev.kernel
> 
> The sort of folks who would worry about that very last 1% are the
> sort of people that would more than likely hit these BUGs as they're
> really stressing things.
> 
> Losing a bunch of potential reports (and possibly doing bad things),
> in the name of a 1% performance boost doesn't sound too productive to me.
> 

It's useful to have the hooks to actually *measure* the overhead,
though.

	-hpa
-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
Architectures needed: ia64 m68k mips64 ppc ppc64 s390 s390x sh v850 x86-64

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

* Re: BUGed to death
  2003-04-14 21:17       ` Dave Jones
@ 2003-04-15 11:57         ` Duncan Sands
  2003-04-15 12:05           ` Dave Jones
  0 siblings, 1 reply; 35+ messages in thread
From: Duncan Sands @ 2003-04-15 11:57 UTC (permalink / raw)
  To: Dave Jones; +Cc: Martin J. Bligh, Andrew Morton, linux-kernel

> Right, BUG_ON was added later (possibly for the purpose of
> marking unlikely branches).  I see your point now about gcc
> not recognising branches which are going to be unlikely, but
> whether or not it should is questionable IMO.

It is questionable.  Since even in core kernel code there are
many places with
	if (cond)
		BUG();
rather than
	BUG_ON(cond);
it may be worth seeing if converting them makes a difference
(increases code size though).

Ciao,

Duncan.

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

* Re: BUGed to death
  2003-04-14 20:19 BUGed to death Martin J. Bligh
  2003-04-14 20:40 ` Duncan Sands
  2003-04-14 21:00 ` Dave Jones
@ 2003-04-15 12:01 ` Duncan Sands
  2003-04-15 12:31   ` Jens Axboe
  2003-04-15 14:35   ` Martin J. Bligh
  2 siblings, 2 replies; 35+ messages in thread
From: Duncan Sands @ 2003-04-15 12:01 UTC (permalink / raw)
  To: Martin J. Bligh, Andrew Morton; +Cc: linux-kernel

On Monday 14 April 2003 22:19, Martin J. Bligh wrote:
> Seems all these bug checks are fairly expensive. I can get 1%
> back on system time for kernel compiles by changing BUG to
> "do {} while (0)" to make them all compile away. Profiles aren't
> very revealing though ... seems to be within experimental error ;-(

What happens if you just turn BUG_ON into "do {} while (0)"?

Ciao,

Duncan.

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

* Re: BUGed to death
  2003-04-15 11:57         ` Duncan Sands
@ 2003-04-15 12:05           ` Dave Jones
  2003-04-15 14:39             ` Martin J. Bligh
  2003-04-23 15:08             ` Duncan Sands
  0 siblings, 2 replies; 35+ messages in thread
From: Dave Jones @ 2003-04-15 12:05 UTC (permalink / raw)
  To: Duncan Sands; +Cc: Martin J. Bligh, Andrew Morton, linux-kernel

On Tue, Apr 15, 2003 at 01:57:32PM +0200, Duncan Sands wrote:

 > It is questionable.  Since even in core kernel code there are
 > many places with
 > 	if (cond)
 > 		BUG();
 > rather than
 > 	BUG_ON(cond);
 > it may be worth seeing if converting them makes a difference
 > (increases code size though).

The spinlock code sticks out as a possible good target.
Any takers for benchmarking ?

		Dave


diff -urpN --exclude-from=/home/davej/.exclude bk-linus/include/asm-i386/mmu_context.h linux-2.5/include/asm-i386/mmu_context.h
--- bk-linus/include/asm-i386/mmu_context.h	2003-04-10 06:01:31.000000000 +0100
+++ linux-2.5/include/asm-i386/mmu_context.h	2003-04-15 06:02:46.000000000 +0100
@@ -45,8 +45,8 @@ static inline void switch_mm(struct mm_s
 #ifdef CONFIG_SMP
 	else {
 		cpu_tlbstate[cpu].state = TLBSTATE_OK;
-		if (cpu_tlbstate[cpu].active_mm != next)
-			BUG();
+		BUG_ON (cpu_tlbstate[cpu].active_mm != next);
+
 		if (!test_and_set_bit(cpu, &next->cpu_vm_mask)) {
 			/* We were in lazy tlb mode and leave_mm disabled 
 			 * tlb flush IPI delivery. We must reload %cr3.
diff -urpN --exclude-from=/home/davej/.exclude bk-linus/include/asm-i386/spinlock.h linux-2.5/include/asm-i386/spinlock.h
--- bk-linus/include/asm-i386/spinlock.h	2003-04-10 06:01:31.000000000 +0100
+++ linux-2.5/include/asm-i386/spinlock.h	2003-04-15 06:02:46.000000000 +0100
@@ -5,6 +5,7 @@
 #include <asm/rwlock.h>
 #include <asm/page.h>
 #include <linux/config.h>
+#include <linux/compiler.h>
 
 extern int printk(const char * fmt, ...)
 	__attribute__ ((format (printf, 1, 2)));
@@ -70,10 +71,8 @@ typedef struct {
 static inline void _raw_spin_unlock(spinlock_t *lock)
 {
 #ifdef CONFIG_DEBUG_SPINLOCK
-	if (lock->magic != SPINLOCK_MAGIC)
-		BUG();
-	if (!spin_is_locked(lock))
-		BUG();
+	BUG_ON (lock->magic != SPINLOCK_MAGIC);
+	BUG_ON (!spin_is_locked(lock));
 #endif
 	__asm__ __volatile__(
 		spin_unlock_string
@@ -91,10 +90,8 @@ static inline void _raw_spin_unlock(spin
 {
 	char oldval = 1;
 #ifdef CONFIG_DEBUG_SPINLOCK
-	if (lock->magic != SPINLOCK_MAGIC)
-		BUG();
-	if (!spin_is_locked(lock))
-		BUG();
+	BUG_ON (lock->magic != SPINLOCK_MAGIC);
+	BUG_ON (!spin_is_locked(lock));
 #endif
 	__asm__ __volatile__(
 		spin_unlock_string
@@ -118,8 +115,8 @@ static inline void _raw_spin_lock(spinlo
 #ifdef CONFIG_DEBUG_SPINLOCK
 	__label__ here;
 here:
-	if (lock->magic != SPINLOCK_MAGIC) {
-printk("eip: %p\n", &&here);
+	if (unlikely(lock->magic != SPINLOCK_MAGIC)) {
+		printk("eip: %p\n", &&here);
 		BUG();
 	}
 #endif
@@ -174,8 +171,7 @@ typedef struct {
 static inline void _raw_read_lock(rwlock_t *rw)
 {
 #ifdef CONFIG_DEBUG_SPINLOCK
-	if (rw->magic != RWLOCK_MAGIC)
-		BUG();
+	BUG_ON (rw->magic != RWLOCK_MAGIC);
 #endif
 	__build_read_lock(rw, "__read_lock_failed");
 }
@@ -183,8 +179,7 @@ static inline void _raw_read_lock(rwlock
 static inline void _raw_write_lock(rwlock_t *rw)
 {
 #ifdef CONFIG_DEBUG_SPINLOCK
-	if (rw->magic != RWLOCK_MAGIC)
-		BUG();
+	BUG_ON (rw->magic != RWLOCK_MAGIC);
 #endif
 	__build_write_lock(rw, "__write_lock_failed");
 }

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

* Re: BUGed to death
  2003-04-15 12:01 ` Duncan Sands
@ 2003-04-15 12:31   ` Jens Axboe
  2003-04-15 12:36     ` Dave Jones
                       ` (3 more replies)
  2003-04-15 14:35   ` Martin J. Bligh
  1 sibling, 4 replies; 35+ messages in thread
From: Jens Axboe @ 2003-04-15 12:31 UTC (permalink / raw)
  To: Duncan Sands; +Cc: Martin J. Bligh, Andrew Morton, linux-kernel

On Tue, Apr 15 2003, Duncan Sands wrote:
> On Monday 14 April 2003 22:19, Martin J. Bligh wrote:
> > Seems all these bug checks are fairly expensive. I can get 1%
> > back on system time for kernel compiles by changing BUG to
> > "do {} while (0)" to make them all compile away. Profiles aren't
> > very revealing though ... seems to be within experimental error ;-(
> 
> What happens if you just turn BUG_ON into "do {} while (0)"?

If you do that, you must audit every single BUG_ON to make sure the
expression doesn't have any side effects.

	BUG_ON(do_the_good_stuff());

-- 
Jens Axboe, proud inventer of BUG_ON :-)


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

* Re: BUGed to death
  2003-04-15 12:31   ` Jens Axboe
@ 2003-04-15 12:36     ` Dave Jones
  2003-04-15 12:40       ` Jens Axboe
  2003-04-15 12:49     ` Sean Neakums
                       ` (2 subsequent siblings)
  3 siblings, 1 reply; 35+ messages in thread
From: Dave Jones @ 2003-04-15 12:36 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Duncan Sands, Martin J. Bligh, Andrew Morton, linux-kernel

On Tue, Apr 15, 2003 at 02:31:34PM +0200, Jens Axboe wrote:

 > If you do that, you must audit every single BUG_ON to make sure the
 > expression doesn't have any side effects.
 > 
 > 	BUG_ON(do_the_good_stuff());

Sure, but such a construct looks really bad anyway.
Relying on side-effects of whats essentially a debug macro
sounds very dodgy.

	foo = do_the_good_stuff();
	BUG_ON (foo==baz)

Would be a better way of expressing this.

		Dave


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

* Re: BUGed to death
  2003-04-15 12:36     ` Dave Jones
@ 2003-04-15 12:40       ` Jens Axboe
  0 siblings, 0 replies; 35+ messages in thread
From: Jens Axboe @ 2003-04-15 12:40 UTC (permalink / raw)
  To: Dave Jones, Duncan Sands, Martin J. Bligh, Andrew Morton, linux-kernel

On Tue, Apr 15 2003, Dave Jones wrote:
> On Tue, Apr 15, 2003 at 02:31:34PM +0200, Jens Axboe wrote:
> 
>  > If you do that, you must audit every single BUG_ON to make sure the
>  > expression doesn't have any side effects.
>  > 
>  > 	BUG_ON(do_the_good_stuff());
> 
> Sure, but such a construct looks really bad anyway.
> Relying on side-effects of whats essentially a debug macro
> sounds very dodgy.
> 
> 	foo = do_the_good_stuff();
> 	BUG_ON (foo==baz)
> 
> Would be a better way of expressing this.

Oh I agree, it wasn't an example of good code :). It still needs to be
audited though.

-- 
Jens Axboe


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

* Re: BUGed to death
  2003-04-15 12:31   ` Jens Axboe
  2003-04-15 12:36     ` Dave Jones
@ 2003-04-15 12:49     ` Sean Neakums
  2003-04-15 12:52       ` Sean Neakums
  2003-04-15 13:01     ` Roman Zippel
  2003-04-15 13:55     ` Duncan Sands
  3 siblings, 1 reply; 35+ messages in thread
From: Sean Neakums @ 2003-04-15 12:49 UTC (permalink / raw)
  To: linux-kernel

Jens Axboe <axboe@suse.de> writes:

> On Tue, Apr 15 2003, Duncan Sands wrote:
>> On Monday 14 April 2003 22:19, Martin J. Bligh wrote:
>> > Seems all these bug checks are fairly expensive. I can get 1%
>> > back on system time for kernel compiles by changing BUG to
>> > "do {} while (0)" to make them all compile away. Profiles aren't
>> > very revealing though ... seems to be within experimental error ;-(
>> 
>> What happens if you just turn BUG_ON into "do {} while (0)"?
>
> If you do that, you must audit every single BUG_ON to make sure the
> expression doesn't have any side effects.
>
> 	BUG_ON(do_the_good_stuff());

#define BUG_ON(x) x; do the trick.  With any luck the compiler will
throw away most of the simple comparisons and whatnot.

-- 
Sean Neakums - <sneakums@zork.net>

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

* Re: BUGed to death
  2003-04-15 12:49     ` Sean Neakums
@ 2003-04-15 12:52       ` Sean Neakums
  0 siblings, 0 replies; 35+ messages in thread
From: Sean Neakums @ 2003-04-15 12:52 UTC (permalink / raw)
  To: linux-kernel

Sean Neakums <sneakums@zork.net> writes:

> #define BUG_ON(x) x; do the trick.  With any luck the compiler will
> throw away most of the simple comparisons and whatnot.

s/;/ might/

-- 
Sean Neakums - <sneakums@zork.net>

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

* Re: BUGed to death
  2003-04-15 12:31   ` Jens Axboe
  2003-04-15 12:36     ` Dave Jones
  2003-04-15 12:49     ` Sean Neakums
@ 2003-04-15 13:01     ` Roman Zippel
  2003-04-15 13:17       ` Jens Axboe
  2003-04-15 13:55     ` Duncan Sands
  3 siblings, 1 reply; 35+ messages in thread
From: Roman Zippel @ 2003-04-15 13:01 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Duncan Sands, Martin J. Bligh, Andrew Morton, linux-kernel

Hi,

On Tue, 15 Apr 2003, Jens Axboe wrote:

> > What happens if you just turn BUG_ON into "do {} while (0)"?
> 
> If you do that, you must audit every single BUG_ON to make sure the
> expression doesn't have any side effects.
> 
> 	BUG_ON(do_the_good_stuff());

This should avoid the problem:

#define BUG_ON(cond) do { if (cond); } while (0)

bye, Roman


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

* Re: BUGed to death
  2003-04-15 13:01     ` Roman Zippel
@ 2003-04-15 13:17       ` Jens Axboe
  0 siblings, 0 replies; 35+ messages in thread
From: Jens Axboe @ 2003-04-15 13:17 UTC (permalink / raw)
  To: Roman Zippel; +Cc: Duncan Sands, Martin J. Bligh, Andrew Morton, linux-kernel

On Tue, Apr 15 2003, Roman Zippel wrote:
> Hi,
> 
> On Tue, 15 Apr 2003, Jens Axboe wrote:
> 
> > > What happens if you just turn BUG_ON into "do {} while (0)"?
> > 
> > If you do that, you must audit every single BUG_ON to make sure the
> > expression doesn't have any side effects.
> > 
> > 	BUG_ON(do_the_good_stuff());
> 
> This should avoid the problem:
> 
> #define BUG_ON(cond) do { if (cond); } while (0)

Yes I'm aware of the problem being fixable, the above is not likely to
be fast than BUG_ON(). My point was just that you cannot simply count on
being able to make BUG_ON a noop, some thought has to go into it.

-- 
Jens Axboe


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

* Re: BUGed to death
  2003-04-15 12:31   ` Jens Axboe
                       ` (2 preceding siblings ...)
  2003-04-15 13:01     ` Roman Zippel
@ 2003-04-15 13:55     ` Duncan Sands
  2003-04-15 14:22       ` Jens Axboe
  3 siblings, 1 reply; 35+ messages in thread
From: Duncan Sands @ 2003-04-15 13:55 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Martin J. Bligh, Andrew Morton, linux-kernel

> If you do that, you must audit every single BUG_ON to make sure the
> expression doesn't have any side effects.
>
> 	BUG_ON(do_the_good_stuff());

Good point, but easily dealt with (see other posts).

All the best,

Duncan.

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

* Re: BUGed to death
  2003-04-15 13:55     ` Duncan Sands
@ 2003-04-15 14:22       ` Jens Axboe
  0 siblings, 0 replies; 35+ messages in thread
From: Jens Axboe @ 2003-04-15 14:22 UTC (permalink / raw)
  To: Duncan Sands; +Cc: Martin J. Bligh, Andrew Morton, linux-kernel

On Tue, Apr 15 2003, Duncan Sands wrote:
> > If you do that, you must audit every single BUG_ON to make sure the
> > expression doesn't have any side effects.
> >
> > 	BUG_ON(do_the_good_stuff());
> 
> Good point, but easily dealt with (see other posts).

I'm more inclined to agree with the view that it's a tool chain problem,
if the path leading to the bug isn't considered unlikely.

-- 
Jens Axboe


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

* Re: BUGed to death
  2003-04-15 12:01 ` Duncan Sands
  2003-04-15 12:31   ` Jens Axboe
@ 2003-04-15 14:35   ` Martin J. Bligh
  2003-04-15 14:39     ` Duncan Sands
  1 sibling, 1 reply; 35+ messages in thread
From: Martin J. Bligh @ 2003-04-15 14:35 UTC (permalink / raw)
  To: Duncan Sands, Andrew Morton; +Cc: linux-kernel

>> Seems all these bug checks are fairly expensive. I can get 1%
>> back on system time for kernel compiles by changing BUG to
>> "do {} while (0)" to make them all compile away. Profiles aren't
>> very revealing though ... seems to be within experimental error ;-(
> 
> What happens if you just turn BUG_ON into "do {} while (0)"?

I believe I already did that by turning BUG() into a null expression.

#define BUG_ON(condition) 
	do { if (unlikely((condition)!=0)) BUG(); } while(0)

The compiler should be smart enough to optimise that away, methinks.

M.

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

* Re: BUGed to death
  2003-04-15 12:05           ` Dave Jones
@ 2003-04-15 14:39             ` Martin J. Bligh
  2003-04-23 15:08             ` Duncan Sands
  1 sibling, 0 replies; 35+ messages in thread
From: Martin J. Bligh @ 2003-04-15 14:39 UTC (permalink / raw)
  To: Dave Jones, Duncan Sands; +Cc: Andrew Morton, linux-kernel

>  > It is questionable.  Since even in core kernel code there are
>  > many places with
>  > 	if (cond)
>  > 		BUG();
>  > rather than
>  > 	BUG_ON(cond);
>  > it may be worth seeing if converting them makes a difference
>  > (increases code size though).
> 
> The spinlock code sticks out as a possible good target.
> Any takers for benchmarking ?

I can try that ... However, do I have to use gcc > 3 to see anything?
I had some vague recollection that unlikely() doesn't do much for gcc 2.95,
is that correct?

M.



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

* Re: BUGed to death
  2003-04-15 14:35   ` Martin J. Bligh
@ 2003-04-15 14:39     ` Duncan Sands
  2003-04-15 14:45       ` Martin J. Bligh
  0 siblings, 1 reply; 35+ messages in thread
From: Duncan Sands @ 2003-04-15 14:39 UTC (permalink / raw)
  To: Martin J. Bligh, Andrew Morton; +Cc: linux-kernel

On Tuesday 15 April 2003 16:35, Martin J. Bligh wrote:
> >> Seems all these bug checks are fairly expensive. I can get 1%
> >> back on system time for kernel compiles by changing BUG to
> >> "do {} while (0)" to make them all compile away. Profiles aren't
> >> very revealing though ... seems to be within experimental error ;-(
> >
> > What happens if you just turn BUG_ON into "do {} while (0)"?
>
> I believe I already did that by turning BUG() into a null expression.
>
> #define BUG_ON(condition)
> 	do { if (unlikely((condition)!=0)) BUG(); } while(0)
>
> The compiler should be smart enough to optimise that away, methinks.

No, I meant what happens if BUG() is non-trivial and BUG_ON is a no-op.
I thought it might give an indication of whether time was being lost
evaluating the condition (occurs with BUG and BUG_ON), or mispredicting
the branch (only occurs with BUG).

Duncan.

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

* Re: BUGed to death
  2003-04-15 14:39     ` Duncan Sands
@ 2003-04-15 14:45       ` Martin J. Bligh
  2003-04-15 14:58         ` Duncan Sands
  0 siblings, 1 reply; 35+ messages in thread
From: Martin J. Bligh @ 2003-04-15 14:45 UTC (permalink / raw)
  To: Duncan Sands, Andrew Morton; +Cc: linux-kernel

>> >> Seems all these bug checks are fairly expensive. I can get 1%
>> >> back on system time for kernel compiles by changing BUG to
>> >> "do {} while (0)" to make them all compile away. Profiles aren't
>> >> very revealing though ... seems to be within experimental error ;-(
>> > 
>> > What happens if you just turn BUG_ON into "do {} while (0)"?
>> 
>> I believe I already did that by turning BUG() into a null expression.
>> 
>> # define BUG_ON(condition)
>> 	do { if (unlikely((condition)!=0)) BUG(); } while(0)
>> 
>> The compiler should be smart enough to optimise that away, methinks.
> 
> No, I meant what happens if BUG() is non-trivial and BUG_ON is a no-op.
> I thought it might give an indication of whether time was being lost
> evaluating the condition (occurs with BUG and BUG_ON), or mispredicting
> the branch (only occurs with BUG).

Mmmm. wouldn't that just optimise away the BUG_ONs, but not the BUGs ?
Which will tell you something, but I'm not sure anything interesting.
My impression is that if I do:

if (foo)
	do {} while (0);

the compiler will just ditch the whole thing, and not evaluate foo.
I'll admit to not having checked that, but still ....

M.


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

* Re: BUGed to death
  2003-04-15 14:45       ` Martin J. Bligh
@ 2003-04-15 14:58         ` Duncan Sands
  0 siblings, 0 replies; 35+ messages in thread
From: Duncan Sands @ 2003-04-15 14:58 UTC (permalink / raw)
  To: Martin J. Bligh, Andrew Morton; +Cc: linux-kernel

> > No, I meant what happens if BUG() is non-trivial and BUG_ON is a no-op.
> > I thought it might give an indication of whether time was being lost
> > evaluating the condition (occurs with BUG and BUG_ON), or mispredicting
> > the branch (only occurs with BUG).
>
> Mmmm. wouldn't that just optimise away the BUG_ONs, but not the BUGs ?
> Which will tell you something, but I'm not sure anything interesting.
> My impression is that if I do:
>
> if (foo)
> 	do {} while (0);
>
> the compiler will just ditch the whole thing, and not evaluate foo.
> I'll admit to not having checked that, but still ....

Good point.  So much for trying to get info without converting a bunch
of BUG's into BUG_ON's...

Duncan.

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

* Re: BUGed to death
  2003-04-15 12:05           ` Dave Jones
  2003-04-15 14:39             ` Martin J. Bligh
@ 2003-04-23 15:08             ` Duncan Sands
  1 sibling, 0 replies; 35+ messages in thread
From: Duncan Sands @ 2003-04-23 15:08 UTC (permalink / raw)
  To: Dave Jones; +Cc: Martin J. Bligh, Andrew Morton, linux-kernel

> The spinlock code sticks out as a possible good target.
> Any takers for benchmarking ?

How about this one  (i386 only)?
Before: vmlinux -> 28730335 bytes
After: vmlinux -> 28738567 bytes (+8k, i.e. +0.03%)
This is with a fairly maximal .config.
	
 asm-i386/mmu_context.h |    3 +--
 asm-i386/spinlock.h    |   18 ++++++------------
 linux/aio.h            |    4 ++--
 linux/bio.h            |    3 +--
 linux/buffer_head.h    |    3 +--
 linux/dcache.h         |    3 +--
 linux/highmem.h        |    3 +--
 linux/netdevice.h      |    2 +-
 linux/nfs_fs.h         |    3 +--
 linux/quotaops.h       |    6 ++----
 linux/skbuff.h         |   15 +++++----------
 linux/smp_lock.h       |    3 +--
 net/irda/vlsi_ir.h     |    5 +----
 net/sctp/sm.h          |    3 +--
 net/tcp.h              |    2 +-
 rxrpc/call.h           |    3 +--
 rxrpc/connection.h     |    3 +--
 rxrpc/message.h        |    3 +--
 rxrpc/peer.h           |    3 +--
 rxrpc/transport.h      |    3 +--
 20 files changed, 31 insertions(+), 60 deletions(-)


diff -Nru a/include/asm-i386/mmu_context.h b/include/asm-i386/mmu_context.h
--- a/include/asm-i386/mmu_context.h	Wed Apr 23 17:03:35 2003
+++ b/include/asm-i386/mmu_context.h	Wed Apr 23 17:03:35 2003
@@ -45,8 +45,7 @@
 #ifdef CONFIG_SMP
 	else {
 		cpu_tlbstate[cpu].state = TLBSTATE_OK;
-		if (cpu_tlbstate[cpu].active_mm != next)
-			BUG();
+		BUG_ON(cpu_tlbstate[cpu].active_mm != next);
 		if (!test_and_set_bit(cpu, &next->cpu_vm_mask)) {
 			/* We were in lazy tlb mode and leave_mm disabled 
 			 * tlb flush IPI delivery. We must reload %cr3.
diff -Nru a/include/asm-i386/spinlock.h b/include/asm-i386/spinlock.h
--- a/include/asm-i386/spinlock.h	Wed Apr 23 17:03:35 2003
+++ b/include/asm-i386/spinlock.h	Wed Apr 23 17:03:35 2003
@@ -70,10 +70,8 @@
 static inline void _raw_spin_unlock(spinlock_t *lock)
 {
 #ifdef CONFIG_DEBUG_SPINLOCK
-	if (lock->magic != SPINLOCK_MAGIC)
-		BUG();
-	if (!spin_is_locked(lock))
-		BUG();
+	BUG_ON(lock->magic != SPINLOCK_MAGIC);
+	BUG_ON(!spin_is_locked(lock));
 #endif
 	__asm__ __volatile__(
 		spin_unlock_string
@@ -91,10 +89,8 @@
 {
 	char oldval = 1;
 #ifdef CONFIG_DEBUG_SPINLOCK
-	if (lock->magic != SPINLOCK_MAGIC)
-		BUG();
-	if (!spin_is_locked(lock))
-		BUG();
+	BUG_ON(lock->magic != SPINLOCK_MAGIC);
+	BUG_ON(!spin_is_locked(lock));
 #endif
 	__asm__ __volatile__(
 		spin_unlock_string
@@ -174,8 +170,7 @@
 static inline void _raw_read_lock(rwlock_t *rw)
 {
 #ifdef CONFIG_DEBUG_SPINLOCK
-	if (rw->magic != RWLOCK_MAGIC)
-		BUG();
+	BUG_ON(rw->magic != RWLOCK_MAGIC);
 #endif
 	__build_read_lock(rw, "__read_lock_failed");
 }
@@ -183,8 +178,7 @@
 static inline void _raw_write_lock(rwlock_t *rw)
 {
 #ifdef CONFIG_DEBUG_SPINLOCK
-	if (rw->magic != RWLOCK_MAGIC)
-		BUG();
+	BUG_ON(rw->magic != RWLOCK_MAGIC);
 #endif
 	__build_write_lock(rw, "__write_lock_failed");
 }
diff -Nru a/include/linux/aio.h b/include/linux/aio.h
--- a/include/linux/aio.h	Wed Apr 23 17:03:35 2003
+++ b/include/linux/aio.h	Wed Apr 23 17:03:35 2003
@@ -153,8 +153,8 @@
 int FASTCALL(io_submit_one(struct kioctx *ctx, struct iocb *user_iocb,
 				  struct iocb *iocb));
 
-#define get_ioctx(kioctx)	do { if (unlikely(atomic_read(&(kioctx)->users) <= 0)) BUG(); atomic_inc(&(kioctx)->users); } while (0)
-#define put_ioctx(kioctx)	do { if (unlikely(atomic_dec_and_test(&(kioctx)->users))) __put_ioctx(kioctx); else if (unlikely(atomic_read(&(kioctx)->users) < 0)) BUG(); } while (0)
+#define get_ioctx(kioctx)	do { BUG_ON(atomic_read(&(kioctx)->users) <= 0); atomic_inc(&(kioctx)->users); } while (0)
+#define put_ioctx(kioctx)	do { if (unlikely(atomic_dec_and_test(&(kioctx)->users))) __put_ioctx(kioctx); else BUG_ON(atomic_read(&(kioctx)->users) < 0); } while (0)
 
 #include <linux/aio_abi.h>
 
diff -Nru a/include/linux/bio.h b/include/linux/bio.h
--- a/include/linux/bio.h	Wed Apr 23 17:03:35 2003
+++ b/include/linux/bio.h	Wed Apr 23 17:03:35 2003
@@ -242,8 +242,7 @@
 	local_irq_save(*flags);
 	addr = (unsigned long) kmap_atomic(bio_page(bio), KM_BIO_SRC_IRQ);
 
-	if (addr & ~PAGE_MASK)
-		BUG();
+	BUG_ON(addr & ~PAGE_MASK);
 
 	return (char *) addr + bio_offset(bio);
 }
diff -Nru a/include/linux/buffer_head.h b/include/linux/buffer_head.h
--- a/include/linux/buffer_head.h	Wed Apr 23 17:03:35 2003
+++ b/include/linux/buffer_head.h	Wed Apr 23 17:03:35 2003
@@ -122,8 +122,7 @@
 /* If we *know* page->private refers to buffer_heads */
 #define page_buffers(page)					\
 	({							\
-		if (!PagePrivate(page))				\
-			BUG();					\
+		BUG_ON(!PagePrivate(page));			\
 		((struct buffer_head *)(page)->private);	\
 	})
 #define page_has_buffers(page)	PagePrivate(page)
diff -Nru a/include/linux/dcache.h b/include/linux/dcache.h
--- a/include/linux/dcache.h	Wed Apr 23 17:03:35 2003
+++ b/include/linux/dcache.h	Wed Apr 23 17:03:35 2003
@@ -267,8 +267,7 @@
 static inline struct dentry *dget(struct dentry *dentry)
 {
 	if (dentry) {
-		if (!atomic_read(&dentry->d_count))
-			BUG();
+		BUG_ON(!atomic_read(&dentry->d_count));
 		atomic_inc(&dentry->d_count);
 	}
 	return dentry;
diff -Nru a/include/linux/highmem.h b/include/linux/highmem.h
--- a/include/linux/highmem.h	Wed Apr 23 17:03:35 2003
+++ b/include/linux/highmem.h	Wed Apr 23 17:03:35 2003
@@ -61,8 +61,7 @@
 {
 	void *kaddr;
 
-	if (offset + size > PAGE_SIZE)
-		BUG();
+	BUG_ON(offset + size > PAGE_SIZE);
 
 	kaddr = kmap_atomic(page, KM_USER0);
 	memset((char *)kaddr + offset, 0, size);
diff -Nru a/include/linux/netdevice.h b/include/linux/netdevice.h
--- a/include/linux/netdevice.h	Wed Apr 23 17:03:35 2003
+++ b/include/linux/netdevice.h	Wed Apr 23 17:03:35 2003
@@ -794,7 +794,7 @@
 	unsigned long flags;
 
 	local_irq_save(flags);
-	if (!test_bit(__LINK_STATE_RX_SCHED, &dev->state)) BUG();
+	BUG_ON(!test_bit(__LINK_STATE_RX_SCHED, &dev->state));
 	list_del(&dev->poll_list);
 	clear_bit(__LINK_STATE_RX_SCHED, &dev->state);
 	local_irq_restore(flags);
diff -Nru a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
--- a/include/linux/nfs_fs.h	Wed Apr 23 17:03:35 2003
+++ b/include/linux/nfs_fs.h	Wed Apr 23 17:03:35 2003
@@ -260,8 +260,7 @@
 	if (file)
 		cred = (struct rpc_cred *)file->private_data;
 #ifdef RPC_DEBUG
-	if (cred && cred->cr_magic != RPCAUTH_CRED_MAGIC)
-		BUG();
+	BUG_ON(cred && cred->cr_magic != RPCAUTH_CRED_MAGIC);
 #endif
 	return cred;
 }
diff -Nru a/include/linux/quotaops.h b/include/linux/quotaops.h
--- a/include/linux/quotaops.h	Wed Apr 23 17:03:35 2003
+++ b/include/linux/quotaops.h	Wed Apr 23 17:03:35 2003
@@ -44,8 +44,7 @@
 
 static __inline__ void DQUOT_INIT(struct inode *inode)
 {
-	if (!inode->i_sb)
-		BUG();
+	BUG_ON(!inode->i_sb);
 	if (sb_any_quota_enabled(inode->i_sb) && !IS_NOQUOTA(inode))
 		inode->i_sb->dq_op->initialize(inode, -1);
 }
@@ -53,8 +52,7 @@
 static __inline__ void DQUOT_DROP(struct inode *inode)
 {
 	if (IS_QUOTAINIT(inode)) {
-		if (!inode->i_sb)
-			BUG();
+		BUG_ON(!inode->i_sb);
 		inode->i_sb->dq_op->drop(inode);	/* Ops must be set when there's any quota... */
 	}
 }
diff -Nru a/include/linux/skbuff.h b/include/linux/skbuff.h
--- a/include/linux/skbuff.h	Wed Apr 23 17:03:35 2003
+++ b/include/linux/skbuff.h	Wed Apr 23 17:03:35 2003
@@ -792,12 +792,9 @@
 	return len + skb_headlen(skb);
 }
 
-#define SKB_PAGE_ASSERT(skb) do { if (skb_shinfo(skb)->nr_frags) \
-					BUG(); } while (0)
-#define SKB_FRAG_ASSERT(skb) do { if (skb_shinfo(skb)->frag_list) \
-					BUG(); } while (0)
-#define SKB_LINEAR_ASSERT(skb) do { if (skb_is_nonlinear(skb)) \
-					BUG(); } while (0)
+#define SKB_PAGE_ASSERT(skb) BUG_ON(skb_shinfo(skb)->nr_frags)
+#define SKB_FRAG_ASSERT(skb) BUG_ON(skb_shinfo(skb)->frag_list)
+#define SKB_LINEAR_ASSERT(skb) BUG_ON(skb_is_nonlinear(skb))
 
 /*
  *	Add data to an sk_buff
@@ -859,8 +856,7 @@
 static inline char *__skb_pull(struct sk_buff *skb, unsigned int len)
 {
 	skb->len -= len;
-	if (skb->len < skb->data_len)
-		BUG();
+	BUG_ON(skb->len < skb->data_len);
 	return skb->data += len;
 }
 
@@ -1122,8 +1118,7 @@
 static inline void *kmap_skb_frag(const skb_frag_t *frag)
 {
 #ifdef CONFIG_HIGHMEM
-	if (in_irq())
-		BUG();
+	BUG_ON(in_irq());
 
 	local_bh_disable();
 #endif
diff -Nru a/include/linux/smp_lock.h b/include/linux/smp_lock.h
--- a/include/linux/smp_lock.h	Wed Apr 23 17:03:35 2003
+++ b/include/linux/smp_lock.h	Wed Apr 23 17:03:35 2003
@@ -49,8 +49,7 @@
 
 static inline void unlock_kernel(void)
 {
-	if (unlikely(current->lock_depth < 0))
-		BUG();
+	BUG_ON(current->lock_depth < 0);
 	if (likely(--current->lock_depth < 0))
 		put_kernel_lock();
 }
diff -Nru a/include/net/irda/vlsi_ir.h b/include/net/irda/vlsi_ir.h
--- a/include/net/irda/vlsi_ir.h	Wed Apr 23 17:03:35 2003
+++ b/include/net/irda/vlsi_ir.h	Wed Apr 23 17:03:35 2003
@@ -615,10 +615,7 @@
 	 *    case status has RD_ACTIVE set
 	 */
 
-	if ((a & ~DMA_MASK_MSTRPAGE)>>24 != MSTRPAGE_VALUE) {
-		BUG();
-		return;
-	}
+	BUG_ON((a & ~DMA_MASK_MSTRPAGE)>>24 != MSTRPAGE_VALUE);
 
 	a &= DMA_MASK_MSTRPAGE;  /* clear highbyte to make sure we won't write
 				  * to status - just in case MSTRPAGE_VALUE!=0
diff -Nru a/include/net/sctp/sm.h b/include/net/sctp/sm.h
--- a/include/net/sctp/sm.h	Wed Apr 23 17:03:35 2003
+++ b/include/net/sctp/sm.h	Wed Apr 23 17:03:35 2003
@@ -441,8 +441,7 @@
 /* Run sctp_add_cmd() generating a BUG() if there is a failure.  */
 static inline void sctp_add_cmd_sf(sctp_cmd_seq_t *seq, sctp_verb_t verb, sctp_arg_t obj)
 {
-	if (unlikely(!sctp_add_cmd(seq, verb, obj)))
-		BUG();
+	BUG_ON(!sctp_add_cmd(seq, verb, obj));
 }
 
 /* Check VTAG of the packet matches the sender's own tag OR its peer's
diff -Nru a/include/net/tcp.h b/include/net/tcp.h
--- a/include/net/tcp.h	Wed Apr 23 17:03:35 2003
+++ b/include/net/tcp.h	Wed Apr 23 17:03:35 2003
@@ -1361,7 +1361,7 @@
 		if (tp->ucopy.memory > sk->rcvbuf) {
 			struct sk_buff *skb1;
 
-			if (sock_owned_by_user(sk)) BUG();
+			BUG_ON(sock_owned_by_user(sk));
 
 			while ((skb1 = __skb_dequeue(&tp->ucopy.prequeue)) != NULL) {
 				sk->backlog_rcv(sk, skb1);
diff -Nru a/include/rxrpc/call.h b/include/rxrpc/call.h
--- a/include/rxrpc/call.h	Wed Apr 23 17:03:35 2003
+++ b/include/rxrpc/call.h	Wed Apr 23 17:03:35 2003
@@ -187,8 +187,7 @@
 
 static inline void rxrpc_get_call(struct rxrpc_call *call)
 {
-	if (atomic_read(&call->usage)<=0)
-		BUG();
+	BUG_ON(atomic_read(&call->usage)<=0);
 	atomic_inc(&call->usage);
 	/*printk("rxrpc_get_call(%p{u=%d})\n",(C),atomic_read(&(C)->usage));*/
 }
diff -Nru a/include/rxrpc/connection.h b/include/rxrpc/connection.h
--- a/include/rxrpc/connection.h	Wed Apr 23 17:03:35 2003
+++ b/include/rxrpc/connection.h	Wed Apr 23 17:03:35 2003
@@ -66,8 +66,7 @@
 
 static inline void rxrpc_get_connection(struct rxrpc_connection *conn)
 {
-	if (atomic_read(&conn->usage)<0)
-		BUG();
+	BUG_ON(atomic_read(&conn->usage)<0);
 	atomic_inc(&conn->usage);
 	//printk("rxrpc_get_conn(%p{u=%d})\n",conn,atomic_read(&conn->usage));
 }
diff -Nru a/include/rxrpc/message.h b/include/rxrpc/message.h
--- a/include/rxrpc/message.h	Wed Apr 23 17:03:35 2003
+++ b/include/rxrpc/message.h	Wed Apr 23 17:03:35 2003
@@ -53,8 +53,7 @@
 extern void __rxrpc_put_message(struct rxrpc_message *msg);
 static inline void rxrpc_put_message(struct rxrpc_message *msg)
 {
-	if (atomic_read(&msg->usage)<=0)
-		BUG();
+	BUG_ON(atomic_read(&msg->usage)<=0);
 	if (atomic_dec_and_test(&msg->usage))
 		__rxrpc_put_message(msg);
 }
diff -Nru a/include/rxrpc/peer.h b/include/rxrpc/peer.h
--- a/include/rxrpc/peer.h	Wed Apr 23 17:03:35 2003
+++ b/include/rxrpc/peer.h	Wed Apr 23 17:03:35 2003
@@ -69,8 +69,7 @@
 
 static inline void rxrpc_get_peer(struct rxrpc_peer *peer)
 {
-	if (atomic_read(&peer->usage)<0)
-		BUG();
+	BUG_ON(atomic_read(&peer->usage)<0);
 	atomic_inc(&peer->usage);
 	//printk("rxrpc_get_peer(%p{u=%d})\n",peer,atomic_read(&peer->usage));
 }
diff -Nru a/include/rxrpc/transport.h b/include/rxrpc/transport.h
--- a/include/rxrpc/transport.h	Wed Apr 23 17:03:35 2003
+++ b/include/rxrpc/transport.h	Wed Apr 23 17:03:35 2003
@@ -85,8 +85,7 @@
 
 static inline void rxrpc_get_transport(struct rxrpc_transport *trans)
 {
-	if (atomic_read(&trans->usage)<=0)
-		BUG();
+	BUG_ON(atomic_read(&trans->usage)<=0);
 	atomic_inc(&trans->usage);
 	//printk("rxrpc_get_transport(%p{u=%d})\n",trans,atomic_read(&trans->usage));
 }


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

* Re: BUGed to death
@ 2003-04-15 18:33 Chuck Ebbert
  0 siblings, 0 replies; 35+ messages in thread
From: Chuck Ebbert @ 2003-04-15 18:33 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-kernel


> If you do that, you must audit every single BUG_ON to make sure the
> expression doesn't have any side effects.
>
>	BUG_ON(do_the_good_stuff());


  Sounds like a candidate for machine audit.  You can declare
pure functions in GCC 2.96+ but I see no way to assert that an
expression is pure...



--
 Chuck

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

* Re: BUGed to death
  2003-04-15 16:42   ` Michael Buesch
@ 2003-04-15 16:45     ` Dave Jones
  0 siblings, 0 replies; 35+ messages in thread
From: Dave Jones @ 2003-04-15 16:45 UTC (permalink / raw)
  To: Michael Buesch; +Cc: linux-kernel

On Tue, Apr 15, 2003 at 06:42:35PM +0200, Michael Buesch wrote:
 > On Tuesday 15 April 2003 17:57, Dave Jones wrote:
 > > Imagine I pass in 20. Previously, the BUG triggers. Not any more.
 > > Ditto the other changes.  Or am _I_ missing something ?
 > 
 > Yes ;)
 > You're missing the (not posted) while loop before. Just look into source.

Yup. I stand corrected.

		Dave


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

* Re: BUGed to death
  2003-04-15 15:57 ` Dave Jones
  2003-04-15 16:11   ` Nick Piggin
@ 2003-04-15 16:42   ` Michael Buesch
  2003-04-15 16:45     ` Dave Jones
  1 sibling, 1 reply; 35+ messages in thread
From: Michael Buesch @ 2003-04-15 16:42 UTC (permalink / raw)
  To: Dave Jones; +Cc: linux-kernel

On Tuesday 15 April 2003 17:57, Dave Jones wrote:
> Imagine I pass in 20. Previously, the BUG triggers. Not any more.
> Ditto the other changes.  Or am _I_ missing something ?

Yes ;)
You're missing the (not posted) while loop before. Just look into source.

-- 
Regards Michael Buesch.
http://www.8ung.at/tuxsoft

$ cat /dev/zero > /dev/null
/dev/null: That's *not* funny! :(


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

* Re: BUGed to death
  2003-04-15 15:57 ` Dave Jones
@ 2003-04-15 16:11   ` Nick Piggin
  2003-04-15 16:42   ` Michael Buesch
  1 sibling, 0 replies; 35+ messages in thread
From: Nick Piggin @ 2003-04-15 16:11 UTC (permalink / raw)
  To: Dave Jones; +Cc: rwhron, linux-kernel, reiserfs-list

Dave Jones wrote:

>On Tue, Apr 15, 2003 at 10:30:24AM -0400, rwhron@earthlink.net wrote:
> > The patch below eliminates 4 BUG() calls that clearly 
> > cannot happen based on the context.
>
>This looks bogus.
>
> > --- linux-2.5.67-mm2/fs/reiserfs/hashes.c.orig	2003-04-15 10:11:44.000000000 -0400
> > +++ linux-2.5.67-mm2/fs/reiserfs/hashes.c	2003-04-15 10:13:43.000000000 -0400
> > @@ -90,10 +90,6 @@
> >  
> >  	if (len >= 12)
> >  	{
> > -	    	//assert(len < 16);
> > -		if (len >= 16)
> > -		    BUG();
> > -
>
>Imagine I pass in 20. Previously, the BUG triggers. Not any more.
>Ditto the other changes.  Or am _I_ missing something ?
>
Just from the context of the patch, you are right with the first
one. Subsequent bugs can be removed due to their possibility being
eliminated by previous if statements. The code suggests that
len will trivially not be >= 16 at this point, however. So the
patch is ok.


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

* Re: BUGed to death
  2003-04-15 14:30 rwhron
@ 2003-04-15 15:57 ` Dave Jones
  2003-04-15 16:11   ` Nick Piggin
  2003-04-15 16:42   ` Michael Buesch
  0 siblings, 2 replies; 35+ messages in thread
From: Dave Jones @ 2003-04-15 15:57 UTC (permalink / raw)
  To: rwhron; +Cc: linux-kernel, reiserfs-list

On Tue, Apr 15, 2003 at 10:30:24AM -0400, rwhron@earthlink.net wrote:
 > The patch below eliminates 4 BUG() calls that clearly 
 > cannot happen based on the context.

This looks bogus.

 > --- linux-2.5.67-mm2/fs/reiserfs/hashes.c.orig	2003-04-15 10:11:44.000000000 -0400
 > +++ linux-2.5.67-mm2/fs/reiserfs/hashes.c	2003-04-15 10:13:43.000000000 -0400
 > @@ -90,10 +90,6 @@
 >  
 >  	if (len >= 12)
 >  	{
 > -	    	//assert(len < 16);
 > -		if (len >= 16)
 > -		    BUG();
 > -

Imagine I pass in 20. Previously, the BUG triggers. Not any more.
Ditto the other changes.  Or am _I_ missing something ?

		Dave


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

* Re: BUGed to death
@ 2003-04-15 14:30 rwhron
  2003-04-15 15:57 ` Dave Jones
  0 siblings, 1 reply; 35+ messages in thread
From: rwhron @ 2003-04-15 14:30 UTC (permalink / raw)
  To: linux-kernel, reiserfs-list

The patch below eliminates 4 BUG() calls that clearly 
cannot happen based on the context.

--- linux-2.5.67-mm2/fs/reiserfs/hashes.c.orig	2003-04-15 10:11:44.000000000 -0400
+++ linux-2.5.67-mm2/fs/reiserfs/hashes.c	2003-04-15 10:13:43.000000000 -0400
@@ -90,10 +90,6 @@
 
 	if (len >= 12)
 	{
-	    	//assert(len < 16);
-		if (len >= 16)
-		    BUG();
-
 		a = (u32)msg[ 0]      |
 		    (u32)msg[ 1] << 8 |
 		    (u32)msg[ 2] << 16|
@@ -116,9 +112,6 @@
 	}
 	else if (len >= 8)
 	{
-	    	//assert(len < 12);
-		if (len >= 12)
-		    BUG();
 		a = (u32)msg[ 0]      |
 		    (u32)msg[ 1] << 8 |
 		    (u32)msg[ 2] << 16|
@@ -137,9 +130,6 @@
 	}
 	else if (len >= 4)
 	{
-	    	//assert(len < 8);
-		if (len >= 8)
-		    BUG();
 		a = (u32)msg[ 0]      |
 		    (u32)msg[ 1] << 8 |
 		    (u32)msg[ 2] << 16|
@@ -154,9 +144,6 @@
 	}
 	else
 	{
-	    	//assert(len < 4);
-		if (len >= 4)
-		    BUG();
 		a = b = c = d = pad;
 		for(i = 0; i < len; i++)
 		{
-- 
Randy Hron
http://home.earthlink.net/~rwhron/kernel/bigbox.html


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

end of thread, other threads:[~2003-04-23 14:56 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-04-14 20:19 BUGed to death Martin J. Bligh
2003-04-14 20:40 ` Duncan Sands
2003-04-14 21:02   ` Dave Jones
2003-04-14 21:10     ` Duncan Sands
2003-04-14 21:17       ` Dave Jones
2003-04-15 11:57         ` Duncan Sands
2003-04-15 12:05           ` Dave Jones
2003-04-15 14:39             ` Martin J. Bligh
2003-04-23 15:08             ` Duncan Sands
2003-04-14 21:00 ` Dave Jones
2003-04-14 20:55   ` Martin J. Bligh
2003-04-14 21:08     ` Dave Jones
2003-04-14 21:50       ` Andrew Morton
2003-04-14 21:55         ` Dave Jones
2003-04-15  0:23   ` H. Peter Anvin
2003-04-15 12:01 ` Duncan Sands
2003-04-15 12:31   ` Jens Axboe
2003-04-15 12:36     ` Dave Jones
2003-04-15 12:40       ` Jens Axboe
2003-04-15 12:49     ` Sean Neakums
2003-04-15 12:52       ` Sean Neakums
2003-04-15 13:01     ` Roman Zippel
2003-04-15 13:17       ` Jens Axboe
2003-04-15 13:55     ` Duncan Sands
2003-04-15 14:22       ` Jens Axboe
2003-04-15 14:35   ` Martin J. Bligh
2003-04-15 14:39     ` Duncan Sands
2003-04-15 14:45       ` Martin J. Bligh
2003-04-15 14:58         ` Duncan Sands
2003-04-15 14:30 rwhron
2003-04-15 15:57 ` Dave Jones
2003-04-15 16:11   ` Nick Piggin
2003-04-15 16:42   ` Michael Buesch
2003-04-15 16:45     ` Dave Jones
2003-04-15 18:33 Chuck Ebbert

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).