[PATCH] md: do not use ++ in rcu_dereference() argument

[PATCH] md: do not use ++ in rcu_dereference() argument

[Kulikov Vasiliy]

From: Vasiliy Kulikov <segooon@gmail.com>

rcu_dereference() is macro, so it might use its argument twice.
Argument must not has side effects.

It was found by compiler warning:
drivers/md/raid1.c: In function ‘read_balance’:
drivers/md/raid1.c:445: warning: operation on ‘new_disk’ may be undefined

Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
 drivers/md/raid1.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index ad83a4d..12194df 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -442,7 +442,7 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
 		     r1_bio->bios[new_disk] == IO_BLOCKED ||
 		     !rdev || !test_bit(In_sync, &rdev->flags)
 			     || test_bit(WriteMostly, &rdev->flags);
-		     rdev = rcu_dereference(conf->mirrors[++new_disk].rdev)) {
+		     rdev = rcu_dereference(conf->mirrors[new_disk].rdev)) {
 
 			if (rdev && test_bit(In_sync, &rdev->flags) &&
 				r1_bio->bios[new_disk] != IO_BLOCKED)
@@ -452,6 +452,7 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
 				new_disk = wonly_disk;
 				break;
 			}
+			new_disk++;
 		}
 		goto rb_out;
 	}
-- 
1.7.0.4

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Sam Ravnborg]

On Sun, Sep 05, 2010 at 10:32:18PM +0400, Kulikov Vasiliy wrote:
> From: Vasiliy Kulikov <segooon@gmail.com>
> 
> rcu_dereference() is macro, so it might use its argument twice.
> Argument must not has side effects.
> 
> It was found by compiler warning:
> drivers/md/raid1.c: In function ‘read_balance’:
> drivers/md/raid1.c:445: warning: operation on ‘new_disk’ may be undefined
> 
> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> ---
>  drivers/md/raid1.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index ad83a4d..12194df 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -442,7 +442,7 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
>  		     r1_bio->bios[new_disk] == IO_BLOCKED ||
>  		     !rdev || !test_bit(In_sync, &rdev->flags)
>  			     || test_bit(WriteMostly, &rdev->flags);
> -		     rdev = rcu_dereference(conf->mirrors[++new_disk].rdev)) {
> +		     rdev = rcu_dereference(conf->mirrors[new_disk].rdev)) {
>  
>  			if (rdev && test_bit(In_sync, &rdev->flags) &&
>  				r1_bio->bios[new_disk] != IO_BLOCKED)
> @@ -452,6 +452,7 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
>  				new_disk = wonly_disk;
>  				break;
>  			}
> +			new_disk++;
>  		}
>  		goto rb_out;

This change looks wrong.
In the original implementation new_disk is incremented and
then we do the array lookup.
With your implementation it looks like we increment it after
the array lookup.

	Sam
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Kulikov Vasiliy]

On Sun, Sep 05, 2010 at 21:01 +0200, Sam Ravnborg wrote:
> On Sun, Sep 05, 2010 at 10:32:18PM +0400, Kulikov Vasiliy wrote:
> > From: Vasiliy Kulikov <segooon@gmail.com>
> > 
> > rcu_dereference() is macro, so it might use its argument twice.
> > Argument must not has side effects.
> > 
> > It was found by compiler warning:
> > drivers/md/raid1.c: In function ‘read_balance’:
> > drivers/md/raid1.c:445: warning: operation on ‘new_disk’ may be undefined
> 
> This change looks wrong.
> In the original implementation new_disk is incremented and
> then we do the array lookup.
> With your implementation it looks like we increment it after
> the array lookup.

No, the original code increments new_disk and then dereferences mirrors.

The full code:

		for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
		     r1_bio->bios[new_disk] == IO_BLOCKED ||
		     !rdev || !test_bit(In_sync, &rdev->flags)
			     || test_bit(WriteMostly, &rdev->flags);
		     rdev = rcu_dereference(conf->mirrors[++new_disk].rdev)) {

			if (rdev && test_bit(In_sync, &rdev->flags) &&
				r1_bio->bios[new_disk] != IO_BLOCKED)
				wonly_disk = new_disk;

			if (new_disk == conf->raid_disks - 1) {
				new_disk = wonly_disk;
				break;
			}
		}

    so,

    for (a; b; c = f(++g)) {
       ...
    } 

    ==

    a;
    while (b) {
       ...
       l_continue:
       c = f(++g);
    }

    ==

    a;
    while (b) {
       ...
       l_continue:
       g++;
       c = f(g);
    }

    ==

    for (a; b; c = f(g)) {
       ...
       g++;
    } 

Or you mean smth more?

-- 
Vasiliy
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Sam Ravnborg]

On Sun, Sep 05, 2010 at 11:23:35PM +0400, Kulikov Vasiliy wrote:
> On Sun, Sep 05, 2010 at 21:01 +0200, Sam Ravnborg wrote:
> > On Sun, Sep 05, 2010 at 10:32:18PM +0400, Kulikov Vasiliy wrote:
> > > From: Vasiliy Kulikov <segooon@gmail.com>
> > > 
> > > rcu_dereference() is macro, so it might use its argument twice.
> > > Argument must not has side effects.
> > > 
> > > It was found by compiler warning:
> > > drivers/md/raid1.c: In function ‘read_balance’:
> > > drivers/md/raid1.c:445: warning: operation on ‘new_disk’ may be undefined
> > 
> > This change looks wrong.
> > In the original implementation new_disk is incremented and
> > then we do the array lookup.
> > With your implementation it looks like we increment it after
> > the array lookup.
> 
> No, the original code increments new_disk and then dereferences mirrors.
> 
> The full code:
> 
> 		for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
> 		     r1_bio->bios[new_disk] == IO_BLOCKED ||
> 		     !rdev || !test_bit(In_sync, &rdev->flags)
> 			     || test_bit(WriteMostly, &rdev->flags);
> 		     rdev = rcu_dereference(conf->mirrors[++new_disk].rdev)) {
> 
> 			if (rdev && test_bit(In_sync, &rdev->flags) &&
> 				r1_bio->bios[new_disk] != IO_BLOCKED)
> 				wonly_disk = new_disk;
> 
> 			if (new_disk == conf->raid_disks - 1) {
> 				new_disk = wonly_disk;
> 				break;
> 			}
> 		}
> 
>     so,
> 
>     for (a; b; c = f(++g)) {
>        ...
>     } 

Thanks - that explains it.
This code really screams for a helper function but thats another matter.

	Sam

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Neil Brown]

On Sun, 5 Sep 2010 22:39:08 +0200
Sam Ravnborg <sam@ravnborg.org> wrote:

> On Sun, Sep 05, 2010 at 11:23:35PM +0400, Kulikov Vasiliy wrote:
> > On Sun, Sep 05, 2010 at 21:01 +0200, Sam Ravnborg wrote:
> > > On Sun, Sep 05, 2010 at 10:32:18PM +0400, Kulikov Vasiliy wrote:
> > > > From: Vasiliy Kulikov <segooon@gmail.com>
> > > > 
> > > > rcu_dereference() is macro, so it might use its argument twice.
> > > > Argument must not has side effects.
> > > > 
> > > > It was found by compiler warning:
> > > > drivers/md/raid1.c: In function ‘read_balance’:
> > > > drivers/md/raid1.c:445: warning: operation on ‘new_disk’ may be undefined
> > > 
> > > This change looks wrong.
> > > In the original implementation new_disk is incremented and
> > > then we do the array lookup.
> > > With your implementation it looks like we increment it after
> > > the array lookup.
> > 
> > No, the original code increments new_disk and then dereferences mirrors.
> > 
> > The full code:
> > 
> > 		for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
> > 		     r1_bio->bios[new_disk] == IO_BLOCKED ||
> > 		     !rdev || !test_bit(In_sync, &rdev->flags)
> > 			     || test_bit(WriteMostly, &rdev->flags);
> > 		     rdev = rcu_dereference(conf->mirrors[++new_disk].rdev)) {
> > 
> > 			if (rdev && test_bit(In_sync, &rdev->flags) &&
> > 				r1_bio->bios[new_disk] != IO_BLOCKED)
> > 				wonly_disk = new_disk;
> > 
> > 			if (new_disk == conf->raid_disks - 1) {
> > 				new_disk = wonly_disk;
> > 				break;
> > 			}
> > 		}
> > 
> >     so,
> > 
> >     for (a; b; c = f(++g)) {
> >        ...
> >     } 
> 
> Thanks - that explains it.
> This code really screams for a helper function but thats another matter.

