Skip to contentSkip to conversation

BUG_ON() Conversion in md/raid6main.c

PATCH In review Eric Sesterhenn / snakebyte 3 participants 8 messages Jan 26, 2006

Subsystems (2)
Kernel janitors MD RAID
lore
Share & tools
lore

Eric Sesterhenn / snakebyte

author
#0 lore raw

PATCH 8/9 BUG_ON() Conversion in md/raid6main.c

hi,

this changes if() BUG(); constructs to BUG_ON() which is cleaner and can better optimized away

Signed-off-by Eric Sesterhenn <[email protected]>
drivers/md/raid6main.c renamed from drivers/md/raid6main.c.orig +10 -19
@@ -91,10 +91,8 @@ static void print_raid6_conf (raid6_conf
 static void __release_stripe(raid6_conf_t *conf, struct stripe_head *sh)
 {
 	if (atomic_dec_and_test(&sh->count)) {
-		if (!list_empty(&sh->lru))
-			BUG();
-		if (atomic_read(&conf->active_stripes)==0)
-			BUG();
+		BUG_ON(!list_empty(&sh->lru));
+		BUG_ON(atomic_read(&conf->active_stripes)==0);
 		if (test_bit(STRIPE_HANDLE, &sh->state)) {
 			if (test_bit(STRIPE_DELAYED, &sh->state))
 				list_add_tail(&sh->lru, &conf->delayed_list);

@@ -202,10 +200,8 @@ static void init_stripe(struct stripe_he
 	raid6_conf_t *conf = sh->raid_conf;
 	int disks = conf->raid_disks, i;
 
-	if (atomic_read(&sh->count) != 0)
-		BUG();
-	if (test_bit(STRIPE_HANDLE, &sh->state))
-		BUG();
+	BUG_ON(atomic_read(&sh->count) != 0);
+	BUG_ON(test_bit(STRIPE_HANDLE, &sh->state));
 
 	CHECK_DEVLOCK();
 	PRINTK("init_stripe called, stripe %llu\n",

@@ -283,13 +279,11 @@ static struct stripe_head *get_active_st
 				init_stripe(sh, sector, pd_idx);
 		} else {
 			if (atomic_read(&sh->count)) {
-				if (!list_empty(&sh->lru))
-					BUG();
+				BUG_ON(!list_empty(&sh->lru));
 			} else {
 				if (!test_bit(STRIPE_HANDLE, &sh->state))
 					atomic_inc(&conf->active_stripes);
-				if (list_empty(&sh->lru))
-					BUG();
+				BUG_ON(list_empty(&sh->lru));
 				list_del_init(&sh->lru);
 			}
 		}

@@ -348,8 +342,7 @@ static void shrink_stripes(raid6_conf_t 
 		spin_unlock_irq(&conf->device_lock);
 		if (!sh)
 			break;
-		if (atomic_read(&sh->count))
-			BUG();
+		BUG_ON(atomic_read(&sh->count));
 		shrink_buffers(sh, conf->raid_disks);
 		kmem_cache_free(conf->slab_cache, sh);
 		atomic_dec(&conf->active_stripes);

@@ -767,7 +760,7 @@ static void compute_parity(struct stripe
 				if (test_and_clear_bit(R5_Overlap, &sh->dev[i].flags))
 					wake_up(&conf->wait_for_overlap);
 
-				if (sh->dev[i].written) BUG();
+				BUG_ON(sh->dev[i].written);
 				sh->dev[i].written = chosen;
 			}
 		break;

@@ -957,8 +950,7 @@ static int add_stripe_bio(struct stripe_
 	if (*bip && (*bip)->bi_sector < bi->bi_sector + ((bi->bi_size)>>9))
 		goto overlap;
 
-	if (*bip && bi->bi_next && (*bip) != bi->bi_next)
-		BUG();
+	BUG_ON(*bip && bi->bi_next && (*bip) != bi->bi_next);
 	if (*bip)
 		bi->bi_next = *bip;
 	*bip = bi;

@@ -1893,8 +1885,7 @@ static void raid6d (mddev_t *mddev)
 
 		list_del_init(first);
 		atomic_inc(&sh->count);
-		if (atomic_read(&sh->count)!= 1)
-			BUG();
+		BUG_ON(atomic_read(&sh->count)!= 1);
 		spin_unlock_irq(&conf->device_lock);
 
 		handled++;

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#2 lore raw
In reply to Eric Sesterhenn / snakebyte
Quoted 10 lines · Eric Sesterhenn / snakebyte

hi,

this changes if() BUG(); constructs to BUG_ON() which is cleaner and can better optimized away

Signed-off-by Eric Sesterhenn <[email protected]>

drivers/md/raid6main.c static void print_raid6_conf (raid6_conf · line 95

Quoted 9 lines
drivers/md/raid6main.c renamed from drivers/md/raid6main.c.orig +2 -4
@@ -91,10 +91,8 @@ static void print_raid6_conf (raid6_conf
 static void __release_stripe(raid6_conf_t *conf, struct stripe_head *sh)
 {
 	if (atomic_dec_and_test(&sh->count)) {
-		if (!list_empty(&sh->lru))
-			BUG();
-		if (atomic_read(&conf->active_stripes)==0)
-			BUG();
+		BUG_ON(!list_empty(&sh->lru));
+		BUG_ON(atomic_read(&conf->active_stripes)==0);

Also, no, do not do this!
If BUG_ON() is turned off, then the logic changes here.

thanks,

greg k-h

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

author
#3 lore raw
In reply to Eric Sesterhenn / snakebyte

hi,

Quoted 5 lines
Quoted 3 lines

this changes if() BUG(); constructs to BUG_ON() which is cleaner and can better optimized away

gcc optimizes them away if BUG() is not defined, so this is not a true statement.

if BUG() is defined to nothing it still leaves the if, which must be optimized away by the compiler, not by the preprocessor

Quoted 9 lines

drivers/md/raid6main.c static void print_raid6_conf (raid6_conf · line 95

Quoted 6 lines
drivers/md/raid6main.c renamed from drivers/md/raid6main.c.orig +2 -4
-		if (!list_empty(&sh->lru))
-			BUG();
-		if (atomic_read(&conf->active_stripes)==0)
-			BUG();
+		BUG_ON(!list_empty(&sh->lru));
+		BUG_ON(atomic_read(&conf->active_stripes)==0);

Also, no, do not do this!
If BUG_ON() is turned off, then the logic changes here.

list_empty() just returns wheter the list is empty or not, so there is no logic change. and atomic_read() as far as i can see just reads the conf->active_stripes->counter value, so there is nothing changed somewhere and the logic still stays the same. Or did i miss something here?

Thanks, Eric Sesterhenn

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#4 lore raw
In reply to Eric Sesterhenn / snakebyte
Quoted 10 lines · Eric Sesterhenn / snakebyte

hi,

Quoted 5 lines
Quoted 3 lines

this changes if() BUG(); constructs to BUG_ON() which is cleaner and can better optimized away

gcc optimizes them away if BUG() is not defined, so this is not a true statement.

if BUG() is defined to nothing it still leaves the if, which must be optimized away by the compiler, not by the preprocessor

"gcc" is the compiler, not the preprocessor, like I stated :)

And gcc does this for you, have you looked?

Quoted 10 lines
Quoted 9 lines

drivers/md/raid6main.c static void print_raid6_conf (raid6_conf · line 95

Quoted 6 lines
drivers/md/raid6main.c renamed from drivers/md/raid6main.c.orig +2 -4
-		if (!list_empty(&sh->lru))
-			BUG();
-		if (atomic_read(&conf->active_stripes)==0)
-			BUG();
+		BUG_ON(!list_empty(&sh->lru));
+		BUG_ON(atomic_read(&conf->active_stripes)==0);

Also, no, do not do this!
If BUG_ON() is turned off, then the logic changes here.

Quoted 4 lines

list_empty() just returns wheter the list is empty or not, so there is no logic change. and atomic_read() as far as i can see just reads the conf->active_stripes->counter value, so there is nothing changed somewhere and the logic still stays the same. Or did i miss something here?

Sorry, I was thinking that the atomic_read() was doing something there.

Anyway, these cleanups don't really change anything, at the code or assembly level, so I don't see the need for them...

thanks,

greg k-h

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

author
#5 lore raw
In reply to Eric Sesterhenn / snakebyte

hi,

Quoted 6 lines · Greg KH
Quoted 2 lines

if BUG() is defined to nothing it still leaves the if, which must be optimized away by the compiler, not by the preprocessor

"gcc" is the compiler, not the preprocessor, like I stated :)

And gcc does this for you, have you looked?

Sure the end result is the same in both cases. But in one case the preprocessor does the work, by replacing BUG_ON() with nothing, and in the other case the compiler has to notice there is an empty if statement.

Quoted 2 lines

Anyway, these cleanups don't really change anything, at the code or assembly level, so I don't see the need for them...

I think it is much cleaner to read ( takes less screen space ), but i guess there was a discussion like this already somewhen, at least the janitor.kernelnewbies.org/TODO states:

"make sure BUG() is used correctly (i.e. if(function()) BUG(); is evil)
i.e. even when no-op-ing BUG we still have an if (See also: BUG_ON)"

In addition to this the BUG_ON define in asm-generic/bug.h also includes an unlikely() which might generate better code.

Greetings, Eric

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#6 lore raw
In reply to Eric Sesterhenn / snakebyte
Quoted 8 lines · Greg KH
Quoted 4 lines · Eric Sesterhenn / snakebyte

hi,

this changes if() BUG(); constructs to BUG_ON() which is cleaner and can better optimized away

gcc optimizes them away if BUG() is not defined, so this is not a true statement.

This is true due to the unlikely() in the BUG_ON() definition.

I don't know whether this makes a measurable difference, but besides this I consider it a coding style issue since

  BUG_ON(foo);

is IMHO better readable than

  if (foo)
     BUG();
Quoted 3 lines

thanks,

greg k-h

cu
Adrian

Signature · 4 lines
       "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

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#7 lore raw
In reply to Eric Sesterhenn / snakebyte
Quoted 8 lines · Greg KH
Quoted 7 lines · Eric Sesterhenn / snakebyte

hi,

this changes if() BUG(); constructs to BUG_ON() which is cleaner and can better optimized away

Signed-off-by Eric Sesterhenn <[email protected]>
Quoted 15 lines

drivers/md/raid6main.c static void print_raid6_conf (raid6_conf · line 95

Quoted 12 lines
drivers/md/raid6main.c renamed from drivers/md/raid6main.c.orig +2 -4
@@ -91,10 +91,8 @@ static void print_raid6_conf (raid6_conf
 static void __release_stripe(raid6_conf_t *conf, struct stripe_head *sh)
 {
 	if (atomic_dec_and_test(&sh->count)) {
-		if (!list_empty(&sh->lru))
-			BUG();
-		if (atomic_read(&conf->active_stripes)==0)
-			BUG();
+		BUG_ON(!list_empty(&sh->lru));
+		BUG_ON(atomic_read(&conf->active_stripes)==0);

Also, no, do not do this!
If BUG_ON() is turned off, then the logic changes here.

That's wrong, the CONFIG_BUG=n case in include/asm-generic/bug.h is:

  #define BUG_ON(condition) do { if (condition) ; } while(0)
Quoted 3 lines

thanks,

greg k-h

cu
Adrian

Signature · 4 lines
       "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

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors