[2.6 patch][0/3] BUG -> BUG_ON conversions

[2.6 patch][0/3] BUG -> BUG_ON conversions

[Adrian Bunk]

Hi Andrew,

in the following mails are the first [1] three patches that convert

  if(foo)
	BUG():

to

  BUG_ON(foo);


This makes the code slightly better readable and it might result in 
slightly better code with recent gcc versions due to the "unlikely" in 
the definition of BUG_ON (it might not be a measurable difference, but  
it comes for free).


Obviosly, in constructs like

  if (foo) {
	printk(KERN_ERR "some error");
	BUG();
  }

or

  switch (foo) {
  case A:
	...
	break;
  case B:
	...
	break;
  default:
	BUG();
  }


BUG() can't be replaced by BUG_ON(), and it's therefore unchanged.
  

cu
Adrian

[1] I plan to send more such patches.

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

[2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

[Adrian Bunk]

The patch below does BUG -> BUG_ON conversions in ipc/ .

diffstat output:
 ipc/msg.c  |    3 +--
 ipc/sem.c  |    6 ++----
 ipc/shm.c  |   12 ++++--------
 ipc/util.c |    6 ++----
 4 files changed, 9 insertions(+), 18 deletions(-)


Signed-off-by: Adrian Bunk <bunk@fs.tum.de>

--- linux-2.6.9-rc1-mm1-full-3.4/ipc/msg.c.old	2004-08-28 15:55:28.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/ipc/msg.c	2004-08-28 16:01:42.000000000 +0200
@@ -215,8 +215,7 @@
 		ret = -EEXIST;
 	} else {
 		msq = msg_lock(id);
-		if(msq==NULL)
-			BUG();
+		BUG_ON(msq == NULL);
 		if (ipcperms(&msq->q_perm, msgflg))
 			ret = -EACCES;
 		else {
--- linux-2.6.9-rc1-mm1-full-3.4/ipc/sem.c.old	2004-08-28 15:55:28.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/ipc/sem.c	2004-08-28 16:02:09.000000000 +0200
@@ -222,8 +222,7 @@
 		err = -EEXIST;
 	} else {
 		sma = sem_lock(id);
-		if(sma==NULL)
-			BUG();
+		BUG_ON(sma == NULL);
 		if (nsems > sma->sem_nsems)
 			err = -EINVAL;
 		else if (ipcperms(&sma->sem_perm, semflg))
@@ -1160,8 +1159,7 @@
 
 	sma = sem_lock(semid);
 	if(sma==NULL) {
-		if(queue.prev != NULL)
-			BUG();
+		BUG_ON(queue.prev != NULL);
 		error = -EIDRM;
 		goto out_free;
 	}
--- linux-2.6.9-rc1-mm1-full-3.4/ipc/shm.c.old	2004-08-28 15:55:28.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/ipc/shm.c	2004-08-28 16:02:56.000000000 +0200
@@ -86,8 +86,7 @@
 static inline void shm_inc (int id) {
 	struct shmid_kernel *shp;
 
-	if(!(shp = shm_lock(id)))
-		BUG();
+	BUG_ON(!(shp = shm_lock(id)));
 	shp->shm_atim = get_seconds();
 	shp->shm_lprid = current->tgid;
 	shp->shm_nattch++;
@@ -137,8 +136,7 @@
 
 	down (&shm_ids.sem);
 	/* remove from the list of attaches of the shm segment */
-	if(!(shp = shm_lock(id)))
-		BUG();
+	BUG_ON(!(shp = shm_lock(id)));
 	shp->shm_lprid = current->tgid;
 	shp->shm_dtim = get_seconds();
 	shp->shm_nattch--;
@@ -261,8 +259,7 @@
 		err = -EEXIST;
 	} else {
 		shp = shm_lock(id);
-		if(shp==NULL)
-			BUG();
+		BUG_ON(shp == NULL);
 		if (shp->shm_segsz < size)
 			err = -EINVAL;
 		else if (ipcperms(&shp->shm_perm, shmflg))
@@ -744,8 +741,7 @@
 	up_write(&current->mm->mmap_sem);
 
 	down (&shm_ids.sem);
-	if(!(shp = shm_lock(shmid)))
-		BUG();
+	BUG_ON(!(shp = shm_lock(shmid)));
 	shp->shm_nattch--;
 	if(shp->shm_nattch == 0 &&
 	   shp->shm_flags & SHM_DEST)
--- linux-2.6.9-rc1-mm1-full-3.4/ipc/util.c.old	2004-08-28 15:55:28.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/ipc/util.c	2004-08-28 16:03:24.000000000 +0200
@@ -216,8 +216,7 @@
 {
 	struct kern_ipc_perm* p;
 	int lid = id % SEQ_MULTIPLIER;
-	if(lid >= ids->size)
-		BUG();
+	BUG_ON(lid >= ids->size);
 
 	/* 
 	 * do not need a rcu_dereference()() here to force ordering
@@ -225,8 +224,7 @@
 	 */	
 	p = ids->entries[lid].p;
 	ids->entries[lid].p = NULL;
-	if(p==NULL)
-		BUG();
+	BUG_ON(p == NULL);
 	ids->in_use--;
 
 	if (lid == ids->max_id) {

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

[Kyle Moffett]

On Aug 28, 2004, at 11:15, Adrian Bunk wrote:
> The patch below does BUG -> BUG_ON conversions in ipc/ .
> --- linux-2.6.9-rc1-mm1-full-3.4/ipc/shm.c.old	2004-08-28 
> 15:55:28.000000000 +0200
> +++ linux-2.6.9-rc1-mm1-full-3.4/ipc/shm.c	2004-08-28 
> 16:02:56.000000000 +0200
> @@ -86,8 +86,7 @@
>  static inline void shm_inc (int id) {
>  	struct shmid_kernel *shp;
>
> -	if(!(shp = shm_lock(id)))
> -		BUG();
> +	BUG_ON(!(shp = shm_lock(id)));

This won't work:

With debugging mode:
if (unlikely(!(shp = shm_lock(id)))) BUG();

Without debugging mode:
do { } while(0)

Anything you put in BUG_ON() must *NOT* have side effects.


>  	shp->shm_atim = get_seconds();
>  	shp->shm_lprid = current->tgid;
>  	shp->shm_nattch++;
> @@ -137,8 +136,7 @@
>
>  	down (&shm_ids.sem);
>  	/* remove from the list of attaches of the shm segment */
> -	if(!(shp = shm_lock(id)))
> -		BUG();
> +	BUG_ON(!(shp = shm_lock(id)));

Same here

>  	shp->shm_lprid = current->tgid;
>  	shp->shm_dtim = get_seconds();
>  	shp->shm_nattch--;
> @@ -744,8 +741,7 @@
>  	up_write(&current->mm->mmap_sem);
>
>  	down (&shm_ids.sem);
> -	if(!(shp = shm_lock(shmid)))
> -		BUG();
> +	BUG_ON(!(shp = shm_lock(shmid)));

And here

>  	shp->shm_nattch--;
>  	if(shp->shm_nattch == 0 &&
>  	   shp->shm_flags & SHM_DEST)

Cheers,
Kyle Moffett

-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a17 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$
L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+
PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r  
!y?(-)
------END GEEK CODE BLOCK------

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

[Adrian Bunk]

On Sat, Aug 28, 2004 at 12:05:10PM -0400, Kyle Moffett wrote:
> On Aug 28, 2004, at 11:15, Adrian Bunk wrote:
> >The patch below does BUG -> BUG_ON conversions in ipc/ .
> >--- linux-2.6.9-rc1-mm1-full-3.4/ipc/shm.c.old	2004-08-28 
> >15:55:28.000000000 +0200
> >+++ linux-2.6.9-rc1-mm1-full-3.4/ipc/shm.c	2004-08-28 
> >16:02:56.000000000 +0200
> >@@ -86,8 +86,7 @@
> > static inline void shm_inc (int id) {
> > 	struct shmid_kernel *shp;
> >
> >-	if(!(shp = shm_lock(id)))
> >-		BUG();
> >+	BUG_ON(!(shp = shm_lock(id)));
> 
> This won't work:
> 
> With debugging mode:
> if (unlikely(!(shp = shm_lock(id)))) BUG();
> 
> Without debugging mode:
> do { } while(0)

Where in 2.6.9-rc1-mm1 is this "Without debugging mode" defined?

> Anything you put in BUG_ON() must *NOT* have side effects.
>...

I'd have said exactly the same some time ago, but I was convinced by 
Arjan that if done correctly, a BUG_ON() with side effects is possible  
with no extra cost even if you want to make BUG configurably do nothing.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

[Michael Buesch]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

Quoting Adrian Bunk <bunk@fs.tum.de>:
> > Anything you put in BUG_ON() must *NOT* have side effects.
> >...
> 
> I'd have said exactly the same some time ago, but I was convinced by 
> Arjan that if done correctly, a BUG_ON() with side effects is possible  
> with no extra cost even if you want to make BUG configurably do nothing.

#if is_debugging_on
# define BUG_ON(x) do { if (unlikely(x)) BUG(); } while (0)
#else
# define BUG_ON(x) do { if (x) { /* nothing */ } } while (0)
#endif

> cu
> Adrian
> 

-- 
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

[Andrew Morton]

Adrian Bunk <bunk@fs.tum.de> wrote:
>
>  > Anything you put in BUG_ON() must *NOT* have side effects.
>  >...
> 
>  I'd have said exactly the same some time ago, but I was convinced by 
>  Arjan that if done correctly, a BUG_ON() with side effects is possible  
>  with no extra cost even if you want to make BUG configurably do nothing.

Nevertheless, I think I'd prefer that we not move code which has
side-effects into BUG_ONs.  For some reason it seems neater that way.

Plus one would like to be able to do

	BUG_ON(strlen(str) > 22);

and have strlen() not be evaluated if BUG_ON is disabled.

A minor distinction, but one which it would be nice to preserve.

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

[Adrian Bunk]

On Sat, Aug 28, 2004 at 12:58:16PM -0700, Andrew Morton wrote:
> Adrian Bunk <bunk@fs.tum.de> wrote:
> >
> >  > Anything you put in BUG_ON() must *NOT* have side effects.
> >  >...
> > 
> >  I'd have said exactly the same some time ago, but I was convinced by 
> >  Arjan that if done correctly, a BUG_ON() with side effects is possible  
> >  with no extra cost even if you want to make BUG configurably do nothing.
> 
> Nevertheless, I think I'd prefer that we not move code which has
> side-effects into BUG_ONs.  For some reason it seems neater that way.
> 
> Plus one would like to be able to do
> 
> 	BUG_ON(strlen(str) > 22);
> 
> and have strlen() not be evaluated if BUG_ON is disabled.
> 
> A minor distinction, but one which it would be nice to preserve.

OTOH, only very few people use the disabled BUG_ON, and a statement 
with a side effect might stay there unnoticed for some time.

If it's a
  #define BUG_ON(x)
and in one place something with a side effect slipped into the BUG_ON, 
you have a classical example for a heisenbug...

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

[Jens Axboe]

On Sat, Aug 28 2004, Andrew Morton wrote:
> Adrian Bunk <bunk@fs.tum.de> wrote:
> >
> >  > Anything you put in BUG_ON() must *NOT* have side effects.
> >  >...
> > 
> >  I'd have said exactly the same some time ago, but I was convinced by 
> >  Arjan that if done correctly, a BUG_ON() with side effects is possible  
> >  with no extra cost even if you want to make BUG configurably do nothing.
> 
> Nevertheless, I think I'd prefer that we not move code which has
> side-effects into BUG_ONs.  For some reason it seems neater that way.
> 
> Plus one would like to be able to do
> 
> 	BUG_ON(strlen(str) > 22);
> 
> and have strlen() not be evaluated if BUG_ON is disabled.
> 
> A minor distinction, but one which it would be nice to preserve.

Precisely, I fully agree (even though BUG_ON() will never be defined
away, if you should not do the check kill it completely).

-- 
Jens Axboe

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

[Matt Mackall]

On Sat, Aug 28, 2004 at 12:58:16PM -0700, Andrew Morton wrote:
> Adrian Bunk <bunk@fs.tum.de> wrote:
> >
> >  > Anything you put in BUG_ON() must *NOT* have side effects.
> >  >...
> > 
> >  I'd have said exactly the same some time ago, but I was convinced by 
> >  Arjan that if done correctly, a BUG_ON() with side effects is possible  
> >  with no extra cost even if you want to make BUG configurably do nothing.
> 
> Nevertheless, I think I'd prefer that we not move code which has
> side-effects into BUG_ONs.  For some reason it seems neater that way.
> 
> Plus one would like to be able to do
> 
> 	BUG_ON(strlen(str) > 22);
> 
> and have strlen() not be evaluated if BUG_ON is disabled.

Well, strlen doesn't actually have a side effect, so it could be
marked pure and then be optimized away.

I'll push configurable BUG_ON support from -tiny to you shortly so
this will stop being theoretical.

-- 
Mathematics is the supreme nostalgia of our time.

[2.6 patch][2/3] kernel/ BUG -> BUG_ON conversions

[Adrian Bunk]

The patch below does BUG -> BUG_ON conversions in kernel/ .

diffstat output:
 kernel/cpu.c      |    8 +++-----
 kernel/exit.c     |   13 +++++--------
 kernel/fork.c     |    3 +--
 kernel/module.c   |    9 +++------
 kernel/power/pm.c |    3 +--
 kernel/printk.c   |    6 ++----
 kernel/ptrace.c   |    6 ++----
 kernel/signal.c   |   27 +++++++++------------------
 kernel/softirq.c  |    6 ++----
 kernel/timer.c    |    3 +--
 10 files changed, 29 insertions(+), 55 deletions(-)


Signed-off-by: Adrian Bunk <bunk@fs.tum.de>

--- linux-2.6.9-rc1-mm1-full-3.4/kernel/cpu.c.old	2004-08-28 16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/cpu.c	2004-08-28 16:06:24.000000000 +0200
@@ -156,9 +156,8 @@
 	kthread_bind(p, smp_processor_id());
 
 	/* CPU is completely dead: tell everyone.  Too late to complain. */
-	if (notifier_call_chain(&cpu_chain, CPU_DEAD, (void *)(long)cpu)
-	    == NOTIFY_BAD)
-		BUG();
+	BUG_ON(notifier_call_chain(&cpu_chain, CPU_DEAD, (void *)(long)cpu)
+	       == NOTIFY_BAD);
 
 	check_for_tasks(cpu);
 
@@ -203,8 +202,7 @@
 	ret = __cpu_up(cpu);
 	if (ret != 0)
 		goto out_notify;
-	if (!cpu_online(cpu))
-		BUG();
+	BUG_ON(!cpu_online(cpu));
 
 	/* Now call notifier in preparation. */
 	notifier_call_chain(&cpu_chain, CPU_ONLINE, hcpu);
--- linux-2.6.9-rc1-mm1-full-3.4/kernel/exit.c.old	2004-08-28 16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/exit.c	2004-08-28 16:08:24.000000000 +0200
@@ -503,7 +503,7 @@
 		down_read(&mm->mmap_sem);
 	}
 	atomic_inc(&mm->mm_count);
-	if (mm != tsk->active_mm) BUG();
+	BUG_ON(mm != tsk->active_mm);
 	/* more a memory barrier than a real lock */
 	task_lock(tsk);
 	tsk->mm = NULL;
@@ -878,11 +877,9 @@
 	const struct list_head *tmp, *head = &link->pidptr->task_list;
 
 #ifdef CONFIG_SMP
-	if (!p->sighand)
-		BUG();
-	if (!spin_is_locked(&p->sighand->siglock) &&
-				!rwlock_is_locked(&tasklist_lock))
-		BUG();
+	BUG_ON(!p->sighand);
+	BUG_ON(!spin_is_locked(&p->sighand->siglock) &&
+				!rwlock_is_locked(&tasklist_lock));
 #endif
 	tmp = link->pid_chain.next;
 	if (tmp == head)
@@ -1350,8 +1347,7 @@
 		if (options & __WNOTHREAD)
 			break;
 		tsk = next_thread(tsk);
-		if (tsk->signal != current->signal)
-			BUG();
+		BUG_ON(tsk->signal != current->signal);
 	} while (tsk != current);
 
 	read_unlock(&tasklist_lock);
--- linux-2.6.9-rc1-mm1-full-3.4/kernel/fork.c.old	2004-08-28 16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/fork.c	2004-08-28 16:08:45.000000000 +0200
@@ -809,8 +809,7 @@
 	struct files_struct *files  = current->files;
 	int rc;
 
-	if(!files)
-		BUG();
+	BUG_ON(!files);
 
 	/* This can race but the race causes us to copy when we don't
 	   need to and drop the copy */
--- linux-2.6.9-rc1-mm1-full-3.4/kernel/module.c.old	2004-08-28 16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/module.c	2004-08-28 16:13:44.000000000 +0200
@@ -655,8 +655,7 @@
 	const unsigned long *crc;
 
 	spin_lock_irqsave(&modlist_lock, flags);
-	if (!__find_symbol(symbol, &owner, &crc, 1))
-		BUG();
+	BUG_ON(!__find_symbol(symbol, &owner, &crc, 1));
 	module_put(owner);
 	spin_unlock_irqrestore(&modlist_lock, flags);
 }
@@ -667,8 +666,7 @@
 	unsigned long flags;
 
 	spin_lock_irqsave(&modlist_lock, flags);
-	if (!kernel_text_address((unsigned long)addr))
-		BUG();
+	BUG_ON(!kernel_text_address((unsigned long)addr));
 
 	module_put(module_text_address((unsigned long)addr));
 	spin_unlock_irqrestore(&modlist_lock, flags);
@@ -905,8 +903,7 @@
 	const unsigned long *crc;
 	struct module *owner;
 
-	if (!__find_symbol("struct_module", &owner, &crc, 1))
-		BUG();
+	BUG_ON(!__find_symbol("struct_module", &owner, &crc, 1));
 	return check_version(sechdrs, versindex, "struct_module", mod,
 			     crc);
 }
--- linux-2.6.9-rc1-mm1-full-3.4/kernel/power/pm.c.old	2004-08-28 16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/power/pm.c	2004-08-28 16:14:19.000000000 +0200
@@ -156,8 +156,7 @@
 	int status = 0;
 	unsigned long prev_state, next_state;
 
-	if (in_interrupt())
-		BUG();
+	BUG_ON(in_interrupt());
 
 	switch (rqst) {
 	case PM_SUSPEND:
--- linux-2.6.9-rc1-mm1-full-3.4/kernel/printk.c.old	2004-08-28 16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/printk.c	2004-08-28 16:14:47.000000000 +0200
@@ -418,8 +418,7 @@
 	unsigned long cur_index, start_print;
 	static int msg_level = -1;
 
-	if (((long)(start - end)) > 0)
-		BUG();
+	BUG_ON(((long)(start - end)) > 0);
 
 	cur_index = start;
 	start_print = start;
@@ -596,8 +595,7 @@
  */
 void acquire_console_sem(void)
 {
-	if (in_interrupt())
-		BUG();
+	BUG_ON(in_interrupt());
 	down(&console_sem);
 	console_locked = 1;
 	console_may_schedule = 1;
--- linux-2.6.9-rc1-mm1-full-3.4/kernel/ptrace.c.old	2004-08-28 16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/ptrace.c	2004-08-28 16:15:19.000000000 +0200
@@ -28,8 +28,7 @@
  */
 void __ptrace_link(task_t *child, task_t *new_parent)
 {
-	if (!list_empty(&child->ptrace_list))
-		BUG();
+	BUG_ON(!list_empty(&child->ptrace_list));
 	if (child->parent == new_parent)
 		return;
 	list_add(&child->ptrace_list, &child->parent->ptrace_children);
@@ -46,8 +45,7 @@
  */
 void __ptrace_unlink(task_t *child)
 {
-	if (!child->ptrace)
-		BUG();
+	BUG_ON(!child->ptrace);
 	child->ptrace = 0;
 	if (list_empty(&child->ptrace_list))
 		return;
--- linux-2.6.9-rc1-mm1-full-3.4/kernel/signal.c.old	2004-08-28 16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/signal.c	2004-08-28 16:17:07.000000000 +0200
@@ -346,10 +346,8 @@
 	struct signal_struct * sig = tsk->signal;
 	struct sighand_struct * sighand = tsk->sighand;
 
-	if (!sig)
-		BUG();
-	if (!atomic_read(&sig->count))
-		BUG();
+	BUG_ON(!sig);
+	BUG_ON(!atomic_read(&sig->count));
 	spin_lock(&sighand->siglock);
 	if (atomic_dec_and_test(&sig->count)) {
 		if (tsk == sig->curr_target)
@@ -816,11 +814,9 @@
 {
 	int ret = 0;
 
-	if (!irqs_disabled())
-		BUG();
+	BUG_ON(!irqs_disabled());
 #ifdef CONFIG_SMP
-	if (!spin_is_locked(&t->sighand->siglock))
-		BUG();
+	BUG_ON(!spin_is_locked(&t->sighand->siglock));
 #endif
 
 	if (((unsigned long)info > 2) && (info->si_code == SI_TIMER))
@@ -1008,8 +1004,7 @@
 	int ret = 0;
 
 #ifdef CONFIG_SMP
-	if (!spin_is_locked(&p->sighand->siglock))
-		BUG();
+	BUG_ON(!spin_is_locked(&p->sighand->siglock));
 #endif
 	handle_stop_signal(sig, p);
 
@@ -1378,8 +1373,7 @@
 		 * If an SI_TIMER entry is already queue just increment
 		 * the overrun count.
 		 */
-		if (q->info.si_code != SI_TIMER)
-			BUG();
+		BUG_ON(q->info.si_code != SI_TIMER);
 		q->info.si_overrun++;
 		goto out;
 	} 
@@ -1425,8 +1419,7 @@
 		 * the overrun count.  Other uses should not try to
 		 * send the signal multiple times.
 		 */
-		if (q->info.si_code != SI_TIMER)
-			BUG();
+		BUG_ON(q->info.si_code != SI_TIMER);
 		q->info.si_overrun++;
 		goto out;
 	} 
@@ -1474,8 +1467,7 @@
 	do {
 		wake_up_interruptible_sync(&tsk->wait_chldexit);
 		tsk = next_thread(tsk);
-		if (tsk->signal != parent->signal)
-			BUG();
+		BUG_ON(tsk->signal != parent->signal);
 	} while (tsk != parent);
 }
 
@@ -1490,8 +1482,7 @@
 	int why, status;
 	struct sighand_struct *psig;
 
-	if (sig == -1)
-		BUG();
+	BUG_ON(sig == -1);
 
 	BUG_ON(!tsk->ptrace &&
 	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
--- linux-2.6.9-rc1-mm1-full-3.4/kernel/softirq.c.old	2004-08-28 16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/softirq.c	2004-08-28 16:18:12.000000000 +0200
@@ -240,8 +240,7 @@
 
 		if (tasklet_trylock(t)) {
 			if (!atomic_read(&t->count)) {
-				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
-					BUG();
+				BUG_ON(!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state));
 				t->func(t->data);
 				tasklet_unlock(t);
 				continue;
@@ -273,8 +272,7 @@
 
 		if (tasklet_trylock(t)) {
 			if (!atomic_read(&t->count)) {
-				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
-					BUG();
+				BUG_ON(!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state));
 				t->func(t->data);
 				tasklet_unlock(t);
 				continue;
--- linux-2.6.9-rc1-mm1-full-3.4/kernel/timer.c.old	2004-08-28 16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/timer.c	2004-08-28 16:18:42.000000000 +0200
@@ -1374,8 +1374,7 @@
 		spin_lock(&new_base->lock);
 	}
 
-	if (old_base->running_timer)
-		BUG();
+	BUG_ON(old_base->running_timer);
 	for (i = 0; i < TVR_SIZE; i++)
 		if (!migrate_timer_list(new_base, old_base->tv1.vec + i))
 			goto unlock_again;

Re: [2.6 patch][2/3] kernel/ BUG -> BUG_ON conversions

[Kyle Moffett]

On Aug 28, 2004, at 11:17, Adrian Bunk wrote:
> The patch below does BUG -> BUG_ON conversions in kernel/ .
>
> --- linux-2.6.9-rc1-mm1-full-3.4/kernel/cpu.c.old	2004-08-28 
> 16:05:17.000000000 +0200
> +++ linux-2.6.9-rc1-mm1-full-3.4/kernel/cpu.c	2004-08-28 
> 16:06:24.000000000 +0200
> @@ -156,9 +156,8 @@
>  	kthread_bind(p, smp_processor_id());
>
>  	/* CPU is completely dead: tell everyone.  Too late to complain. */
> -	if (notifier_call_chain(&cpu_chain, CPU_DEAD, (void *)(long)cpu)
> -	    == NOTIFY_BAD)
> -		BUG();
> +	BUG_ON(notifier_call_chain(&cpu_chain, CPU_DEAD, (void *)(long)cpu)
> +	       == NOTIFY_BAD);

Can't do this, without DEBUG the notifier_call_chain _won't_ be executed

>  	check_for_tasks(cpu);
>
> @@ -203,8 +202,7 @@
>  	ret = __cpu_up(cpu);
>  	if (ret != 0)
>  		goto out_notify;
> -	if (!cpu_online(cpu))
> -		BUG();
> +	BUG_ON(!cpu_online(cpu));

Does cpu_online() have any side effects?

>
>  	/* Now call notifier in preparation. */
>  	notifier_call_chain(&cpu_chain, CPU_ONLINE, hcpu);
> --- linux-2.6.9-rc1-mm1-full-3.4/kernel/exit.c.old	2004-08-28 
> 16:05:17.000000000 +0200
> +++ linux-2.6.9-rc1-mm1-full-3.4/kernel/exit.c	2004-08-28 
> 16:08:24.000000000 +0200
> @@ -503,7 +503,7 @@
>  		down_read(&mm->mmap_sem);
>  	}
>  	atomic_inc(&mm->mm_count);
> -	if (mm != tsk->active_mm) BUG();
> +	BUG_ON(mm != tsk->active_mm);
>  	/* more a memory barrier than a real lock */
>  	task_lock(tsk);
>  	tsk->mm = NULL;
> @@ -878,11 +877,9 @@
>  	const struct list_head *tmp, *head = &link->pidptr->task_list;
>
>  #ifdef CONFIG_SMP
> -	if (!p->sighand)
> -		BUG();
> -	if (!spin_is_locked(&p->sighand->siglock) &&
> -				!rwlock_is_locked(&tasklist_lock))
> -		BUG();
> +	BUG_ON(!p->sighand);
> +	BUG_ON(!spin_is_locked(&p->sighand->siglock) &&
> +				!rwlock_is_locked(&tasklist_lock));

These _should_ be OK, but I'm not sure if spin_is_locked and 
rwlock_is_locked
have any memory barrier guarantees.

>  #endif
>  	tmp = link->pid_chain.next;
>  	if (tmp == head)
> @@ -1350,8 +1347,7 @@
>  		if (options & __WNOTHREAD)
>  			break;
>  		tsk = next_thread(tsk);
> -		if (tsk->signal != current->signal)
> -			BUG();
> +		BUG_ON(tsk->signal != current->signal);
>  	} while (tsk != current);
>
>  	read_unlock(&tasklist_lock);
> --- linux-2.6.9-rc1-mm1-full-3.4/kernel/fork.c.old	2004-08-28 
> 16:05:17.000000000 +0200
> +++ linux-2.6.9-rc1-mm1-full-3.4/kernel/fork.c	2004-08-28 
> 16:08:45.000000000 +0200
> @@ -809,8 +809,7 @@
>  	struct files_struct *files  = current->files;
>  	int rc;
>
> -	if(!files)
> -		BUG();
> +	BUG_ON(!files);
>
>  	/* This can race but the race causes us to copy when we don't
>  	   need to and drop the copy */
> --- linux-2.6.9-rc1-mm1-full-3.4/kernel/module.c.old	2004-08-28 
> 16:05:17.000000000 +0200
> +++ linux-2.6.9-rc1-mm1-full-3.4/kernel/module.c	2004-08-28 
> 16:13:44.000000000 +0200
> @@ -655,8 +655,7 @@
>  	const unsigned long *crc;
>
>  	spin_lock_irqsave(&modlist_lock, flags);
> -	if (!__find_symbol(symbol, &owner, &crc, 1))
> -		BUG();
> +	BUG_ON(!__find_symbol(symbol, &owner, &crc, 1));

Does __find_symbol have side effects?

>  	module_put(owner);
>  	spin_unlock_irqrestore(&modlist_lock, flags);
>  }
> @@ -667,8 +666,7 @@
>  	unsigned long flags;
>
>  	spin_lock_irqsave(&modlist_lock, flags);
> -	if (!kernel_text_address((unsigned long)addr))
> -		BUG();
> +	BUG_ON(!kernel_text_address((unsigned long)addr));

Side effects?

>  	module_put(module_text_address((unsigned long)addr));
>  	spin_unlock_irqrestore(&modlist_lock, flags);
> @@ -905,8 +903,7 @@
>  	const unsigned long *crc;
>  	struct module *owner;
>
> -	if (!__find_symbol("struct_module", &owner, &crc, 1))
> -		BUG();
> +	BUG_ON(!__find_symbol("struct_module", &owner, &crc, 1));
>  	return check_version(sechdrs, versindex, "struct_module", mod,
>  			     crc);
>  }

[Bunch of patch chomped]

Please verify that you don't pull important function calls into 
BUG_ON() statements.

Cheers,
Kyle Moffett

-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a17 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$
L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+
PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r  
!y?(-)
------END GEEK CODE BLOCK------

[2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

[Adrian Bunk]

The patch below does BUG -> BUG_ON conversions in mm/ .

diffstat output:
 mm/bootmem.c    |    6 ++----
 mm/filemap.c    |    6 ++----
 mm/highmem.c    |   15 +++++----------
 mm/memory.c     |   12 ++++--------
 mm/mempool.c    |    5 +++--
 mm/mmap.c       |   12 ++++--------
 mm/mprotect.c   |    3 +--
 mm/msync.c      |    3 +--
 mm/page_alloc.c |    3 +--
 mm/pdflush.c    |    3 +--
 mm/shmem.c      |    3 +--
 mm/slab.c       |   30 ++++++++++--------------------
 mm/swap.c       |   12 ++++--------
 mm/swap_state.c |    6 ++----
 mm/swapfile.c   |    6 ++----
 mm/vmalloc.c    |    3 +--
 mm/vmscan.c     |   18 ++++++------------
 17 files changed, 50 insertions(+), 96 deletions(-)


Signed-off-by: Adrian Bunk <bunk@fs.tum.de>

--- linux-2.6.9-rc1-mm1-full-3.4/mm/bootmem.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/bootmem.c	2004-08-28 16:26:48.000000000 +0200
@@ -125,8 +125,7 @@
 	sidx = start - (bdata->node_boot_start/PAGE_SIZE);
 
 	for (i = sidx; i < eidx; i++) {
-		if (unlikely(!test_and_clear_bit(i, bdata->node_bootmem_map)))
-			BUG();
+		BUG_ON(!test_and_clear_bit(i, bdata->node_bootmem_map));
 	}
 }
 
@@ -246,8 +245,7 @@
 	 * Reserve the area now:
 	 */
 	for (i = start; i < start+areasize; i++)
-		if (unlikely(test_and_set_bit(i, bdata->node_bootmem_map)))
-			BUG();
+		BUG_ON(test_and_set_bit(i, bdata->node_bootmem_map));
 	memset(ret, 0, size);
 	return ret;
 }
--- linux-2.6.9-rc1-mm1-full-3.4/mm/filemap.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/filemap.c	2004-08-28 16:27:22.000000000 +0200
@@ -440,8 +440,7 @@
 void fastcall unlock_page(struct page *page)
 {
 	smp_mb__before_clear_bit();
-	if (!TestClearPageLocked(page))
-		BUG();
+	BUG_ON(!TestClearPageLocked(page));
 	smp_mb__after_clear_bit(); 
 	wake_up_page(page);
 }
@@ -455,8 +454,7 @@
 void end_page_writeback(struct page *page)
 {
 	if (!TestClearPageReclaim(page) || rotate_reclaimable_page(page)) {
-		if (!test_clear_page_writeback(page))
-			BUG();
+		BUG_ON(!test_clear_page_writeback(page));
 		smp_mb__after_clear_bit();
 	}
 	wake_up_page(page);
--- linux-2.6.9-rc1-mm1-full-3.4/mm/highmem.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/highmem.c	2004-08-28 16:28:22.000000000 +0200
@@ -79,8 +79,7 @@
 		pkmap_count[i] = 0;
 
 		/* sanity check */
-		if (pte_none(pkmap_page_table[i]))
-			BUG();
+		BUG_ON(pte_none(pkmap_page_table[i]));
 
 		/*
 		 * Don't need an atomic fetch-and-clear op here;
@@ -161,8 +160,7 @@
 	if (!vaddr)
 		vaddr = map_new_virtual(page);
 	pkmap_count[PKMAP_NR(vaddr)]++;
-	if (pkmap_count[PKMAP_NR(vaddr)] < 2)
-		BUG();
+	BUG_ON(pkmap_count[PKMAP_NR(vaddr)] < 2);
 	spin_unlock(&kmap_lock);
 	return (void*) vaddr;
 }
@@ -177,8 +175,7 @@
 
 	spin_lock(&kmap_lock);
 	vaddr = (unsigned long)page_address(page);
-	if (!vaddr)
-		BUG();
+	BUG_ON(!vaddr);
 	nr = PKMAP_NR(vaddr);
 
 	/*
@@ -223,8 +220,7 @@
 		return 0;
 
 	page_pool = mempool_create(POOL_SIZE, page_pool_alloc, page_pool_free, NULL);
-	if (!page_pool)
-		BUG();
+	BUG_ON(!page_pool);
 	printk("highmem bounce pool size: %d pages\n", POOL_SIZE);
 
 	return 0;
@@ -266,8 +262,7 @@
 		return 0;
 
 	isa_page_pool = mempool_create(ISA_POOL_SIZE, page_pool_alloc, page_pool_free, (void *) __GFP_DMA);
-	if (!isa_page_pool)
-		BUG();
+	BUG_ON(!isa_page_pool);
 
 	printk("isa bounce pool size: %d pages\n", ISA_POOL_SIZE);
 	return 0;
--- linux-2.6.9-rc1-mm1-full-3.4/mm/memory.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/memory.c	2004-08-28 16:29:24.000000000 +0200
@@ -902,8 +902,7 @@
 
 	dir = pgd_offset(mm, address);
 	flush_cache_range(vma, beg, end);
-	if (address >= end)
-		BUG();
+	BUG_ON(address >= end);
 
 	spin_lock(&mm->page_table_lock);
 	do {
@@ -986,8 +985,7 @@
 	phys_addr -= from;
 	dir = pgd_offset(mm, from);
 	flush_cache_range(vma, beg, end);
-	if (from >= end)
-		BUG();
+	BUG_ON(from >= end);
 
 	spin_lock(&mm->page_table_lock);
 	do {
@@ -1766,10 +1764,8 @@
 
 	vma = find_vma(current->mm, addr);
 	write = (vma->vm_flags & VM_WRITE) != 0;
-	if (addr >= end)
-		BUG();
-	if (end > vma->vm_end)
-		BUG();
+	BUG_ON(addr >= end);
+	BUG_ON(end > vma->vm_end);
 	len = (end+PAGE_SIZE-1)/PAGE_SIZE-addr/PAGE_SIZE;
 	ret = get_user_pages(current, current->mm, addr,
 			len, write, 0, NULL, NULL);
--- linux-2.6.9-rc1-mm1-full-3.4/mm/mempool.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/mempool.c	2004-08-28 16:30:34.000000000 +0200
@@ -170,8 +170,9 @@
  */
 void mempool_destroy(mempool_t *pool)
 {
-	if (pool->curr_nr != pool->min_nr)
-		BUG();		/* There were outstanding elements */
+	/* There were outstanding elements */
+	BUG_ON(pool->curr_nr != pool->min_nr);
+
 	free_pool(pool);
 }
 EXPORT_SYMBOL(mempool_destroy);
--- linux-2.6.9-rc1-mm1-full-3.4/mm/mmap.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/mmap.c	2004-08-28 16:31:30.000000000 +0200
@@ -197,8 +197,7 @@
 	i = browse_rb(&mm->mm_rb);
 	if (i != mm->map_count)
 		printk("map_count %d rb %d\n", mm->map_count, i), bug = 1;
-	if (bug)
-		BUG();
+	BUG_ON(bug);
 }
 #else
 #define validate_mm(mm) do { } while (0)
@@ -333,8 +332,7 @@
 	struct rb_node ** rb_link, * rb_parent;
 
 	__vma = find_vma_prepare(mm, vma->vm_start,&prev, &rb_link, &rb_parent);
-	if (__vma && __vma->vm_start < vma->vm_end)
-		BUG();
+	BUG_ON(__vma && __vma->vm_start < vma->vm_end);
 	__vma_link(mm, vma, prev, rb_link, rb_parent);
 	mm->map_count++;
 }
@@ -704,8 +702,7 @@
 	 * (e.g. stash info in next's anon_vma_node when assigning
 	 * an anon_vma, or when trying vma_merge).  Another time.
 	 */
-	if (find_vma_prev(vma->vm_mm, vma->vm_start, &near) != vma)
-		BUG();
+	BUG_ON(find_vma_prev(vma->vm_mm, vma->vm_start, &near) != vma);
 	if (!near)
 		goto none;
 
@@ -1886,8 +1883,7 @@
 		vma->vm_pgoff = vma->vm_start >> PAGE_SHIFT;
 	}
 	__vma = find_vma_prepare(mm,vma->vm_start,&prev,&rb_link,&rb_parent);
-	if (__vma && __vma->vm_start < vma->vm_end)
-		BUG();
+	BUG_ON(__vma && __vma->vm_start < vma->vm_end);
 	vma_link(mm, vma, prev, rb_link, rb_parent);
 }
 
