[22/37] lustre: lprocfs: use log2.h macros instead of shift loop.
diff mbox series

Message ID 155053494599.24125.1795878344125234751.stgit@noble.brown
State New
Headers show
Series
  • More lustre patches from obdclass
Related show

Commit Message

NeilBrown Feb. 19, 2019, 12:09 a.m. UTC
These shift loops seem to be trying to avoid doing a
multiplication.
We same effect can be achieved more transparently using
rounddown_pow_of_two().  Even though there is a multiplication
in the C code, the resulting machine code just does a single shift.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 .../lustre/lustre/obdclass/lprocfs_status.c        |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Comments

James Simmons Feb. 24, 2019, 6:09 p.m. UTC | #1
> These shift loops seem to be trying to avoid doing a
> multiplication.
> We same effect can be achieved more transparently using
> rounddown_pow_of_two().  Even though there is a multiplication
> in the C code, the resulting machine code just does a single shift.

Be aware rounddown_pow_of_two(n) is undefined when "n == 0". We should
handle the "0" case as well. We are aware of this because this case
bit us in the LNet layer.
 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  .../lustre/lustre/obdclass/lprocfs_status.c        |   10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> index a179b0d6979e..637aaca96888 100644
> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> @@ -341,9 +341,7 @@ static ssize_t kbytestotal_show(struct kobject *kobj, struct attribute *attr,
>  		u32 blk_size = osfs.os_bsize >> 10;
>  		u64 result = osfs.os_blocks;
>  
> -		while (blk_size >>= 1)
> -			result <<= 1;
> -
> +		result *= rounddown_pow_of_two(blk_size);
>  		return sprintf(buf, "%llu\n", result);
>  	}
>  
> @@ -364,8 +362,7 @@ static ssize_t kbytesfree_show(struct kobject *kobj, struct attribute *attr,
>  		u32 blk_size = osfs.os_bsize >> 10;
>  		u64 result = osfs.os_bfree;
>  
> -		while (blk_size >>= 1)
> -			result <<= 1;
> +		result *= rounddown_pow_of_two(blk_size);
>  
>  		return sprintf(buf, "%llu\n", result);
>  	}
> @@ -387,8 +384,7 @@ static ssize_t kbytesavail_show(struct kobject *kobj, struct attribute *attr,
>  		u32 blk_size = osfs.os_bsize >> 10;
>  		u64 result = osfs.os_bavail;
>  
> -		while (blk_size >>= 1)
> -			result <<= 1;
> +		result *= rounddown_pow_of_two(blk_size);
>  
>  		return sprintf(buf, "%llu\n", result);
>  	}
> 
> 
>
James Simmons Feb. 26, 2019, 8:55 p.m. UTC | #2
> These shift loops seem to be trying to avoid doing a
> multiplication.
> We same effect can be achieved more transparently using
> rounddown_pow_of_two().  Even though there is a multiplication
> in the C code, the resulting machine code just does a single shift.

Be aware rounddown_pow_of_two(n) is undefined when "n == 0". 
 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  .../lustre/lustre/obdclass/lprocfs_status.c        |   10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> index a179b0d6979e..637aaca96888 100644
> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> @@ -341,9 +341,7 @@ static ssize_t kbytestotal_show(struct kobject *kobj, struct attribute *attr,
>  		u32 blk_size = osfs.os_bsize >> 10;
>  		u64 result = osfs.os_blocks;
>  
> -		while (blk_size >>= 1)
> -			result <<= 1;
> -
> +		result *= rounddown_pow_of_two(blk_size);
>  		return sprintf(buf, "%llu\n", result);
>  	}
>  
> @@ -364,8 +362,7 @@ static ssize_t kbytesfree_show(struct kobject *kobj, struct attribute *attr,
>  		u32 blk_size = osfs.os_bsize >> 10;
>  		u64 result = osfs.os_bfree;
>  
> -		while (blk_size >>= 1)
> -			result <<= 1;
> +		result *= rounddown_pow_of_two(blk_size);
>  
>  		return sprintf(buf, "%llu\n", result);
>  	}
> @@ -387,8 +384,7 @@ static ssize_t kbytesavail_show(struct kobject *kobj, struct attribute *attr,
>  		u32 blk_size = osfs.os_bsize >> 10;
>  		u64 result = osfs.os_bavail;
>  
> -		while (blk_size >>= 1)
> -			result <<= 1;
> +		result *= rounddown_pow_of_two(blk_size);
>  
>  		return sprintf(buf, "%llu\n", result);
>  	}
> 
> 
>
NeilBrown Feb. 27, 2019, 12:51 a.m. UTC | #3
On Tue, Feb 26 2019, James Simmons wrote:

>> These shift loops seem to be trying to avoid doing a
>> multiplication.
>> We same effect can be achieved more transparently using
>> rounddown_pow_of_two().  Even though there is a multiplication
>> in the C code, the resulting machine code just does a single shift.
>
> Be aware rounddown_pow_of_two(n) is undefined when "n == 0". 

Hmm... can os_bsize ever be less than 1024?  I guess we still need to be
careful of the possibility.

The current code treats anything less than 1024 as though it were 1024,
so I could achieve the same thing with 

		result *= rounddown_pow_of_two(blk_size ?: 1);

Is that too obscure?

Thanks,
NeilBrown