Not an uncommon complaint about my code as it happens......

I've taken the opportunity to substantially re-write that code.

Comments?

Thanks,
NeilBrown

commit e4062735c8f7233923df5858ed20f1278f3ee669
Author: NeilBrown <neilb@suse.de>
Date:   Mon Sep 6 14:10:08 2010 +1000

    md: tidy up device searches in read_balance.
    
    We have a pre-increment side-effect in the arg to a macro:
      rcu_dereference
    
    This is poor form and triggers a warning.  Rather than just fix that,
    take the opportunity to re-write the code it make it more readable.
    
    Reported-by: Kulikov Vasiliy <segooon@gmail.com>
    Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index ad83a4d..e29e13f 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -420,11 +420,13 @@ static void raid1_end_write_request(struct bio *bio, int error)
 static int read_balance(conf_t *conf, r1bio_t *r1_bio)
 {
 	const sector_t this_sector = r1_bio->sector;
-	int new_disk = conf->last_used, disk = new_disk;
-	int wonly_disk = -1;
+	int new_disk = -1;
+	int start_disk;
+	int i;
 	const int sectors = r1_bio->sectors;
 	sector_t new_distance, current_distance;
 	mdk_rdev_t *rdev;
+	int choose_first;
 
 	rcu_read_lock();
 	/*
@@ -435,54 +437,35 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
  retry:
 	if (conf->mddev->recovery_cp < MaxSector &&
 	    (this_sector + sectors >= conf->next_resync)) {
-		/* Choose the first operational device, for consistancy */
-		new_disk = 0;
-
-		for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
-		     r1_bio->bios[new_disk] == IO_BLOCKED ||
-		     !rdev || !test_bit(In_sync, &rdev->flags)
-			     || test_bit(WriteMostly, &rdev->flags);
-		     rdev = rcu_dereference(conf->mirrors[++new_disk].rdev)) {
-
-			if (rdev && test_bit(In_sync, &rdev->flags) &&
-				r1_bio->bios[new_disk] != IO_BLOCKED)
-				wonly_disk = new_disk;
-
-			if (new_disk == conf->raid_disks - 1) {
-				new_disk = wonly_disk;
-				break;
-			}
-		}
-		goto rb_out;
+		choose_first = 1;
+		start_disk = 0;
+	} else {
+		choose_first = 0;
+		start_disk = conf->last_used;
 	}
 
-
 	/* make sure the disk is operational */
