[KJ] [Patch 8/9] BUG_ON() Conversion in md/raid6main.c

[KJ] [Patch 8/9] BUG_ON() Conversion in md/raid6main.c

[Eric Sesterhenn / snakebyte]

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

hi,

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

Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>

--- linux-2.6.16-rc1-git4/drivers/md/raid6main.c.orig	2006-01-26 00:30:19.000000000 +0100
+++ linux-2.6.16-rc1-git4/drivers/md/raid6main.c	2006-01-26 00:32:31.000000000 +0100
@@ -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++;



[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [Patch 8/9] BUG_ON() Conversion in md/raid6main.c

[Greg KH]

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

On Thu, Jan 26, 2006 at 01:00:43AM +0100, Eric Sesterhenn / snakebyte wrote:
> 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.

thanks,

greg k-h

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [Patch 8/9] BUG_ON() Conversion in md/raid6main.c

[Greg KH]

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

On Thu, Jan 26, 2006 at 01:00:43AM +0100, Eric Sesterhenn / snakebyte wrote:
> hi,
> 
> this changes if() BUG(); constructs to BUG_ON() which is
> cleaner and can better optimized away
> 
> Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
> 
> --- linux-2.6.16-rc1-git4/drivers/md/raid6main.c.orig	2006-01-26 00:30:19.000000000 +0100
> +++ linux-2.6.16-rc1-git4/drivers/md/raid6main.c	2006-01-26 00:32:31.000000000 +0100
> @@ -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

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [Patch 8/9] BUG_ON() Conversion in md/raid6main.c

[Eric Sesterhenn / snakebyte]

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

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.

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


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


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [Patch 8/9] BUG_ON() Conversion in md/raid6main.c

[Greg KH]

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

On Thu, Jan 26, 2006 at 02:25:07AM +0100, Eric Sesterhenn / snakebyte wrote:
> 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.
> 
> 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?

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

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

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [Patch 8/9] BUG_ON() Conversion in md/raid6main.c

[Eric Sesterhenn / snakebyte]

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

hi,

On Wed, 2006-01-25 at 18:01 -0800, Greg KH wrote:
> >  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.

> 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



[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [Patch 8/9] BUG_ON() Conversion in md/raid6main.c

[Adrian Bunk]

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

On Wed, Jan 25, 2006 at 04:34:45PM -0800, Greg KH wrote:
> On Thu, Jan 26, 2006 at 01:00:43AM +0100, Eric Sesterhenn / snakebyte wrote:
> > 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();


> thanks,
> 
> greg k-h

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


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [Patch 8/9] BUG_ON() Conversion in md/raid6main.c

[Adrian Bunk]

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

On Wed, Jan 25, 2006 at 04:35:14PM -0800, Greg KH wrote:
> On Thu, Jan 26, 2006 at 01:00:43AM +0100, Eric Sesterhenn / snakebyte wrote:
> > hi,
> > 
> > this changes if() BUG(); constructs to BUG_ON() which is
> > cleaner and can better optimized away
> > 
> > Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
> > 
> > --- linux-2.6.16-rc1-git4/drivers/md/raid6main.c.orig	2006-01-26 00:30:19.000000000 +0100
> > +++ linux-2.6.16-rc1-git4/drivers/md/raid6main.c	2006-01-26 00:32:31.000000000 +0100
> > @@ -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)


> thanks,
> 
> greg k-h

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


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors