[lustre-devel] [PATCH 2/4] staging: lustre: hash.c: replace IS_PO2 by is_power_of_2

[Aya Mahfouz <mahfouz.saif.elyazal@gmail.com>]

On Sat, Oct 17, 2015 at 12:47:13PM +0200, Julia Lawall wrote:
> On Sat, 17 Oct 2015, Aya Mahfouz wrote:
> 
> > On Fri, Oct 16, 2015 at 10:40:25PM -0700, Greg KH wrote:
> > > On Sat, Oct 17, 2015 at 12:06:28AM +0200, Aya Mahfouz wrote:
> > > > Replaces IS_PO2 by is_power_of_2. IS_PO2 is used with several debug
> > > > macros. In this case, it is CDEBUG. Note that the replacement changes
> > > > the types involved, because the parameter of IS_PO2 is of type long long
> > > > and the return type is int, while the parameter of is_power_of_2 is of
> > > > type long and the return type is bool. This, however, has no impact,
> > > > because the actual argument is always of type int, and the return value
> > > > is always used as a boolean.
> > > > 
> > > > Signed-off-by: Aya Mahfouz <mahfouz.saif.elyazal@gmail.com>
> > > > ---
> > > >  drivers/staging/lustre/lustre/libcfs/hash.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/staging/lustre/lustre/libcfs/hash.c b/drivers/staging/lustre/lustre/libcfs/hash.c
> > > > index 6f4c7d4..4b5e79a 100644
> > > > --- a/drivers/staging/lustre/lustre/libcfs/hash.c
> > > > +++ b/drivers/staging/lustre/lustre/libcfs/hash.c
> > > > @@ -109,6 +109,8 @@
> > > >  
> > > >  #include "../../include/linux/libcfs/libcfs.h"
> > > >  #include <linux/seq_file.h>
> > > > +#include <linux/log2.h>
> > > > +
> > > >  
> > > >  #if CFS_HASH_DEBUG_LEVEL >= CFS_HASH_DEBUG_1
> > > >  static unsigned int warn_on_depth = 8;
> > > > @@ -1785,7 +1787,7 @@ cfs_hash_rehash_cancel_locked(struct cfs_hash *hs)
> > > >  	for (i = 2; cfs_hash_is_rehashing(hs); i++) {
> > > >  		cfs_hash_unlock(hs, 1);
> > > >  		/* raise console warning while waiting too long */
> > > > -		CDEBUG(IS_PO2(i >> 3) ? D_WARNING : D_INFO,
> > > > +		CDEBUG(is_power_of_2(i >> 3) ? D_WARNING : D_INFO,
> > > 
> > > is_power_of_2() works differently than IS_PO2(), are you _sure_ that the
> > > value here can not be 0?  If so, this will do something different :(
> > >
> > 
> > This is actually an interesting point to raise. __is_po2 the inline
> > function used by IS_PO2 should actually check if the number is greater
> > than 0. The current implementation of __is_po2 would allow the 
> > comparison of 0 with 2^(size of unsigned long long)-1. Is this correct?
> > Or is this something intentional?
> 
> What is the actual result in each case?
>

for __is_po2, if the number is 0 or power of 2 i.e. 1,2,4,8,16,32 etc
then return 1, 0 otherwise
for is_power_of_2 if the number is power of 2 return 1, 0 otherwise

> julia

-- 
Kind Regards,
Aya Saif El-yazal Mahfouz