>  
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  .../lustre/lustre/obdclass/lprocfs_status.c        |   10 +++-------
>>  1 file changed, 3 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> index a179b0d6979e..637aaca96888 100644
>> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> @@ -341,9 +341,7 @@ static ssize_t kbytestotal_show(struct kobject *kobj, struct attribute *attr,
>>  		u32 blk_size = osfs.os_bsize >> 10;
>>  		u64 result = osfs.os_blocks;
>>  
>> -		while (blk_size >>= 1)
>> -			result <<= 1;
>> -
>> +		result *= rounddown_pow_of_two(blk_size);
>>  		return sprintf(buf, "%llu\n", result);
>>  	}
>>  
>> @@ -364,8 +362,7 @@ static ssize_t kbytesfree_show(struct kobject *kobj, struct attribute *attr,
>>  		u32 blk_size = osfs.os_bsize >> 10;
>>  		u64 result = osfs.os_bfree;
>>  
>> -		while (blk_size >>= 1)
>> -			result <<= 1;
>> +		result *= rounddown_pow_of_two(blk_size);
>>  
>>  		return sprintf(buf, "%llu\n", result);
>>  	}
>> @@ -387,8 +384,7 @@ static ssize_t kbytesavail_show(struct kobject *kobj, struct attribute *attr,
>>  		u32 blk_size = osfs.os_bsize >> 10;
>>  		u64 result = osfs.os_bavail;
>>  
>> -		while (blk_size >>= 1)
>> -			result <<= 1;
>> +		result *= rounddown_pow_of_two(blk_size);
>>  
>>  		return sprintf(buf, "%llu\n", result);
>>  	}
>> 
>> 
>>
Andreas Dilger Feb. 27, 2019, 12:54 a.m. UTC | #4
On Feb 26, 2019, at 17:51, NeilBrown <neilb@suse.com> wrote:
> 
> On Tue, Feb 26 2019, James Simmons wrote:
> 
>>> These shift loops seem to be trying to avoid doing a
>>> multiplication.
>>> We same effect can be achieved more transparently using
>>> rounddown_pow_of_two().  Even though there is a multiplication
>>> in the C code, the resulting machine code just does a single shift.
>> 
>> Be aware rounddown_pow_of_two(n) is undefined when "n == 0". 
> 
> Hmm... can os_bsize ever be less than 1024?  I guess we still need to be
> careful of the possibility.

In theory, ZFS allows a 512-byte blocksize, but we don't use that today.
There is some chance the MDT will be on a byte-granular NVRAM storage at
some point in the future, but even those would have an allocation unit
of 16-32 bytes or so.

> The current code treats anything less than 1024 as though it were 1024,
> so I could achieve the same thing with 
> 
> 		result *= rounddown_pow_of_two(blk_size ?: 1);
> 
> Is that too obscure?

Should be fine.

Cheers, Andreas

> 
> Thanks,
> NeilBrown
> 
> 
>> 
>>> Signed-off-by: NeilBrown <neilb@suse.com>
>>> ---
>>> .../lustre/lustre/obdclass/lprocfs_status.c        |   10 +++-------
>>> 1 file changed, 3 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>>> index a179b0d6979e..637aaca96888 100644
>>> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>>> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>>> @@ -341,9 +341,7 @@ static ssize_t kbytestotal_show(struct kobject *kobj, struct attribute *attr,
>>> 		u32 blk_size = osfs.os_bsize >> 10;
>>> 		u64 result = osfs.os_blocks;
>>> 
>>> -		while (blk_size >>= 1)
>>> -			result <<= 1;
>>> -
>>> +		result *= rounddown_pow_of_two(blk_size);
>>> 		return sprintf(buf, "%llu\n", result);
>>> 	}
>>> 
>>> @@ -364,8 +362,7 @@ static ssize_t kbytesfree_show(struct kobject *kobj, struct attribute *attr,
>>> 		u32 blk_size = osfs.os_bsize >> 10;
>>> 		u64 result = osfs.os_bfree;
>>> 
>>> -		while (blk_size >>= 1)
>>> -			result <<= 1;
>>> +		result *= rounddown_pow_of_two(blk_size);
>>> 
>>> 		return sprintf(buf, "%llu\n", result);
>>> 	}
>>> @@ -387,8 +384,7 @@ static ssize_t kbytesavail_show(struct kobject *kobj, struct attribute *attr,
>>> 		u32 blk_size = osfs.os_bsize >> 10;
>>> 		u64 result = osfs.os_bavail;
>>> 
>>> -		while (blk_size >>= 1)
>>> -			result <<= 1;
>>> +		result *= rounddown_pow_of_two(blk_size);
>>> 
>>> 		return sprintf(buf, "%llu\n", result);
>>> 	}
>>> 
>>> 
>>> 

Cheers, Andreas
---
Andreas Dilger
CTO Whamcloud

Patch
diff mbox series

diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
index a179b0d6979e..637aaca96888 100644
--- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
+++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
@@ -341,9 +341,7 @@  static ssize_t kbytestotal_show(struct kobject *kobj, struct attribute *attr,
 		u32 blk_size = osfs.os_bsize >> 10;
 		u64 result = osfs.os_blocks;
 
-		while (blk_size >>= 1)
-			result <<= 1;
-
+		result *= rounddown_pow_of_two(blk_size);
 		return sprintf(buf, "%llu\n", result);
 	}
 
@@ -364,8 +362,7 @@  static ssize_t kbytesfree_show(struct kobject *kobj, struct attribute *attr,
 		u32 blk_size = osfs.os_bsize >> 10;
 		u64 result = osfs.os_bfree;
 
-		while (blk_size >>= 1)
-			result <<= 1;
+		result *= rounddown_pow_of_two(blk_size);
 
 		return sprintf(buf, "%llu\n", result);
 	}
@@ -387,8 +384,7 @@  static ssize_t kbytesavail_show(struct kobject *kobj, struct attribute *attr,
 		u32 blk_size = osfs.os_bsize >> 10;
 		u64 result = osfs.os_bavail;
 
-		while (blk_size >>= 1)
-			result <<= 1;
+		result *= rounddown_pow_of_two(blk_size);
 
 		return sprintf(buf, "%llu\n", result);
 	}