-	for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
-	     r1_bio->bios[new_disk] == IO_BLOCKED ||
-	     !rdev || !test_bit(In_sync, &rdev->flags) ||
-		     test_bit(WriteMostly, &rdev->flags);
-	     rdev = rcu_dereference(conf->mirrors[new_disk].rdev)) {
-
-		if (rdev && test_bit(In_sync, &rdev->flags) &&
-		    r1_bio->bios[new_disk] != IO_BLOCKED)
-			wonly_disk = new_disk;
-
-		if (new_disk <= 0)
-			new_disk = conf->raid_disks;
-		new_disk--;
-		if (new_disk == disk) {
-			new_disk = wonly_disk;
-			break;
+	for (i = 0 ; i < conf->raid_disks ; i++) {
+		int disk = start_disk + i;
+		if (disk >= conf->raid_disks)
+			disk -= conf->raid_disks;
+
+		if (r1_bio->bios[disk] == IO_BLOCKED
+		    || !(rdev = rcu_dereference(conf->mirrors[disk].rdev))
+		    || !test_bit(In_sync, &rdev->flags))
+			continue;
+
+		if (test_bit(WriteMostly, &rdev->flags)) {
+			new_disk = disk;
+			continue;
 		}
+		new_disk = disk;
+		break;
 	}
 
-	if (new_disk < 0)
+	if (new_disk < 0 || choose_first)
 		goto rb_out;
 
-	disk = new_disk;
-	/* now disk == new_disk == starting point for search */
-
 	/*
 	 * Don't change to another disk for sequential reads:
 	 */
@@ -491,20 +474,20 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
 	if (this_sector == conf->mirrors[new_disk].head_position)
 		goto rb_out;
 
-	current_distance = abs(this_sector - conf->mirrors[disk].head_position);
+	current_distance = abs(this_sector 
+			       - conf->mirrors[new_disk].head_position);
 
-	/* Find the disk whose head is closest */
+	/* look for a better disk - i.e. head is closer */
+	start_disk = new_disk;
+	for (i = 1; i < conf->raid_disks; i++) {
+		int disk = start_disk + 1;
+		if (disk >= conf->raid_disks)
+			disk -= conf->raid_disks;
 
-	do {
-		if (disk <= 0)
-			disk = conf->raid_disks;
-		disk--;
-
-		rdev = rcu_dereference(conf->mirrors[disk].rdev);
-
-		if (!rdev || r1_bio->bios[disk] == IO_BLOCKED ||
-		    !test_bit(In_sync, &rdev->flags) ||
-		    test_bit(WriteMostly, &rdev->flags))
+		if (r1_bio->bios[disk] == IO_BLOCKED
+		    || !(rdev = rcu_dereference(conf->mirrors[disk].rdev))
+		    || !test_bit(In_sync, &rdev->flags)
+		    || test_bit(WriteMostly, &rdev->flags))
 			continue;
 
 		if (!atomic_read(&rdev->nr_pending)) {
@@ -516,11 +499,9 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
 			current_distance = new_distance;
 			new_disk = disk;
 		}
-	} while (disk != conf->last_used);
+	}
 
  rb_out:
-
-
 	if (new_disk >= 0) {
 		rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
 		if (!rdev)

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[walter harms]

Neil Brown schrieb:
> On Sun, 5 Sep 2010 22:39:08 +0200
> Sam Ravnborg <sam@ravnborg.org> wrote:
> 
>> On Sun, Sep 05, 2010 at 11:23:35PM +0400, Kulikov Vasiliy wrote:
>>> On Sun, Sep 05, 2010 at 21:01 +0200, Sam Ravnborg wrote:
>>>> On Sun, Sep 05, 2010 at 10:32:18PM +0400, Kulikov Vasiliy wrote:
>>>>> From: Vasiliy Kulikov <segooon@gmail.com>
>>>>>
>>>>> rcu_dereference() is macro, so it might use its argument twice.
>>>>> Argument must not has side effects.
>>>>>
>>>>> It was found by compiler warning:
>>>>> drivers/md/raid1.c: In function ‘read_balance’:
>>>>> drivers/md/raid1.c:445: warning: operation on ‘new_disk’ may be undefined
>>>> This change looks wrong.
>>>> In the original implementation new_disk is incremented and
>>>> then we do the array lookup.
>>>> With your implementation it looks like we increment it after
>>>> the array lookup.
>>> No, the original code increments new_disk and then dereferences mirrors.
>>>
>>> The full code:
>>>
>>> 		for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
>>> 		     r1_bio->bios[new_disk] == IO_BLOCKED ||
>>> 		     !rdev || !test_bit(In_sync, &rdev->flags)
>>> 			     || test_bit(WriteMostly, &rdev->flags);
>>> 		     rdev = rcu_dereference(conf->mirrors[++new_disk].rdev)) {
>>>
>>> 			if (rdev && test_bit(In_sync, &rdev->flags) &&
>>> 				r1_bio->bios[new_disk] != IO_BLOCKED)
>>> 				wonly_disk = new_disk;
>>>
>>> 			if (new_disk == conf->raid_disks - 1) {
>>> 				new_disk = wonly_disk;
>>> 				break;
>>> 			}
>>> 		}
>>>
>>>     so,
>>>
>>>     for (a; b; c = f(++g)) {
>>>        ...
>>>     } 
>> Thanks - that explains it.
>> This code really screams for a helper function but thats another matter.
> 
> Not an uncommon complaint about my code as it happens......
> 
> I've taken the opportunity to substantially re-write that code.
> 
> Comments?
> 
> Thanks,
> NeilBrown
> 
> commit e4062735c8f7233923df5858ed20f1278f3ee669
> Author: NeilBrown <neilb@suse.de>
> Date:   Mon Sep 6 14:10:08 2010 +1000
> 
>     md: tidy up device searches in read_balance.
>     
>     We have a pre-increment side-effect in the arg to a macro:
>       rcu_dereference
>     
>     This is poor form and triggers a warning.  Rather than just fix that,
>     take the opportunity to re-write the code it make it more readable.
>     
>     Reported-by: Kulikov Vasiliy <segooon@gmail.com>
>     Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index ad83a4d..e29e13f 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -420,11 +420,13 @@ static void raid1_end_write_request(struct bio *bio, int error)
>  static int read_balance(conf_t *conf, r1bio_t *r1_bio)
>  {
>  	const sector_t this_sector = r1_bio->sector;
> -	int new_disk = conf->last_used, disk = new_disk;
> -	int wonly_disk = -1;
> +	int new_disk = -1;
> +	int start_disk;
> +	int i;
>  	const int sectors = r1_bio->sectors;
>  	sector_t new_distance, current_distance;
>  	mdk_rdev_t *rdev;
> +	int choose_first;
>  
>  	rcu_read_lock();
>  	/*
> @@ -435,54 +437,35 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
>   retry:
>  	if (conf->mddev->recovery_cp < MaxSector &&
>  	    (this_sector + sectors >= conf->next_resync)) {
> -		/* Choose the first operational device, for consistancy */
> -		new_disk = 0;
> -
> -		for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
> -		     r1_bio->bios[new_disk] == IO_BLOCKED ||
> -		     !rdev || !test_bit(In_sync, &rdev->flags)
> -			     || test_bit(WriteMostly, &rdev->flags);
> -		     rdev = rcu_dereference(conf->mirrors[++new_disk].rdev)) {
> -
> -			if (rdev && test_bit(In_sync, &rdev->flags) &&
> -				r1_bio->bios[new_disk] != IO_BLOCKED)
> -				wonly_disk = new_disk;
> -
> -			if (new_disk == conf->raid_disks - 1) {
> -				new_disk = wonly_disk;
> -				break;
> -			}
> -		}
> -		goto rb_out;
> +		choose_first = 1;
> +		start_disk = 0;
> +	} else {
> +		choose_first = 0;
> +		start_disk = conf->last_used;
>  	}
>  


perhaps you can drop the else when you init with
choose_first = 0;
start_disk = conf->last_used;



> -
>  	/* make sure the disk is operational */
> -	for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
> -	     r1_bio->bios[new_disk] == IO_BLOCKED ||
> -	     !rdev || !test_bit(In_sync, &rdev->flags) ||
> -		     test_bit(WriteMostly, &rdev->flags);
> -	     rdev = rcu_dereference(conf->mirrors[new_disk].rdev)) {
> -
> -		if (rdev && test_bit(In_sync, &rdev->flags) &&
> -		    r1_bio->bios[new_disk] != IO_BLOCKED)
> -			wonly_disk = new_disk;
> -
> -		if (new_disk <= 0)
> -			new_disk = conf->raid_disks;
> -		new_disk--;
> -		if (new_disk == disk) {
> -			new_disk = wonly_disk;
> -			break;
> +	for (i = 0 ; i < conf->raid_disks ; i++) {
> +		int disk = start_disk + i;
> +		if (disk >= conf->raid_disks)
> +			disk -= conf->raid_disks;
> +
> +		if (r1_bio->bios[disk] == IO_BLOCKED
> +		    || !(rdev = rcu_dereference(conf->mirrors[disk].rdev))
> +		    || !test_bit(In_sync, &rdev->flags))
> +			continue;

i think it is more readable to use:

  rdev = rcu_dereference(conf->mirrors[disk].rdev);
  if ()



> +		if (test_bit(WriteMostly, &rdev->flags)) {
> +			new_disk = disk;
> +			continue;
>  		}
> +		new_disk = disk;
> +		break;
>  	}

to improve readability:

	new_disk = disk;
	if ( ! test_bit(WriteMostly, &rdev->flags) )
		break;


> -	if (new_disk < 0)
> +	if (new_disk < 0 || choose_first)
>  		goto rb_out;
>  
> -	disk = new_disk;
> -	/* now disk == new_disk == starting point for search */
> -
>  	/*
>  	 * Don't change to another disk for sequential reads:
>  	 */
> @@ -491,20 +474,20 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
>  	if (this_sector == conf->mirrors[new_disk].head_position)
>  		goto rb_out;
>  
> -	current_distance = abs(this_sector - conf->mirrors[disk].head_position);
> +	current_distance = abs(this_sector 
> +			       - conf->mirrors[new_disk].head_position);
>  
> -	/* Find the disk whose head is closest */
> +	/* look for a better disk - i.e. head is closer */
> +	start_disk = new_disk;
> +	for (i = 1; i < conf->raid_disks; i++) {
> +		int disk = start_disk + 1;
> +		if (disk >= conf->raid_disks)
> +			disk -= conf->raid_disks;
>  
> -	do {
> -		if (disk <= 0)
> -			disk = conf->raid_disks;
> -		disk--;
> -
> -		rdev = rcu_dereference(conf->mirrors[disk].rdev);
> -
> -		if (!rdev || r1_bio->bios[disk] == IO_BLOCKED ||
> -		    !test_bit(In_sync, &rdev->flags) ||
> -		    test_bit(WriteMostly, &rdev->flags))

> +		if (r1_bio->bios[disk] == IO_BLOCKED
> +		    || !(rdev = rcu_dereference(conf->mirrors[disk].rdev))
> +		    || !test_bit(In_sync, &rdev->flags)
> +		    || test_bit(WriteMostly, &rdev->flags))
>  			continue;

again i would set
 rdev=rcu_dereference(conf->mirrors[disk].rdev));
before if() like it was in the original the statement is complex
anything that reduces the complexity is good.


>  
>  		if (!atomic_read(&rdev->nr_pending)) {
> @@ -516,11 +499,9 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
>  			current_distance = new_distance;
>  			new_disk = disk;
>  		}
> -	} while (disk != conf->last_used);
> +	}
>  
>   rb_out:
> -
> -
>  	if (new_disk >= 0) {
>  		rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
>  		if (!rdev)
> 


just my 2 cents,
re,
 wh

> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Neil Brown]

On Mon, 06 Sep 2010 09:43:32 +0200
walter harms <wharms@bfs.de> wrote:

> 
> 
> Neil Brown schrieb: 
> > I've taken the opportunity to substantially re-write that code.
> > 
> > Comments?
> > 
> > Thanks,
> > NeilBrown
> > 
> > commit e4062735c8f7233923df5858ed20f1278f3ee669
> > Author: NeilBrown <neilb@suse.de>
> > Date:   Mon Sep 6 14:10:08 2010 +1000
> > 
> >     md: tidy up device searches in read_balance.
> >     
> >     We have a pre-increment side-effect in the arg to a macro:
> >       rcu_dereference
> >     
> >     This is poor form and triggers a warning.  Rather than just fix that,
> >     take the opportunity to re-write the code it make it more readable.
> >     
> >     Reported-by: Kulikov Vasiliy <segooon@gmail.com>
> >     Signed-off-by: NeilBrown <neilb@suse.de>
> > 
> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > index ad83a4d..e29e13f 100644
> > --- a/drivers/md/raid1.c
> > +++ b/drivers/md/raid1.c
> > @@ -420,11 +420,13 @@ static void raid1_end_write_request(struct bio *bio, int error)
> >  static int read_balance(conf_t *conf, r1bio_t *r1_bio)
> >  {
> >  	const sector_t this_sector = r1_bio->sector;
> > -	int new_disk = conf->last_used, disk = new_disk;
> > -	int wonly_disk = -1;
> > +	int new_disk = -1;
> > +	int start_disk;
> > +	int i;
> >  	const int sectors = r1_bio->sectors;
> >  	sector_t new_distance, current_distance;
> >  	mdk_rdev_t *rdev;
> > +	int choose_first;
> >  
> >  	rcu_read_lock();
> >  	/*
> > @@ -435,54 +437,35 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
> >   retry:
> >  	if (conf->mddev->recovery_cp < MaxSector &&
> >  	    (this_sector + sectors >= conf->next_resync)) {
> > -		/* Choose the first operational device, for consistancy */
> > -		new_disk = 0;
> > -
> > -		for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
> > -		     r1_bio->bios[new_disk] == IO_BLOCKED ||
> > -		     !rdev || !test_bit(In_sync, &rdev->flags)
> > -			     || test_bit(WriteMostly, &rdev->flags);
> > -		     rdev = rcu_dereference(conf->mirrors[++new_disk].rdev)) {
> > -
> > -			if (rdev && test_bit(In_sync, &rdev->flags) &&
> > -				r1_bio->bios[new_disk] != IO_BLOCKED)
> > -				wonly_disk = new_disk;
> > -
> > -			if (new_disk == conf->raid_disks - 1) {
> > -				new_disk = wonly_disk;
> > -				break;
> > -			}
> > -		}
> > -		goto rb_out;
> > +		choose_first = 1;
> > +		start_disk = 0;
> > +	} else {
> > +		choose_first = 0;
> > +		start_disk = conf->last_used;
> >  	}
> >  
> 
> 
> perhaps you can drop the else when you init with
> choose_first = 0;
> start_disk = conf->last_used;