--- linux-2.6.9-rc1-mm1-full-3.4/mm/mprotect.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/mprotect.c	2004-08-28 16:31:49.000000000 +0200
@@ -95,8 +95,7 @@
 
 	dir = pgd_offset(current->mm, start);
 	flush_cache_range(vma, beg, end);
-	if (start >= end)
-		BUG();
+	BUG_ON(start >= end);
 	spin_lock(&current->mm->page_table_lock);
 	do {
 		change_pmd_range(dir, start, end - start, newprot);
--- linux-2.6.9-rc1-mm1-full-3.4/mm/msync.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/msync.c	2004-08-28 16:32:13.000000000 +0200
@@ -113,8 +113,7 @@
 	if (is_vm_hugetlb_page(vma))
 		goto out;
 
-	if (address >= end)
-		BUG();
+	BUG_ON(address >= end);
 	do {
 		error |= filemap_sync_pmd_range(dir, address, end, vma, flags);
 		address = (address + PGDIR_SIZE) & PGDIR_MASK;
--- linux-2.6.9-rc1-mm1-full-3.4/mm/page_alloc.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/page_alloc.c	2004-08-28 16:32:49.000000000 +0200
@@ -188,8 +188,7 @@
 		destroy_compound_page(page, order);
 	mask = (~0UL) << order;
 	page_idx = page - base;
-	if (page_idx & ~mask)
-		BUG();
+	BUG_ON(page_idx & ~mask);
 	index = page_idx >> (1 + order);
 
 	zone->free_pages += 1 << order;
--- linux-2.6.9-rc1-mm1-full-3.4/mm/pdflush.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/pdflush.c	2004-08-28 16:33:22.000000000 +0200
@@ -191,8 +191,7 @@
 	unsigned long flags;
 	int ret = 0;
 
-	if (fn == NULL)
-		BUG();		/* Hard to diagnose if it's deferred */
+	BUG_ON(fn == NULL);	/* Hard to diagnose if it's deferred */
 
 	spin_lock_irqsave(&pdflush_lock, flags);
 	if (list_empty(&pdflush_list)) {
--- linux-2.6.9-rc1-mm1-full-3.4/mm/shmem.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/shmem.c	2004-08-28 16:33:49.000000000 +0200
@@ -1696,8 +1696,7 @@
 		struct page *page;
 
 		page = find_get_page(dentry->d_inode->i_mapping, 0);
-		if (!page)
-			BUG();
+		BUG_ON(!page);
 		kunmap(page);
 		mark_page_accessed(page);
 		page_cache_release(page);
--- linux-2.6.9-rc1-mm1-full-3.4/mm/slab.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/slab.c	2004-08-28 16:36:04.000000000 +0200
@@ -743,8 +743,7 @@
 
 	cache_estimate(0, cache_cache.objsize, cache_line_size(), 0,
 				&left_over, &cache_cache.num);
-	if (!cache_cache.num)
-		BUG();
+	BUG_ON(!cache_cache.num);
 
 	cache_cache.colour = left_over/cache_cache.colour_off;
 	cache_cache.colour_next = 0;
@@ -887,8 +886,7 @@
 	const unsigned long nr_freed = i;
 
 	while (i--) {
-		if (!TestClearPageSlab(page))
-			BUG();
+		BUG_ON(!TestClearPageSlab(page));
 		page++;
 	}
 	sub_page_state(nr_slab, nr_freed);
@@ -1199,8 +1197,7 @@
 	 * Always checks flags, a caller might be expecting debug
 	 * support which isn't available.
 	 */
-	if (flags & ~CREATE_MASK)
-		BUG();
+	BUG_ON(flags & ~CREATE_MASK);
 
 	if (align) {
 		/* combinations of forced alignment and advanced debugging is
@@ -1476,8 +1473,7 @@
 	func(arg);
 	local_irq_enable();
 
-	if (smp_call_function(func, arg, 1, 1))
-		BUG();
+	BUG_ON(smp_call_function(func, arg, 1, 1));
 
 	preempt_enable();
 }
@@ -1529,8 +1525,7 @@
 
 		slabp = list_entry(cachep->lists.slabs_free.prev, struct slab, list);
 #if DEBUG
-		if (slabp->inuse)
-			BUG();
+		BUG_ON(slabp->inuse);
 #endif
 		list_del(&slabp->list);
 
@@ -1554,8 +1549,7 @@
  */
 int kmem_cache_shrink(kmem_cache_t *cachep)
 {
-	if (!cachep || in_interrupt())
-		BUG();
+	BUG_ON(!cachep || in_interrupt());
 
 	return __cache_shrink(cachep);
 }
@@ -1583,8 +1577,7 @@
 {
 	int i;
 
-	if (!cachep || in_interrupt())
-		BUG();
+	BUG_ON(!cachep || in_interrupt());
 
 	/* Don't let CPUs to come and go */
 	lock_cpu_hotplug();
@@ -1703,11 +1696,9 @@
 static void kmem_flagcheck(kmem_cache_t *cachep, int flags)
 {
 	if (flags & SLAB_DMA) {
-		if (!(cachep->gfpflags & GFP_DMA))
-			BUG();
+		BUG_ON(!(cachep->gfpflags & GFP_DMA));
 	} else {
-		if (cachep->gfpflags & GFP_DMA)
-			BUG();
+		BUG_ON(cachep->gfpflags & GFP_DMA);
 	}
 }
 
@@ -1741,8 +1732,7 @@
 	/* Be lazy and only check for valid flags here,
  	 * keeping it out of the critical path in kmem_cache_alloc().
 	 */
-	if (flags & ~(SLAB_DMA|SLAB_LEVEL_MASK|SLAB_NO_GROW))
-		BUG();
+	BUG_ON(flags & ~(SLAB_DMA|SLAB_LEVEL_MASK|SLAB_NO_GROW));
 	if (flags & SLAB_NO_GROW)
 		return 0;
 
--- linux-2.6.9-rc1-mm1-full-3.4/mm/swap.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/swap.c	2004-08-28 16:36:55.000000000 +0200
@@ -90,8 +90,7 @@
 		list_add_tail(&page->lru, &zone->inactive_list);
 		inc_page_state(pgrotated);
 	}
-	if (!test_clear_page_writeback(page))
-		BUG();
+	BUG_ON(!test_clear_page_writeback(page));
 	spin_unlock_irqrestore(&zone->lru_lock, flags);
 	return 0;
 }
@@ -302,8 +301,7 @@
 			zone = pagezone;
 			spin_lock_irq(&zone->lru_lock);
 		}
-		if (TestSetPageLRU(page))
-			BUG();
+		BUG_ON(TestSetPageLRU(page));
 		add_page_to_inactive_list(zone, page);
 	}
 	if (zone)
@@ -329,10 +327,8 @@
 			zone = pagezone;
 			spin_lock_irq(&zone->lru_lock);
 		}
