Skip to contentSkip to conversation

BUGed to death

DISCUSSION Martin J. Bligh 8 participants 29 messages Apr 14, 2003 – Apr 23, 2003

Subsystems (9)
Networking Network drivers Block layer NFS client +5 more subsystems
lore
Share & tools
lore

Martin J. Bligh

author
#0 lore raw

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
#1 lore raw
Quoted 9 lines · 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 ;-)

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

#2 lore raw
Quoted 3 lines · Duncan Sands

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

#3 lore raw
Quoted 5 lines

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;
}
#4 lore raw
Quoted 2 lines · Duncan Sands

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

#5 lore raw
Quoted 4 lines

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.

#6 lore raw
Quoted 8 lines · Duncan Sands

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

include/asm-i386/mmu_context.h +2 -2
diff -urpN --exclude-from=/home/davej/.exclude bk-linus/include/asm-i386/mmu_context.h linux-2.5/include/asm-i386/mmu_context.h
@@ -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.
include/asm-i386/spinlock.h +9 -14
diff -urpN --exclude-from=/home/davej/.exclude bk-linus/include/asm-i386/spinlock.h linux-2.5/include/asm-i386/spinlock.h
@@ -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");
 }
author
#7 lore raw
Quoted 11 lines
Quoted 8 lines

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.

#8 lore raw
In reply to Dave Jones
Quoted 2 lines

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(-)
include/asm-i386/mmu_context.h +1 -2
diff -Nru a/include/asm-i386/mmu_context.h b/include/asm-i386/mmu_context.h
@@ -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.
include/asm-i386/spinlock.h +6 -12
diff -Nru a/include/asm-i386/spinlock.h b/include/asm-i386/spinlock.h
@@ -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");
 }
include/linux/aio.h +2 -2
diff -Nru a/include/linux/aio.h b/include/linux/aio.h
@@ -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>
include/linux/bio.h +1 -2
diff -Nru a/include/linux/bio.h b/include/linux/bio.h
@@ -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);
 }
include/linux/buffer_head.h +1 -2
diff -Nru a/include/linux/buffer_head.h b/include/linux/buffer_head.h
@@ -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)
include/linux/dcache.h +1 -2
diff -Nru a/include/linux/dcache.h b/include/linux/dcache.h
@@ -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;
include/linux/highmem.h +1 -2
diff -Nru a/include/linux/highmem.h b/include/linux/highmem.h
@@ -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);
include/linux/netdevice.h +1 -1
diff -Nru a/include/linux/netdevice.h b/include/linux/netdevice.h
@@ -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);
include/linux/nfs_fs.h +1 -2
diff -Nru a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
@@ -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;
 }
include/linux/quotaops.h +2 -4
diff -Nru a/include/linux/quotaops.h b/include/linux/quotaops.h
@@ -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... */
 	}
 }
include/linux/skbuff.h +5 -10
diff -Nru a/include/linux/skbuff.h b/include/linux/skbuff.h
@@ -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
include/linux/smp_lock.h +1 -2
diff -Nru a/include/linux/smp_lock.h b/include/linux/smp_lock.h
@@ -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();
 }
include/net/irda/vlsi_ir.h +1 -4
diff -Nru a/include/net/irda/vlsi_ir.h b/include/net/irda/vlsi_ir.h
@@ -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
include/net/sctp/sm.h +1 -2
diff -Nru a/include/net/sctp/sm.h b/include/net/sctp/sm.h
@@ -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
include/net/tcp.h +1 -1
diff -Nru a/include/net/tcp.h b/include/net/tcp.h
@@ -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);
include/rxrpc/call.h +1 -2
diff -Nru a/include/rxrpc/call.h b/include/rxrpc/call.h
@@ -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));*/
 }
include/rxrpc/connection.h +1 -2
diff -Nru a/include/rxrpc/connection.h b/include/rxrpc/connection.h
@@ -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));
 }
include/rxrpc/message.h +1 -2
diff -Nru a/include/rxrpc/message.h b/include/rxrpc/message.h
@@ -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);
 }
include/rxrpc/peer.h +1 -2
diff -Nru a/include/rxrpc/peer.h b/include/rxrpc/peer.h
@@ -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));
 }
include/rxrpc/transport.h +1 -2
diff -Nru a/include/rxrpc/transport.h b/include/rxrpc/transport.h
@@ -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));
 }
#9 lore raw
In reply to Martin J. Bligh
Quoted 6 lines · 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.

Dave

author
#10 lore raw
Quoted 5 lines
Quoted 5 lines

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

Quoted 8 lines
Quoted 1 line

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.

#11 lore raw
Quoted 3 lines · Martin J. Bligh

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

#12 lore raw
Quoted 9 lines · Dave Jones
Quoted 3 lines · Martin J. Bligh

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.

#13 lore raw
Quoted 5 lines · Andrew Morton
	if (foo == NULL)
		BUG();
	*foo = bar;

The BUG is a waste of space.

Agreed. That is somewhat silly.

Dave

#14 lore raw
In reply to Dave Jones
Followup to:  <[email protected]>
By author:    Dave Jones <[email protected]>
In newsgroup: linux.dev.kernel
Quoted 8 lines

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

Signature · 3 lines
<[email protected]> at work, <[email protected]> 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
#15 lore raw
In reply to Martin J. Bligh
Quoted 4 lines · 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)"?

Ciao,

Duncan.

#16 lore raw
Quoted 7 lines · Duncan Sands
Quoted 4 lines · 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)"?

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());
Signature · 1 line
Jens Axboe, proud inventer of BUG_ON :-)
#17 lore raw
Quoted 4 lines · Jens Axboe

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

#18 lore raw
Quoted 7 lines · Dave Jones
Quoted 4 lines · Jens Axboe

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());
Quoted 8 lines

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.

Signature · 1 line
Jens Axboe
#19 lore raw
In reply to Jens Axboe
Quoted 7 lines · Jens Axboe
Quoted 6 lines · Duncan Sands
Quoted 4 lines · 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 ;-(

Quoted 6 lines
Quoted 1 line

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.

Signature · 1 line
Sean Neakums - <[email protected]>
#20 lore raw
Quoted 2 lines · Sean Neakums

#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/

Signature · 1 line
Sean Neakums - <[email protected]>
#21 lore raw
In reply to Jens Axboe

Hi,

Quoted 6 lines · Jens Axboe
Quoted 1 line

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

#22 lore raw
Quoted 6 lines · Roman Zippel

Hi,

Quoted 2 lines · Jens Axboe
Quoted 1 line

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

Quoted 8 lines
Quoted 4 lines

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.

Signature · 1 line
Jens Axboe
#23 lore raw
In reply to Jens Axboe
Quoted 4 lines

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.

#24 lore raw
Quoted 6 lines · Duncan Sands
Quoted 4 lines

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.

Signature · 1 line
Jens Axboe
author
#25 lore raw
In reply to Duncan Sands
Quoted 6 lines
Quoted 4 lines

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.

#26 lore raw
Quoted 5 lines · Martin J. Bligh
Quoted 5 lines
Quoted 4 lines

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

Quoted 8 lines
Quoted 1 line

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.

author
#27 lore raw
Quoted 12 lines
Quoted 12 lines
Quoted 6 lines
Quoted 4 lines

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)
Quoted 6 lines
Quoted 1 line

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.

#28 lore raw
Quoted 9 lines
Quoted 4 lines

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:

Quoted 5 lines

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.