Perhaps.  Though given the 'retry' loop it isn't obvious that would do the
right thing.
I think I'll keep this bit as-is.  I think it helps see to two cases more
clearly.



> > +		if (r1_bio->bios[disk] == IO_BLOCKED
> > +		    || !(rdev = rcu_dereference(conf->mirrors[disk].rdev))
> > +		    || !test_bit(In_sync, &rdev->flags))
> > +			continue;
> 
> i think it is more readable to use:
> 
>   rdev = rcu_dereference(conf->mirrors[disk].rdev);
>   if ()
> 

I think assignments inside 'if' statements have their place, but it seems
that this is far from universal.  I've made this change, thanks.

> 
> 
> > +		if (test_bit(WriteMostly, &rdev->flags)) {
> > +			new_disk = disk;
> > +			continue;
> >  		}
> > +		new_disk = disk;
> > +		break;
> >  	}
> 
> to improve readability:
> 
> 	new_disk = disk;
> 	if ( ! test_bit(WriteMostly, &rdev->flags) )
> 		break;

Yes, that is a distinct improvement.  I've made that change too.

Thanks a lot for your review!!

NeilBrown

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Sam Ravnborg]

> Comments?

Looks better but can still use a few improvements.
See below.

	Sam
> 
> Thanks,
> NeilBrown
> 
> commit e4062735c8f7233923df5858ed20f1278f3ee669
> Author: NeilBrown <neilb@suse.de>
> Date:   Mon Sep 6 14:10:08 2010 +1000
> 
>     md: tidy up device searches in read_balance.
>     
>     We have a pre-increment side-effect in the arg to a macro:
>       rcu_dereference
>     
>     This is poor form and triggers a warning.  Rather than just fix that,
>     take the opportunity to re-write the code it make it more readable.
>     
>     Reported-by: Kulikov Vasiliy <segooon@gmail.com>
>     Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index ad83a4d..e29e13f 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -420,11 +420,13 @@ static void raid1_end_write_request(struct bio *bio, int error)
>  static int read_balance(conf_t *conf, r1bio_t *r1_bio)
>  {
>  	const sector_t this_sector = r1_bio->sector;
> -	int new_disk = conf->last_used, disk = new_disk;
> -	int wonly_disk = -1;
> +	int new_disk = -1;
> +	int start_disk;
> +	int i;
>  	const int sectors = r1_bio->sectors;
>  	sector_t new_distance, current_distance;
>  	mdk_rdev_t *rdev;
> +	int choose_first;

To increase readability the general recommendation is:
1) Sort variable definitions with the longest first.
2) Do not assing variables when they are defined, do that on a separate line
   below the variable definitions.
   With one empty line after variable definitions.

>  
>  	rcu_read_lock();
>  	/*
> @@ -435,54 +437,35 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
>   retry:
>  	if (conf->mddev->recovery_cp < MaxSector &&
>  	    (this_sector + sectors >= conf->next_resync)) {
> -		/* Choose the first operational device, for consistancy */
> -		new_disk = 0;
> -
> -		for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
> -		     r1_bio->bios[new_disk] == IO_BLOCKED ||
> -		     !rdev || !test_bit(In_sync, &rdev->flags)
> -			     || test_bit(WriteMostly, &rdev->flags);
> -		     rdev = rcu_dereference(conf->mirrors[++new_disk].rdev)) {
> -
> -			if (rdev && test_bit(In_sync, &rdev->flags) &&
> -				r1_bio->bios[new_disk] != IO_BLOCKED)
> -				wonly_disk = new_disk;
> -
> -			if (new_disk == conf->raid_disks - 1) {
> -				new_disk = wonly_disk;
> -				break;
> -			}
> -		}
> -		goto rb_out;
> +		choose_first = 1;
> +		start_disk = 0;
> +	} else {
> +		choose_first = 0;
> +		start_disk = conf->last_used;
>  	}
>  
> -
>  	/* make sure the disk is operational */
> -	for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
> -	     r1_bio->bios[new_disk] == IO_BLOCKED ||
> -	     !rdev || !test_bit(In_sync, &rdev->flags) ||
> -		     test_bit(WriteMostly, &rdev->flags);
> -	     rdev = rcu_dereference(conf->mirrors[new_disk].rdev)) {
> -
> -		if (rdev && test_bit(In_sync, &rdev->flags) &&
> -		    r1_bio->bios[new_disk] != IO_BLOCKED)
> -			wonly_disk = new_disk;
> -
> -		if (new_disk <= 0)
> -			new_disk = conf->raid_disks;
> -		new_disk--;
> -		if (new_disk == disk) {
> -			new_disk = wonly_disk;
> -			break;
> +	for (i = 0 ; i < conf->raid_disks ; i++) {
> +		int disk = start_disk + i;
> +		if (disk >= conf->raid_disks)
> +			disk -= conf->raid_disks;
1) Please comment on the purpose of the for loop
2) See comments above aboyt variable definitions

> +
> +		if (r1_bio->bios[disk] == IO_BLOCKED
> +		    || !(rdev = rcu_dereference(conf->mirrors[disk].rdev))
> +		    || !test_bit(In_sync, &rdev->flags))
The rather complex expression - which includes a well hidden assignment -
is repeated a few lines later.
Please use a helper function and do not use such hidden assignments.


> +			continue;
> +
> +		if (test_bit(WriteMostly, &rdev->flags)) {
> +			new_disk = disk;
> +			continue;
>  		}
> +		new_disk = disk;
> +		break;
>  	}
>  
> @@ -491,20 +474,20 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
>  	if (this_sector == conf->mirrors[new_disk].head_position)
>  		goto rb_out;
>  
> -	current_distance = abs(this_sector - conf->mirrors[disk].head_position);
> +	current_distance = abs(this_sector 
> +			       - conf->mirrors[new_disk].head_position);
>  
> -	/* Find the disk whose head is closest */
> +	/* look for a better disk - i.e. head is closer */
> +	start_disk = new_disk;
> +	for (i = 1; i < conf->raid_disks; i++) {
> +		int disk = start_disk + 1;
> +		if (disk >= conf->raid_disks)
> +			disk -= conf->raid_disks;
See comments about for loop above.
I also cannot see why we suddenly start with 1 where the other
almost identical for loop starts with 0?
If I wonder then someone else will wonder too => comment please.

>  
> -	do {
> -		if (disk <= 0)
> -			disk = conf->raid_disks;
> -		disk--;
> -
> -		rdev = rcu_dereference(conf->mirrors[disk].rdev);
> -
> -		if (!rdev || r1_bio->bios[disk] == IO_BLOCKED ||
> -		    !test_bit(In_sync, &rdev->flags) ||
> -		    test_bit(WriteMostly, &rdev->flags))
> +		if (r1_bio->bios[disk] == IO_BLOCKED
> +		    || !(rdev = rcu_dereference(conf->mirrors[disk].rdev))
> +		    || !test_bit(In_sync, &rdev->flags)
> +		    || test_bit(WriteMostly, &rdev->flags))
>  			continue;
Here the complex expression is repeated - at least almost identical.

>  
>  		if (!atomic_read(&rdev->nr_pending)) {
> @@ -516,11 +499,9 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
>  			current_distance = new_distance;
>  			new_disk = disk;
>  		}
> -	} while (disk != conf->last_used);
> +	}
>  
>   rb_out:
> -
> -
>  	if (new_disk >= 0) {
>  		rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
>  		if (!rdev)
> 

	Sam

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Neil Brown]