-		if (TestSetPageLRU(page))
-			BUG();
-		if (TestSetPageActive(page))
-			BUG();
+		BUG_ON(TestSetPageLRU(page));
+		BUG_ON(TestSetPageActive(page));
 		add_page_to_active_list(zone, page);
 	}
 	if (zone)
--- linux-2.6.9-rc1-mm1-full-3.4/mm/swap_state.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/swap_state.c	2004-08-28 16:37:23.000000000 +0200
@@ -144,8 +144,7 @@
 	int pf_flags;
 	int err;
 
-	if (!PageLocked(page))
-		BUG();
+	BUG_ON(!PageLocked(page));
 
 	for (;;) {
 		entry = get_swap_page();
@@ -229,8 +228,7 @@
 	if (!err) {
 		remove_from_page_cache(page);
 		page_cache_release(page);	/* pagecache ref */
-		if (!swap_duplicate(entry))
-			BUG();
+		BUG_ON(!swap_duplicate(entry));
 		SetPageDirty(page);
 		INC_CACHE_INFO(add_total);
 	} else if (err == -EEXIST)
--- linux-2.6.9-rc1-mm1-full-3.4/mm/swapfile.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/swapfile.c	2004-08-28 16:37:50.000000000 +0200
@@ -312,8 +312,7 @@
 {
 	int retval = 0;
 
-	if (!PageLocked(page))
-		BUG();
+	BUG_ON(!PageLocked(page));
 	switch (page_count(page)) {
 	case 3:
 		if (!PagePrivate(page))
@@ -506,8 +505,7 @@
 	end = address + size;
 	if (end > PGDIR_SIZE)
 		end = PGDIR_SIZE;
-	if (address >= end)
-		BUG();
+	BUG_ON(address >= end);
 	do {
 		foundaddr = unuse_pmd(vma, pmd, address, end - address,
 						offset, entry, page);
--- linux-2.6.9-rc1-mm1-full-3.4/mm/vmalloc.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/vmalloc.c	2004-08-28 16:38:14.000000000 +0200
@@ -318,8 +318,7 @@
 		int i;
 
 		for (i = 0; i < area->nr_pages; i++) {
-			if (unlikely(!area->pages[i]))
-				BUG();
+			BUG_ON(!area->pages[i]);
 			__free_page(area->pages[i]);
 		}
 
--- linux-2.6.9-rc1-mm1-full-3.4/mm/vmscan.c.old	2004-08-28 16:25:18.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/mm/vmscan.c	2004-08-28 16:39:28.000000000 +0200
@@ -564,8 +564,7 @@
 			prefetchw_prev_lru_page(page,
 						&zone->inactive_list, flags);
 
-			if (!TestClearPageLRU(page))
-				BUG();
+			BUG_ON(!TestClearPageLRU(page));
 			list_del(&page->lru);
 			if (get_page_testone(page)) {
 				/*
@@ -603,8 +602,7 @@
 		 */
 		while (!list_empty(&page_list)) {
 			page = lru_to_page(&page_list);
-			if (TestSetPageLRU(page))
-				BUG();
+			BUG_ON(TestSetPageLRU(page));
 			list_del(&page->lru);
 			if (PageActive(page))
 				add_page_to_active_list(zone, page);
@@ -662,8 +660,7 @@
 	while (pgscanned < nr_pages && !list_empty(&zone->active_list)) {
 		page = lru_to_page(&zone->active_list);
 		prefetchw_prev_lru_page(page, &zone->active_list, flags);
-		if (!TestClearPageLRU(page))
-			BUG();
+		BUG_ON(!TestClearPageLRU(page));
 		list_del(&page->lru);
 		if (get_page_testone(page)) {
 			/*
@@ -735,10 +732,8 @@
 	while (!list_empty(&l_inactive)) {
 		page = lru_to_page(&l_inactive);
 		prefetchw_prev_lru_page(page, &l_inactive, flags);
-		if (TestSetPageLRU(page))
-			BUG();
-		if (!TestClearPageActive(page))
-			BUG();
+		BUG_ON(TestSetPageLRU(page));
+		BUG_ON(!TestClearPageActive(page));
 		list_move(&page->lru, &zone->inactive_list);
 		pgmoved++;
 		if (!pagevec_add(&pvec, page)) {
@@ -764,8 +759,7 @@
 	while (!list_empty(&l_active)) {
 		page = lru_to_page(&l_active);
 		prefetchw_prev_lru_page(page, &l_active, flags);
-		if (TestSetPageLRU(page))
-			BUG();
+		BUG_ON(TestSetPageLRU(page));
 		BUG_ON(!PageActive(page));
 		list_move(&page->lru, &zone->active_list);
 		pgmoved++;

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

[Denis Vlasenko]

On Saturday 28 August 2004 18:18, Adrian Bunk wrote:
> The patch below does BUG -> BUG_ON conversions in mm/ .
>
> diffstat output:
>  mm/bootmem.c    |    6 ++----
>  mm/filemap.c    |    6 ++----
>  mm/highmem.c    |   15 +++++----------
>  mm/memory.c     |   12 ++++--------
>  mm/mempool.c    |    5 +++--
>  mm/mmap.c       |   12 ++++--------
>  mm/mprotect.c   |    3 +--
>  mm/msync.c      |    3 +--
>  mm/page_alloc.c |    3 +--
>  mm/pdflush.c    |    3 +--
>  mm/shmem.c      |    3 +--
>  mm/slab.c       |   30 ++++++++++--------------------
>  mm/swap.c       |   12 ++++--------
>  mm/swap_state.c |    6 ++----
>  mm/swapfile.c   |    6 ++----
>  mm/vmalloc.c    |    3 +--
>  mm/vmscan.c     |   18 ++++++------------
>  17 files changed, 50 insertions(+), 96 deletions(-)
>
>
> Signed-off-by: Adrian Bunk <bunk@fs.tum.de>
>
> --- linux-2.6.9-rc1-mm1-full-3.4/mm/bootmem.c.old	2004-08-28
> 16:25:18.000000000 +0200 +++
> linux-2.6.9-rc1-mm1-full-3.4/mm/bootmem.c	2004-08-28 16:26:48.000000000
> +0200 @@ -125,8 +125,7 @@
>  	sidx = start - (bdata->node_boot_start/PAGE_SIZE);
>
>  	for (i = sidx; i < eidx; i++) {
> -		if (unlikely(!test_and_clear_bit(i, bdata->node_bootmem_map)))
> -			BUG();
> +		BUG_ON(!test_and_clear_bit(i, bdata->node_bootmem_map));
>  	}
>  }
>
> @@ -246,8 +245,7 @@
>  	 * Reserve the area now:
>  	 */
>  	for (i = start; i < start+areasize; i++)
> -		if (unlikely(test_and_set_bit(i, bdata->node_bootmem_map)))
> -			BUG();
> +		BUG_ON(test_and_set_bit(i, bdata->node_bootmem_map));

BUG_ON is like assert(). It may be #defined to nothing.
Do not place expression with side effects into it.
--
vda

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

[Jens Axboe]

On Sat, Aug 28 2004, Denis Vlasenko wrote:
> On Saturday 28 August 2004 18:18, Adrian Bunk wrote:
> > The patch below does BUG -> BUG_ON conversions in mm/ .
> >
> > diffstat output:
> >  mm/bootmem.c    |    6 ++----
> >  mm/filemap.c    |    6 ++----
> >  mm/highmem.c    |   15 +++++----------
> >  mm/memory.c     |   12 ++++--------
> >  mm/mempool.c    |    5 +++--
> >  mm/mmap.c       |   12 ++++--------
> >  mm/mprotect.c   |    3 +--
> >  mm/msync.c      |    3 +--
> >  mm/page_alloc.c |    3 +--
> >  mm/pdflush.c    |    3 +--
> >  mm/shmem.c      |    3 +--
> >  mm/slab.c       |   30 ++++++++++--------------------
> >  mm/swap.c       |   12 ++++--------
> >  mm/swap_state.c |    6 ++----
> >  mm/swapfile.c   |    6 ++----
> >  mm/vmalloc.c    |    3 +--
> >  mm/vmscan.c     |   18 ++++++------------
> >  17 files changed, 50 insertions(+), 96 deletions(-)
> >
> >
> > Signed-off-by: Adrian Bunk <bunk@fs.tum.de>
> >
> > --- linux-2.6.9-rc1-mm1-full-3.4/mm/bootmem.c.old	2004-08-28
> > 16:25:18.000000000 +0200 +++
> > linux-2.6.9-rc1-mm1-full-3.4/mm/bootmem.c	2004-08-28 16:26:48.000000000
> > +0200 @@ -125,8 +125,7 @@
> >  	sidx = start - (bdata->node_boot_start/PAGE_SIZE);
> >
> >  	for (i = sidx; i < eidx; i++) {
> > -		if (unlikely(!test_and_clear_bit(i, bdata->node_bootmem_map)))
> > -			BUG();
> > +		BUG_ON(!test_and_clear_bit(i, bdata->node_bootmem_map));
> >  	}
> >  }
> >
> > @@ -246,8 +245,7 @@
> >  	 * Reserve the area now:
> >  	 */
> >  	for (i = start; i < start+areasize; i++)
> > -		if (unlikely(test_and_set_bit(i, bdata->node_bootmem_map)))
> > -			BUG();
> > +		BUG_ON(test_and_set_bit(i, bdata->node_bootmem_map));
> 
> BUG_ON is like assert(). It may be #defined to nothing.
> Do not place expression with side effects into it.

I've seen several write this, and I don't agree. I was the one that
introduced BUG_ON, actually, with the original bio patches in 2.5.1-pre.
I never intended it to be a nop, no more than making BUG() a nop would
be stupid. It was just short-hand for adding the unlikely() without the
readability problem.

BUG_ON(1); must always BUG(). That said, it's never wise to put
expressions with side-effects into macros.

-- 
Jens Axboe

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

[Adrian Bunk]

On Sat, Aug 28, 2004 at 10:58:23PM +0200, Jens Axboe wrote:
> 
> BUG_ON(1); must always BUG(). That said, it's never wise to put
> expressions with side-effects into macros.

The intention is, to add an option that lets BUG/BUG_ON/WARN_ON/PAGE_BUG 
do nothing. This option should be hidden under EMBEDDED.

In some environments, this seems to be desirable.

> Jens Axboe

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

[Jens Axboe]

On Sat, Aug 28 2004, Adrian Bunk wrote:
> On Sat, Aug 28, 2004 at 10:58:23PM +0200, Jens Axboe wrote:
> > 
> > BUG_ON(1); must always BUG(). That said, it's never wise to put
> > expressions with side-effects into macros.
> 
> The intention is, to add an option that lets BUG/BUG_ON/WARN_ON/PAGE_BUG 
> do nothing. This option should be hidden under EMBEDDED.
> 
> In some environments, this seems to be desirable.

That only makes sense if you are using BUG incorrectly. A BUG()
condition is something that is non-recoverable, undefining that doesn't
make any sense regardless of the environment.

-- 
Jens Axboe

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

[Oliver Neukum]

Am Sonntag, 29. August 2004 14:03 schrieb Jens Axboe:
> > The intention is, to add an option that lets BUG/BUG_ON/WARN_ON/PAGE_BUG 
> > do nothing. This option should be hidden under EMBEDDED.
> > 
> > In some environments, this seems to be desirable.
> 
> That only makes sense if you are using BUG incorrectly. A BUG()
> condition is something that is non-recoverable, undefining that doesn't
> make any sense regardless of the environment.

Why not? Giving reports about unrecoverable errors is sensible
only if the report can be read. On system this is not the case, you
can just salvage the memory and let it crash.

	Regards
		Oliver

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

[Jens Axboe]

On Sun, Aug 29 2004, Oliver Neukum wrote:
> Am Sonntag, 29. August 2004 14:03 schrieb Jens Axboe:
> > > The intention is, to add an option that lets BUG/BUG_ON/WARN_ON/PAGE_BUG 
> > > do nothing. This option should be hidden under EMBEDDED.
> > > 
> > > In some environments, this seems to be desirable.
> > 
> > That only makes sense if you are using BUG incorrectly. A BUG()
> > condition is something that is non-recoverable, undefining that doesn't
> > make any sense regardless of the environment.
> 
> Why not? Giving reports about unrecoverable errors is sensible
> only if the report can be read. On system this is not the case, you
> can just salvage the memory and let it crash.

"Unrecoverable" can quite easily mean "something really bad has
happened, corruption imminent". So maybe you would want BUG/BUG_ON to
restart the box there, the restart-on-panic should help you there.

You're mail is not making a case for defining BUG/BUG_ON to a nop, which
is what the discussion is about.

-- 
Jens Axboe

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

[Adrian Bunk]

On Sun, Aug 29, 2004 at 03:01:56PM +0200, Jens Axboe wrote:
> On Sun, Aug 29 2004, Oliver Neukum wrote:
> > Am Sonntag, 29. August 2004 14:03 schrieb Jens Axboe:
> > > > The intention is, to add an option that lets BUG/BUG_ON/WARN_ON/PAGE_BUG 
> > > > do nothing. This option should be hidden under EMBEDDED.
> > > > 
> > > > In some environments, this seems to be desirable.
> > > 
> > > That only makes sense if you are using BUG incorrectly. A BUG()
> > > condition is something that is non-recoverable, undefining that doesn't
> > > make any sense regardless of the environment.
> > 
> > Why not? Giving reports about unrecoverable errors is sensible
> > only if the report can be read. On system this is not the case, you
> > can just salvage the memory and let it crash.
> 
> "Unrecoverable" can quite easily mean "something really bad has
> happened, corruption imminent". So maybe you would want BUG/BUG_ON to
> restart the box there, the restart-on-panic should help you there.
>...

The current sh/sh64 implementation doesn't seem to do any of the things 
you expect from BUG:

#define BUG() do { \
        printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__); \
        asm volatile("nop"); \
} while (0)

> Jens Axboe

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

[Jens Axboe]

On Sun, Aug 29 2004, Adrian Bunk wrote:
> On Sun, Aug 29, 2004 at 03:01:56PM +0200, Jens Axboe wrote:
> > On Sun, Aug 29 2004, Oliver Neukum wrote:
> > > Am Sonntag, 29. August 2004 14:03 schrieb Jens Axboe:
> > > > > The intention is, to add an option that lets BUG/BUG_ON/WARN_ON/PAGE_BUG 
> > > > > do nothing. This option should be hidden under EMBEDDED.
> > > > > 
> > > > > In some environments, this seems to be desirable.
> > > > 
> > > > That only makes sense if you are using BUG incorrectly. A BUG()
> > > > condition is something that is non-recoverable, undefining that doesn't
> > > > make any sense regardless of the environment.
> > > 
> > > Why not? Giving reports about unrecoverable errors is sensible
> > > only if the report can be read. On system this is not the case, you
> > > can just salvage the memory and let it crash.
> > 
> > "Unrecoverable" can quite easily mean "something really bad has
> > happened, corruption imminent". So maybe you would want BUG/BUG_ON to
> > restart the box there, the restart-on-panic should help you there.
> >...
> 
> The current sh/sh64 implementation doesn't seem to do any of the things 
> you expect from BUG:
> 
> #define BUG() do { \
>         printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__); \
>         asm volatile("nop"); \
> } while (0)

Well too bad for them, I'm glad I'm not trusting any data to a machine
with that architecture.

-- 
Jens Axboe