BUGed to death

BUGed to death

[Martin J. Bligh]

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

Re: BUGed to death

[Duncan Sands]

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

Re: BUGed to death

[Dave Jones]

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

Re: BUGed to death

[Duncan Sands]

> 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;
}

Re: BUGed to death

[Dave Jones]

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

Re: BUGed to death

[Duncan Sands]

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

Re: BUGed to death

[Dave Jones]

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");
 }

Re: BUGed to death

[Martin J. Bligh]

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

Re: BUGed to death

[Duncan Sands]

> 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));
 }

Re: BUGed to death

[Dave Jones]

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

Re: BUGed to death

[Martin J. Bligh]

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

Re: BUGed to death

[Dave Jones]

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

Re: BUGed to death

[Andrew Morton]

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.

Re: BUGed to death

[Dave Jones]

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

Re: BUGed to death

[H. Peter Anvin]

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

Re: BUGed to death

[Duncan Sands]

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.

Re: BUGed to death

[Jens Axboe]

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

Re: BUGed to death

[Dave Jones]

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

Re: BUGed to death

[Jens Axboe]

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

Re: BUGed to death

[Sean Neakums]

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>

Re: BUGed to death

[Sean Neakums]

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>

Re: BUGed to death

[Roman Zippel]

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

Re: BUGed to death

[Jens Axboe]

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

Re: BUGed to death

[Duncan Sands]

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

Re: BUGed to death

[Jens Axboe]

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

Re: BUGed to death

[Martin J. Bligh]

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

Re: BUGed to death

[Duncan Sands]

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.

Re: BUGed to death

[Martin J. Bligh]

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

Re: BUGed to death

[Duncan Sands]

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

Re: BUGed to death

[rwhron]

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

Re: BUGed to death

[Dave Jones]

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

Re: BUGed to death

[Nick Piggin]

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.

Re: BUGed to death

[Michael Buesch]

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! :(

Re: BUGed to death

[Dave Jones]

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

Re: BUGed to death

[Chuck Ebbert]

> 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