On Mon, 6 Sep 2010 21:21:28 +0200
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Comments?
> 
> Looks better but can still use a few improvements.
> See below.

Thanks for your review and comments....


> 
> 	Sam
> > 
> > Thanks,
> > NeilBrown
> > 
> > commit e4062735c8f7233923df5858ed20f1278f3ee669
> > Author: NeilBrown <neilb@suse.de>
> > Date:   Mon Sep 6 14:10:08 2010 +1000
> > 
> >     md: tidy up device searches in read_balance.
> >     
> >     We have a pre-increment side-effect in the arg to a macro:
> >       rcu_dereference
> >     
> >     This is poor form and triggers a warning.  Rather than just fix that,
> >     take the opportunity to re-write the code it make it more readable.
> >     
> >     Reported-by: Kulikov Vasiliy <segooon@gmail.com>
> >     Signed-off-by: NeilBrown <neilb@suse.de>
> > 
> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > index ad83a4d..e29e13f 100644
> > --- a/drivers/md/raid1.c
> > +++ b/drivers/md/raid1.c
> > @@ -420,11 +420,13 @@ static void raid1_end_write_request(struct bio *bio, int error)
> >  static int read_balance(conf_t *conf, r1bio_t *r1_bio)
> >  {
> >  	const sector_t this_sector = r1_bio->sector;
> > -	int new_disk = conf->last_used, disk = new_disk;
> > -	int wonly_disk = -1;
> > +	int new_disk = -1;
> > +	int start_disk;
> > +	int i;
> >  	const int sectors = r1_bio->sectors;
> >  	sector_t new_distance, current_distance;
> >  	mdk_rdev_t *rdev;
> > +	int choose_first;
> 
> To increase readability the general recommendation is:
> 1) Sort variable definitions with the longest first.
> 2) Do not assing variables when they are defined, do that on a separate line
>    below the variable definitions.
>    With one empty line after variable definitions.
> 

I'm don't really agree with this.  I think declaring related variables
together is much more important that sorting them by length.  I guess it is a
very subjective thing.
And I think initialising at the point of declaration is often a good idea,
though not always.

I've moved 'sectors' up near 'this_sector' but nothing else.




> >  
> >  	rcu_read_lock();
> >  	/*
> > @@ -435,54 +437,35 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
> >   retry:
> >  	if (conf->mddev->recovery_cp < MaxSector &&
> >  	    (this_sector + sectors >= conf->next_resync)) {
> > -		/* Choose the first operational device, for consistancy */
> > -		new_disk = 0;
> > -
> > -		for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
> > -		     r1_bio->bios[new_disk] == IO_BLOCKED ||
> > -		     !rdev || !test_bit(In_sync, &rdev->flags)
> > -			     || test_bit(WriteMostly, &rdev->flags);
> > -		     rdev = rcu_dereference(conf->mirrors[++new_disk].rdev)) {
> > -
> > -			if (rdev && test_bit(In_sync, &rdev->flags) &&
> > -				r1_bio->bios[new_disk] != IO_BLOCKED)
> > -				wonly_disk = new_disk;
> > -
> > -			if (new_disk == conf->raid_disks - 1) {
> > -				new_disk = wonly_disk;
> > -				break;
> > -			}
> > -		}
> > -		goto rb_out;
> > +		choose_first = 1;
> > +		start_disk = 0;
> > +	} else {
> > +		choose_first = 0;
> > +		start_disk = conf->last_used;
> >  	}
> >  
> > -
> >  	/* make sure the disk is operational */
> > -	for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
> > -	     r1_bio->bios[new_disk] == IO_BLOCKED ||
> > -	     !rdev || !test_bit(In_sync, &rdev->flags) ||
> > -		     test_bit(WriteMostly, &rdev->flags);
> > -	     rdev = rcu_dereference(conf->mirrors[new_disk].rdev)) {
> > -
> > -		if (rdev && test_bit(In_sync, &rdev->flags) &&
> > -		    r1_bio->bios[new_disk] != IO_BLOCKED)
> > -			wonly_disk = new_disk;
> > -
> > -		if (new_disk <= 0)
> > -			new_disk = conf->raid_disks;
> > -		new_disk--;
> > -		if (new_disk == disk) {
> > -			new_disk = wonly_disk;
> > -			break;
> > +	for (i = 0 ; i < conf->raid_disks ; i++) {
> > +		int disk = start_disk + i;
> > +		if (disk >= conf->raid_disks)
> > +			disk -= conf->raid_disks;
> 1) Please comment on the purpose of the for loop

That would be the comment "make sure the disk is operational" ??

> 2) See comments above aboyt variable definitions

Still disagree - sorry.

> 
> > +
> > +		if (r1_bio->bios[disk] == IO_BLOCKED
> > +		    || !(rdev = rcu_dereference(conf->mirrors[disk].rdev))
> > +		    || !test_bit(In_sync, &rdev->flags))
> The rather complex expression - which includes a well hidden assignment -
> is repeated a few lines later.
> Please use a helper function and do not use such hidden assignments.

I've moved the assignment out but I don't agree that a helper function is
needed.
Once must balance the total complexity of the function (which should be kept
low) against the cost of having to go look at a separate piece of code to see
what a helper function actually does.

In this case I think that separating this code out would be 'hiding' rather
than 'abstraction'.


> 
> 
> > +			continue;
> > +
> > +		if (test_bit(WriteMostly, &rdev->flags)) {
> > +			new_disk = disk;
> > +			continue;
> >  		}
> > +		new_disk = disk;
> > +		break;
> >  	}
> >  
> > @@ -491,20 +474,20 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
> >  	if (this_sector == conf->mirrors[new_disk].head_position)
> >  		goto rb_out;
> >  
> > -	current_distance = abs(this_sector - conf->mirrors[disk].head_position);
> > +	current_distance = abs(this_sector 
> > +			       - conf->mirrors[new_disk].head_position);
> >  
> > -	/* Find the disk whose head is closest */
> > +	/* look for a better disk - i.e. head is closer */
> > +	start_disk = new_disk;
> > +	for (i = 1; i < conf->raid_disks; i++) {
> > +		int disk = start_disk + 1;
> > +		if (disk >= conf->raid_disks)
> > +			disk -= conf->raid_disks;
> See comments about for loop above.
> I also cannot see why we suddenly start with 1 where the other
> almost identical for loop starts with 0?
> If I wonder then someone else will wonder too => comment please.

Before we were finding a working disk.
Now were a finding a better disk.
First is an absolute statement that needs to consider every device,
second is comparative and only needs to consider every other device (... uhm,
that doesn't sound right - I don't mean every second device, I mean every
device that isn't this one).

Suggestions on a comment that would make that clearer?


> 
> >  
> > -	do {
> > -		if (disk <= 0)
> > -			disk = conf->raid_disks;
> > -		disk--;
> > -
> > -		rdev = rcu_dereference(conf->mirrors[disk].rdev);
> > -
> > -		if (!rdev || r1_bio->bios[disk] == IO_BLOCKED ||
> > -		    !test_bit(In_sync, &rdev->flags) ||
> > -		    test_bit(WriteMostly, &rdev->flags))
> > +		if (r1_bio->bios[disk] == IO_BLOCKED
> > +		    || !(rdev = rcu_dereference(conf->mirrors[disk].rdev))
> > +		    || !test_bit(In_sync, &rdev->flags)
> > +		    || test_bit(WriteMostly, &rdev->flags))
> >  			continue;
> Here the complex expression is repeated - at least almost identical.

The 'almost' is key.
There are two if conditions that are similar but different.  And only two.
Factoring parts out would still leave one of them a little complex and would
split the task of understanding the condition over two separate parts of
program text.
I don't think the benefits of a function out-weigh the costs.

Thanks,
NeilBrown



> 
> >  
> >  		if (!atomic_read(&rdev->nr_pending)) {
> > @@ -516,11 +499,9 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
> >  			current_distance = new_distance;
> >  			new_disk = disk;
> >  		}
> > -	} while (disk != conf->last_used);
> > +	}
> >  
> >   rb_out:
> > -
> > -
> >  	if (new_disk >= 0) {
> >  		rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
> >  		if (!rdev)
> > 
> 
> 	Sam

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Avi Kivity]

  On 09/06/2010 08:29 AM, Neil Brown wrote:
> I've taken the opportunity to substantially re-write that code.
>
>

It's better to have two patches, one a backportable one liner that fixes 
the bug, the other, on top, that cleans up the code but has no sematic 
changes.

This makes it substantially easier to review.  When considering the 
first patch you see the change plainly.  When reviewing the second patch 
you make sure no semantic changes were made at all.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Neil Brown]

On Thu, 16 Sep 2010 14:54:29 +0200
Avi Kivity <avi@redhat.com> wrote:

>   On 09/06/2010 08:29 AM, Neil Brown wrote:
> > I've taken the opportunity to substantially re-write that code.
> >
> >
> 
> It's better to have two patches, one a backportable one liner that fixes 
> the bug, the other, on top, that cleans up the code but has no sematic 
> changes.
> 
> This makes it substantially easier to review.  When considering the 
> first patch you see the change plainly.  When reviewing the second patch 
> you make sure no semantic changes were made at all.
> 

Good advice, I agree.

However the conversation seem have drifted towards viewing the new macro
definition as the bug, and the pre-increment in an argument as a valid thing
to do.
In that case, there is no bug to fix, just a code clean up required.
So I'm currently planning on just submitting that cleanup in the next merge
window, and leaving the rcu guys to 'fix' the macro.

Thanks,
NeilBrown

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Arnd Bergmann]

On Sunday 05 September 2010 20:32:18 Kulikov Vasiliy wrote:
> From: Vasiliy Kulikov <segooon@gmail.com>
> 
> rcu_dereference() is macro, so it might use its argument twice.
> Argument must not has side effects.
> 
> It was found by compiler warning:
> drivers/md/raid1.c: In function ‘read_balance’:
> drivers/md/raid1.c:445: warning: operation on ‘new_disk’ may be undefined

I think the rcu_dereference macro should really not evaluate its argument
twice, and I don't see where it does.
As a general rule, we try to write macros in Linux such that they behave
like functions and don't have surprising side-effects.

Which kernel and gcc version do you see the warning with?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Kulikov Vasiliy]

On Mon, Sep 06, 2010 at 22:10 +0200, Arnd Bergmann wrote:
> On Sunday 05 September 2010 20:32:18 Kulikov Vasiliy wrote:
> > From: Vasiliy Kulikov <segooon@gmail.com>
> > 
> > rcu_dereference() is macro, so it might use its argument twice.
> > Argument must not has side effects.
> > 
> > It was found by compiler warning:
> > drivers/md/raid1.c: In function ‘read_balance’:
> > drivers/md/raid1.c:445: warning: operation on ‘new_disk’ may be undefined
> 
> I think the rcu_dereference macro should really not evaluate its argument
> twice, and I don't see where it does.
> As a general rule, we try to write macros in Linux such that they behave
> like functions and don't have surprising side-effects.
> 
> Which kernel and gcc version do you see the warning with?
> 
> 	Arnd

gcc version 4.4.3 (Ubuntu 4.4.3-4ubuntu5), linux-next.

#define __rcu_dereference_check(p, c, space) \
	({ \
		typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
                                                                   ^
		rcu_lockdep_assert(c); \
		(void) (((typeof (*p) space *)p) == p); \
                                      ^     ^
		smp_read_barrier_depends(); \
		((typeof(*p) __force __kernel *)(_________p1)); \
	})

If I understand this, it is evaluated three times, right?


-- 
Vasiliy
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Arnd Bergmann]

On Tuesday 07 September 2010 21:21:55 Kulikov Vasiliy wrote:
> #define __rcu_dereference_check(p, c, space) \
>         ({ \
>                 typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
>                                                                    ^
>                 rcu_lockdep_assert(c); \
>                 (void) (((typeof (*p) space *)p) == p); \
>                                       ^     ^
>                 smp_read_barrier_depends(); \
>                 ((typeof(*p) __force __kernel *)(_________p1)); \
>         })
> 
> If I understand this, it is evaluated three times, right?
 
Yes, that looks like my own fault, I added that :(

This patch seems to fix it, but I need to think about it some more
to make sure it still does everything we need.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 89414d6..743bc3f 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -313,21 +313,21 @@ extern int rcu_my_thread_group_empty(void);
 #define __rcu_access_pointer(p, space) \
 	({ \
 		typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
-		(void) (((typeof (*p) space *)p) == p); \
+		(void) (((typeof (*p) space *)NULL) == (typeof(p))NULL); \
 		((typeof(*p) __force __kernel *)(_________p1)); \
 	})
 #define __rcu_dereference_check(p, c, space) \
 	({ \
 		typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
 		rcu_lockdep_assert(c); \
-		(void) (((typeof (*p) space *)p) == p); \
+		(void) (((typeof (*p) space *)NULL) == (typeof(p))NULL); \
 		smp_read_barrier_depends(); \
 		((typeof(*p) __force __kernel *)(_________p1)); \
 	})
 #define __rcu_dereference_protected(p, c, space) \
 	({ \
 		rcu_lockdep_assert(c); \
-		(void) (((typeof (*p) space *)p) == p); \
+		(void) (((typeof (*p) space *)NULL) == (typeof(p))NULL); \
 		((typeof(*p) __force __kernel *)(p)); \
 	})
 

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Paul E. McKenney]

On Tue, Sep 07, 2010 at 10:00:58PM +0200, Arnd Bergmann wrote:
> On Tuesday 07 September 2010 21:21:55 Kulikov Vasiliy wrote:
> > #define __rcu_dereference_check(p, c, space) \
> >         ({ \
> >                 typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
> >                                                                    ^
> >                 rcu_lockdep_assert(c); \
> >                 (void) (((typeof (*p) space *)p) == p); \
> >                                       ^     ^
> >                 smp_read_barrier_depends(); \
> >                 ((typeof(*p) __force __kernel *)(_________p1)); \
> >         })
> > 
> > If I understand this, it is evaluated three times, right?
> 
> Yes, that looks like my own fault, I added that :(
> 
> This patch seems to fix it, but I need to think about it some more
> to make sure it still does everything we need.

Let me know when you are satisfied with it, and then I will pick it up.

							Thanx, Paul

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 89414d6..743bc3f 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -313,21 +313,21 @@ extern int rcu_my_thread_group_empty(void);
>  #define __rcu_access_pointer(p, space) \
>  	({ \
>  		typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
> -		(void) (((typeof (*p) space *)p) == p); \
> +		(void) (((typeof (*p) space *)NULL) == (typeof(p))NULL); \
>  		((typeof(*p) __force __kernel *)(_________p1)); \
>  	})
>  #define __rcu_dereference_check(p, c, space) \
>  	({ \
>  		typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
>  		rcu_lockdep_assert(c); \
> -		(void) (((typeof (*p) space *)p) == p); \
> +		(void) (((typeof (*p) space *)NULL) == (typeof(p))NULL); \
>  		smp_read_barrier_depends(); \
>  		((typeof(*p) __force __kernel *)(_________p1)); \
>  	})
>  #define __rcu_dereference_protected(p, c, space) \
>  	({ \
>  		rcu_lockdep_assert(c); \
> -		(void) (((typeof (*p) space *)p) == p); \
> +		(void) (((typeof (*p) space *)NULL) == (typeof(p))NULL); \
>  		((typeof(*p) __force __kernel *)(p)); \
>  	})
> 

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Arnd Bergmann]

On Tuesday 07 September 2010, Paul E. McKenney wrote:
> On Tue, Sep 07, 2010 at 10:00:58PM +0200, Arnd Bergmann wrote:
> > On Tuesday 07 September 2010 21:21:55 Kulikov Vasiliy wrote:
> > > #define __rcu_dereference_check(p, c, space) \
> > >         ({ \
> > >                 typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
> > >                                                                    ^
> > >                 rcu_lockdep_assert(c); \
> > >                 (void) (((typeof (*p) space *)p) == p); \
> > >                                       ^     ^
> > >                 smp_read_barrier_depends(); \
> > >                 ((typeof(*p) __force __kernel *)(_________p1)); \
> > >         })
> > > 
> > > If I understand this, it is evaluated three times, right?
> > 
> > Yes, that looks like my own fault, I added that :(
> > 
> > This patch seems to fix it, but I need to think about it some more
> > to make sure it still does everything we need.
> 
> Let me know when you are satisfied with it, and then I will pick it up.

I guess it would be good to put it in now. I haven't had the time
to try out all cases, but the current code in -next is definitely
broken, so please put the fix in now.

	Arnd

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Paul E. McKenney]

On Thu, Sep 09, 2010 at 05:14:43PM +0200, Arnd Bergmann wrote:
> On Tuesday 07 September 2010, Paul E. McKenney wrote:
> > On Tue, Sep 07, 2010 at 10:00:58PM +0200, Arnd Bergmann wrote:
> > > On Tuesday 07 September 2010 21:21:55 Kulikov Vasiliy wrote:
> > > > #define __rcu_dereference_check(p, c, space) \
> > > >         ({ \
> > > >                 typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
> > > >                                                                    ^
> > > >                 rcu_lockdep_assert(c); \
> > > >                 (void) (((typeof (*p) space *)p) == p); \
> > > >                                       ^     ^
> > > >                 smp_read_barrier_depends(); \
> > > >                 ((typeof(*p) __force __kernel *)(_________p1)); \
> > > >         })
> > > > 
> > > > If I understand this, it is evaluated three times, right?
> > > 
> > > Yes, that looks like my own fault, I added that :(
> > > 
> > > This patch seems to fix it, but I need to think about it some more
> > > to make sure it still does everything we need.
> > 
> > Let me know when you are satisfied with it, and then I will pick it up.
> 
> I guess it would be good to put it in now. I haven't had the time
> to try out all cases, but the current code in -next is definitely
> broken, so please put the fix in now.

Hmmm...  One approach would be have a secondary macro that was:

	#define __rcu_dereference_check_sparse(p, space) \
		(void) (((typeof (*p) space *)p) == p);

when running sparse and:

	#define __rcu_dereference_check_sparse(p, space)

otherwise.

Would that do the trick?

							Thanx, Paul

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Paul E. McKenney]

On Thu, Sep 09, 2010 at 08:46:03PM -0700, Paul E. McKenney wrote:
> On Thu, Sep 09, 2010 at 05:14:43PM +0200, Arnd Bergmann wrote:
> > On Tuesday 07 September 2010, Paul E. McKenney wrote:
> > > On Tue, Sep 07, 2010 at 10:00:58PM +0200, Arnd Bergmann wrote:
> > > > On Tuesday 07 September 2010 21:21:55 Kulikov Vasiliy wrote:
> > > > > #define __rcu_dereference_check(p, c, space) \
> > > > >         ({ \
> > > > >                 typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
> > > > >                                                                    ^
> > > > >                 rcu_lockdep_assert(c); \
> > > > >                 (void) (((typeof (*p) space *)p) == p); \
> > > > >                                       ^     ^
> > > > >                 smp_read_barrier_depends(); \
> > > > >                 ((typeof(*p) __force __kernel *)(_________p1)); \
> > > > >         })
> > > > > 
> > > > > If I understand this, it is evaluated three times, right?
> > > > 
> > > > Yes, that looks like my own fault, I added that :(
> > > > 
> > > > This patch seems to fix it, but I need to think about it some more
> > > > to make sure it still does everything we need.
> > > 
> > > Let me know when you are satisfied with it, and then I will pick it up.
> > 
> > I guess it would be good to put it in now. I haven't had the time
> > to try out all cases, but the current code in -next is definitely
> > broken, so please put the fix in now.
> 
> Hmmm...  One approach would be have a secondary macro that was:
> 
> 	#define __rcu_dereference_check_sparse(p, space) \
> 		(void) (((typeof (*p) space *)p) == p);
> 
> when running sparse and:
> 
> 	#define __rcu_dereference_check_sparse(p, space)
> 
> otherwise.
> 
> Would that do the trick?

Here is a patch that more fully describes what I had in mind.  I have
done light compile/sparse testing, but this should be considered to be
full of bugs.

Thoughts?

							Thanx, Paul

------------------------------------------------------------------------

commit 493a0a1001137f7058da0e01c3d05b0fcb92841d
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Mon Sep 13 17:24:21 2010 -0700

    rcu: only one evaluation of arg in rcu_dereference_check() unless sparse
    
    The current version of the __rcu_access_pointer(), __rcu_dereference_check(),
    and __rcu_dereference_protected() macros evaluate their "p" argument
    three times, not counting typeof()s.  This is bad news if that argument
    contains a side effect.  This commit therefore evaluates this argument
    only once in normal kernel builds.  However, the straightforward approach
    defeats sparse's RCU-pointer checking, so this commit also adds a
    KBUILD_CHECKSRC symbol defined when running a checker.  Therefore, when
    this new KBUILD_CHECKSRC symbol is defined, the additional pair of
    evaluations of the "p" argument are performed in order to permit sparse
    to detect misuse of RCU-protected pointers.
    
    This commit also fixes some sparse complaints in rcutorture.c.
    
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Cc: Arnd Bergmann <arnd@arndb.de>

diff --git a/Makefile b/Makefile
index f3bdff8..1c4984d 100644
--- a/Makefile
+++ b/Makefile
@@ -330,7 +330,7 @@ PERL		= perl
 CHECK		= sparse
 
 CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
-		  -Wbitwise -Wno-return-void $(CF)
+		  -Wbitwise -Wno-return-void -DKBUILD_CHECKSRC $(CF)
 CFLAGS_MODULE   =
 AFLAGS_MODULE   =
 LDFLAGS_MODULE  =
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 682bf4c..9fda1e6 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -309,24 +309,32 @@ extern int rcu_my_thread_group_empty(void);
  * (e.g., __rcu_bh, * __rcu_sched, and __srcu), should this make sense in
  * the future.
  */
+
+#ifdef KBUILD_CHECKSRC
+#define rcu_dereference_sparse(p, space) \
+	((void)(((typeof(*p) space *)p) == p))
+#else /* #ifdef KBUILD_CHECKSRC */
+#define rcu_dereference_sparse(p, space)
+#endif /* #else #ifdef KBUILD_CHECKSRC */
+
 #define __rcu_access_pointer(p, space) \
 	({ \
 		typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
-		(void) (((typeof (*p) space *)p) == p); \
+		rcu_dereference_sparse(p, space); \
 		((typeof(*p) __force __kernel *)(_________p1)); \
 	})
 #define __rcu_dereference_check(p, c, space) \
 	({ \
 		typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
 		rcu_lockdep_assert(c); \
-		(void) (((typeof (*p) space *)p) == p); \
+		rcu_dereference_sparse(p, space); \
 		smp_read_barrier_depends(); \
 		((typeof(*p) __force __kernel *)(_________p1)); \
 	})
 #define __rcu_dereference_protected(p, c, space) \
 	({ \
 		rcu_lockdep_assert(c); \
-		(void) (((typeof (*p) space *)p) == p); \
+		rcu_dereference_sparse(p, space); \
 		((typeof(*p) __force __kernel *)(p)); \
 	})
 
diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
index 439ddab..adb09cb 100644
--- a/kernel/rcutorture.c
+++ b/kernel/rcutorture.c
@@ -131,7 +131,7 @@ struct rcu_torture {
 };
 
 static LIST_HEAD(rcu_torture_freelist);
-static struct rcu_torture *rcu_torture_current;
+static struct rcu_torture __rcu *rcu_torture_current;
 static long rcu_torture_current_version;
 static struct rcu_torture rcu_tortures[10 * RCU_TORTURE_PIPE_LEN];
 static DEFINE_SPINLOCK(rcu_torture_lock);
@@ -171,7 +171,7 @@ int rcutorture_runnable = RCUTORTURE_RUNNABLE_INIT;
 #endif /* #else #ifdef CONFIG_BOOST_RCU */
 
 static unsigned long boost_starttime;	/* jiffies of next boost test start. */
-DEFINE_MUTEX(boost_mutex);		/* protect setting boost_starttime */
+static DEFINE_MUTEX(boost_mutex);	/* protect setting boost_starttime */
 					/*  and boost task create/destroy. */
 
 /* Mediate rmmod and system shutdown.  Concurrent rmmod & shutdown illegal! */
@@ -180,7 +180,8 @@ DEFINE_MUTEX(boost_mutex);		/* protect setting boost_starttime */
 #define FULLSTOP_SHUTDOWN 1	/* System shutdown with rcutorture running. */
 #define FULLSTOP_RMMOD    2	/* Normal rmmod of rcutorture. */
 static int fullstop = FULLSTOP_RMMOD;
-DEFINE_MUTEX(fullstop_mutex);	/* Protect fullstop transitions and spawning */
+static DEFINE_MUTEX(fullstop_mutex);
+				/* Protect fullstop transitions and spawning */
 				/*  of kthreads. */
 
 /*
@@ -873,7 +874,8 @@ rcu_torture_writer(void *arg)
 			continue;
 		rp->rtort_pipe_count = 0;
 		udelay(rcu_random(&rand) & 0x3ff);
-		old_rp = rcu_torture_current;
+		old_rp = rcu_dereference_check(rcu_torture_current,
+					       current == writer_task);
 		rp->rtort_mbtest = 1;
 		rcu_assign_pointer(rcu_torture_current, rp);
 		smp_wmb(); /* Mods to old_rp must follow rcu_assign_pointer() */

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Arnd Bergmann]

On Tuesday 14 September 2010, Paul E. McKenney wrote:
>    The current version of the __rcu_access_pointer(),
> __rcu_dereference_check(), and __rcu_dereference_protected() macros
> evaluate their "p" argument three times, not counting typeof()s.  This is
> bad news if that argument contains a side effect.  This commit therefore
> evaluates this argument only once in normal kernel builds.  However, the
> straightforward approach defeats sparse's RCU-pointer checking, so this
> commit also adds a KBUILD_CHECKSRC symbol defined when running a checker. 
> Therefore, when this new KBUILD_CHECKSRC symbol is defined, the additional
> pair of evaluations of the "p" argument are performed in order to permit
> sparse to detect misuse of RCU-protected pointers.

In general, I don't like the idea much because that means we're passing
semantically different code into sparse and gcc. Of course if my other
patch doesn't work, we might need to do it after all.

> diff --git a/Makefile b/Makefile
> index f3bdff8..1c4984d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -330,7 +330,7 @@ PERL                = perl
>  CHECK          = sparse
>  
>  CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
> -                 -Wbitwise -Wno-return-void $(CF)
> +                 -Wbitwise -Wno-return-void -DKBUILD_CHECKSRC $(CF)
>  CFLAGS_MODULE   =
>  AFLAGS_MODULE   =
>  LDFLAGS_MODULE  =

sparse already define __CHECKER__ itself, no need to define another symbol.

> +#ifdef KBUILD_CHECKSRC
> +#define rcu_dereference_sparse(p, space) \
> +       ((void)(((typeof(*p) space *)p) == p))
> +#else /* #ifdef KBUILD_CHECKSRC */
> +#define rcu_dereference_sparse(p, space)
> +#endif /* #else #ifdef KBUILD_CHECKSRC */

Did you see a problem with my macro?

#define rcu_dereference_sparse(p, space) \
       ((void)(((typeof(*p) space *)NULL) == ((typeof(p))NULL)))

I think this should warn in all the cases we want it to, but have no side-effects.

>  #define __rcu_access_pointer(p, space) \
>         ({ \
>                 typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
> -               (void) (((typeof (*p) space *)p) == p); \
> +               rcu_dereference_sparse(p, space); \
>                 ((typeof(*p) __force __kernel *)(_________p1)); \
>         })
>  #define __rcu_dereference_check(p, c, space) \
>         ({ \
>                 typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
>                 rcu_lockdep_assert(c); \
> -               (void) (((typeof (*p) space *)p) == p); \
> +               rcu_dereference_sparse(p, space); \
>                 smp_read_barrier_depends(); \
>                 ((typeof(*p) __force __kernel *)(_________p1)); \
>         })
>  #define __rcu_dereference_protected(p, c, space) \
>         ({ \
>                 rcu_lockdep_assert(c); \
> -               (void) (((typeof (*p) space *)p) == p); \
> +               rcu_dereference_sparse(p, space); \
>                 ((typeof(*p) __force __kernel *)(p)); \
>         })
>  

This part might be useful in any case, to better document what the cast and
compare does, and to prevent the three users from diverging.

>diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
>index 439ddab..adb09cb 100644
>--- a/kernel/rcutorture.c
>+++ b/kernel/rcutorture.c

This didn't seem to belong here.

	Arnd

Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Paul E. McKenney]

On Wed, Sep 15, 2010 at 02:28:32PM +0200, Arnd Bergmann wrote:
> On Tuesday 14 September 2010, Paul E. McKenney wrote:
> >    The current version of the __rcu_access_pointer(),
> > __rcu_dereference_check(), and __rcu_dereference_protected() macros
> > evaluate their "p" argument three times, not counting typeof()s.  This is
> > bad news if that argument contains a side effect.  This commit therefore
> > evaluates this argument only once in normal kernel builds.  However, the
> > straightforward approach defeats sparse's RCU-pointer checking, so this
> > commit also adds a KBUILD_CHECKSRC symbol defined when running a checker. 
> > Therefore, when this new KBUILD_CHECKSRC symbol is defined, the additional
> > pair of evaluations of the "p" argument are performed in order to permit
> > sparse to detect misuse of RCU-protected pointers.
> 
> In general, I don't like the idea much because that means we're passing
> semantically different code into sparse and gcc. Of course if my other
> patch doesn't work, we might need to do it after all.

Agreed in principle, but please see below.

> > diff --git a/Makefile b/Makefile
> > index f3bdff8..1c4984d 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -330,7 +330,7 @@ PERL                = perl
> >  CHECK          = sparse
> >  
> >  CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
> > -                 -Wbitwise -Wno-return-void $(CF)
> > +                 -Wbitwise -Wno-return-void -DKBUILD_CHECKSRC $(CF)
> >  CFLAGS_MODULE   =
> >  AFLAGS_MODULE   =
> >  LDFLAGS_MODULE  =
> 
> sparse already define __CHECKER__ itself, no need to define another symbol.

Good point, will fix if we are in fact sticking with this solution.

> > +#ifdef KBUILD_CHECKSRC
> > +#define rcu_dereference_sparse(p, space) \
> > +       ((void)(((typeof(*p) space *)p) == p))
> > +#else /* #ifdef KBUILD_CHECKSRC */
> > +#define rcu_dereference_sparse(p, space)
> > +#endif /* #else #ifdef KBUILD_CHECKSRC */
> 
> Did you see a problem with my macro?
> 
> #define rcu_dereference_sparse(p, space) \
>        ((void)(((typeof(*p) space *)NULL) == ((typeof(p))NULL)))

I don't see a specific problem with it.  However, I am not sure that
it really does what we want, and you indicated some doubts when you
posted it.  So I opted for something that very obviously will work.
If you can assure me that sparse will interpret the typeof()s and
space casts properly, I have no problem going with your version.

> I think this should warn in all the cases we want it to, but have no side-effects.

I still note a tone of uncertainty in the above sentence.  ;-)

> >  #define __rcu_access_pointer(p, space) \
> >         ({ \
> >                 typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
> > -               (void) (((typeof (*p) space *)p) == p); \
> > +               rcu_dereference_sparse(p, space); \
> >                 ((typeof(*p) __force __kernel *)(_________p1)); \
> >         })
> >  #define __rcu_dereference_check(p, c, space) \
> >         ({ \
> >                 typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
> >                 rcu_lockdep_assert(c); \
> > -               (void) (((typeof (*p) space *)p) == p); \
> > +               rcu_dereference_sparse(p, space); \
> >                 smp_read_barrier_depends(); \
> >                 ((typeof(*p) __force __kernel *)(_________p1)); \
> >         })
> >  #define __rcu_dereference_protected(p, c, space) \
> >         ({ \
> >                 rcu_lockdep_assert(c); \
> > -               (void) (((typeof (*p) space *)p) == p); \
> > +               rcu_dereference_sparse(p, space); \
> >                 ((typeof(*p) __force __kernel *)(p)); \
> >         })
> >  
> 
> This part might be useful in any case, to better document what the cast and
> compare does, and to prevent the three users from diverging.

And it would probably make sense to pull the rcu_dereference_sparse()
into the macro, for that matter.

> >diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
> >index 439ddab..adb09cb 100644
> >--- a/kernel/rcutorture.c
> >+++ b/kernel/rcutorture.c
> 
> This didn't seem to belong here.

Yep, I really should put this in a separate commit.

							Thanx, Paul