Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked

Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked

[Pranith Kumar]

On 06/06/2014 01:53 PM, Pranith Kumar wrote:
> On Fri, Jun 6, 2014 at 3:35 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> Now in general, I don't particularly like such superfluous changes, so
>> unless you can show that GCC actually generates better code, I'd prefer
>> to keep things as they are.
> 
> Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
> 
> use bool as the return type for rwsem_is_locked() instead of int
> 
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
>  include/linux/rwsem-spinlock.h  |    2 +-
>  include/linux/rwsem.h           |    2 +-
>  kernel/locking/rwsem-spinlock.c |    4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/rwsem-spinlock.h b/include/linux/rwsem-spinlock.h
> index d5b13bc..9026d2a 100644
> --- a/include/linux/rwsem-spinlock.h
> +++ b/include/linux/rwsem-spinlock.h
> @@ -39,7 +39,7 @@ extern int __down_write_trylock(struct rw_semaphore *sem);
>  extern void __up_read(struct rw_semaphore *sem);
>  extern void __up_write(struct rw_semaphore *sem);
>  extern void __downgrade_write(struct rw_semaphore *sem);
> -extern int rwsem_is_locked(struct rw_semaphore *sem);
> +extern bool rwsem_is_locked(struct rw_semaphore *sem);
>  
>  #endif /* __KERNEL__ */
>  #endif /* _LINUX_RWSEM_SPINLOCK_H */
> diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
> index 03f3b05..b056780 100644
> --- a/include/linux/rwsem.h
> +++ b/include/linux/rwsem.h
> @@ -40,7 +40,7 @@ extern struct rw_semaphore *rwsem_downgrade_wake(struct rw_semaphore *sem);
>  #include <asm/rwsem.h>
>  
>  /* In all implementations count != 0 means locked */
> -static inline int rwsem_is_locked(struct rw_semaphore *sem)
> +static inline bool rwsem_is_locked(struct rw_semaphore *sem)
>  {
>  	return sem->count != 0;
>  }
> diff --git a/kernel/locking/rwsem-spinlock.c b/kernel/locking/rwsem-spinlock.c
> index 9be8a91..3f8adf8 100644
> --- a/kernel/locking/rwsem-spinlock.c
> +++ b/kernel/locking/rwsem-spinlock.c
> @@ -20,9 +20,9 @@ struct rwsem_waiter {
>  	enum rwsem_waiter_type type;
>  };
>  
> -int rwsem_is_locked(struct rw_semaphore *sem)
> +bool rwsem_is_locked(struct rw_semaphore *sem)
>  {
> -	int ret = 1;
> +	bool ret = true;
>  	unsigned long flags;
>  
>  	if (raw_spin_trylock_irqsave(&sem->wait_lock, flags)) {
> 


I observed one other user of rwsem_is_locked() in xfs, change accordingly

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 fs/xfs/xfs_inode.c |    2 +-
 fs/xfs/xfs_inode.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 768087b..9047eda 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -285,7 +285,7 @@ xfs_ilock_demote(
 }
 
 #if defined(DEBUG) || defined(XFS_WARN)
-int
+bool
 xfs_isilocked(
 	xfs_inode_t		*ip,
 	uint			lock_flags)
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index f2fcde5..80649a1 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -348,7 +348,7 @@ void		xfs_ilock(xfs_inode_t *, uint);
 int		xfs_ilock_nowait(xfs_inode_t *, uint);
 void		xfs_iunlock(xfs_inode_t *, uint);
 void		xfs_ilock_demote(xfs_inode_t *, uint);
-int		xfs_isilocked(xfs_inode_t *, uint);
+bool		xfs_isilocked(xfs_inode_t *, uint);
 uint		xfs_ilock_data_map_shared(struct xfs_inode *);
 uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
 int		xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
-- 
1.7.9.5



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked

[Dave Chinner]

On Fri, Jun 06, 2014 at 02:11:18PM -0400, Pranith Kumar wrote:
> On 06/06/2014 01:53 PM, Pranith Kumar wrote:
> > On Fri, Jun 6, 2014 at 3:35 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >>
> >> Now in general, I don't particularly like such superfluous changes, so
> >> unless you can show that GCC actually generates better code, I'd prefer
> >> to keep things as they are.
> > 
> > Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
> > 
> > use bool as the return type for rwsem_is_locked() instead of int
> > 
> > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
....

Makes sense to me.

> I observed one other user of rwsem_is_locked() in xfs, change accordingly
> 
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
>  fs/xfs/xfs_inode.c |    2 +-
>  fs/xfs/xfs_inode.h |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 768087b..9047eda 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -285,7 +285,7 @@ xfs_ilock_demote(
>  }
>  
>  #if defined(DEBUG) || defined(XFS_WARN)
> -int
> +bool
>  xfs_isilocked(
>  	xfs_inode_t		*ip,
>  	uint			lock_flags)

If you are going to change the return type to bool, then you should
also remove the manual "!!" conversions to a boolean return and let
the compiler do it in the most optimal way.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked

[Pranith Kumar]

On 06/06/2014 08:18 PM, Dave Chinner wrote:
> On Fri, Jun 06, 2014 at 02:11:18PM -0400, Pranith Kumar wrote:
>> On 06/06/2014 01:53 PM, Pranith Kumar wrote:
>>> On Fri, Jun 6, 2014 at 3:35 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>>>>
>>>> Now in general, I don't particularly like such superfluous changes, so
>>>> unless you can show that GCC actually generates better code, I'd prefer
>>>> to keep things as they are.
>>>
>>> Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
>>>
>>> use bool as the return type for rwsem_is_locked() instead of int
>>>

> 
> If you are going to change the return type to bool, then you should
> also remove the manual "!!" conversions to a boolean return and let
> the compiler do it in the most optimal way.
> 
 
Agreed, please find patch below:

change return type to bool to follow rwsem_is_locked()

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 fs/xfs/xfs_inode.c | 8 ++++----
 fs/xfs/xfs_inode.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index a6115fe..c02ac49 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -285,25 +285,25 @@ xfs_ilock_demote(
 }
 
 #if defined(DEBUG) || defined(XFS_WARN)
-int
+bool
 xfs_isilocked(
 	xfs_inode_t		*ip,
 	uint			lock_flags)
 {
 	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
 		if (!(lock_flags & XFS_ILOCK_SHARED))
-			return !!ip->i_lock.mr_writer;
+			return !(ip->i_lock.mr_writer == 0);
 		return rwsem_is_locked(&ip->i_lock.mr_lock);
 	}
 
 	if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) {
 		if (!(lock_flags & XFS_IOLOCK_SHARED))
-			return !!ip->i_iolock.mr_writer;
+			return !(ip->i_iolock.mr_writer == 0);
 		return rwsem_is_locked(&ip->i_iolock.mr_lock);
 	}
 
 	ASSERT(0);
-	return 0;
+	return false;
 }
 #endif
 
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index f72bffa..efebed6 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -346,7 +346,7 @@ void		xfs_ilock(xfs_inode_t *, uint);
 int		xfs_ilock_nowait(xfs_inode_t *, uint);
 void		xfs_iunlock(xfs_inode_t *, uint);
 void		xfs_ilock_demote(xfs_inode_t *, uint);
-int		xfs_isilocked(xfs_inode_t *, uint);
+bool		xfs_isilocked(xfs_inode_t *, uint);
 uint		xfs_ilock_data_map_shared(struct xfs_inode *);
 uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
 int		xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
-- 
1.9.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked

[Pranith Kumar]

On 06/06/2014 08:59 PM, Pranith Kumar wrote:
> On 06/06/2014 08:18 PM, Dave Chinner wrote:
>> On Fri, Jun 06, 2014 at 02:11:18PM -0400, Pranith Kumar wrote:
>>> On 06/06/2014 01:53 PM, Pranith Kumar wrote:
>>>> On Fri, Jun 6, 2014 at 3:35 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>>>>>
>>>>> Now in general, I don't particularly like such superfluous changes, so
>>>>> unless you can show that GCC actually generates better code, I'd prefer
>>>>> to keep things as they are.
>>>>
>>>> Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
>>>>
>>>> use bool as the return type for rwsem_is_locked() instead of int
>>>>
> 
>>
>> If you are going to change the return type to bool, then you should
>> also remove the manual "!!" conversions to a boolean return and let
>> the compiler do it in the most optimal way.
>>
>  
> Agreed, please find patch below:
> 

Simplify the "!!" condition. This is much simpler. :)

change return type to bool to follow rwsem_is_locked()

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 fs/xfs/xfs_inode.c | 8 ++++----
 fs/xfs/xfs_inode.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index a6115fe..c02ac49 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -285,25 +285,25 @@ xfs_ilock_demote(
 }
 
 #if defined(DEBUG) || defined(XFS_WARN)
-int
+bool
 xfs_isilocked(
 	xfs_inode_t		*ip,
 	uint			lock_flags)
 {
 	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
 		if (!(lock_flags & XFS_ILOCK_SHARED))
-			return !!ip->i_lock.mr_writer;
+			return (ip->i_lock.mr_writer != 0);
 		return rwsem_is_locked(&ip->i_lock.mr_lock);
 	}
 
 	if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) {
 		if (!(lock_flags & XFS_IOLOCK_SHARED))
-			return !!ip->i_iolock.mr_writer;
+			return (ip->i_iolock.mr_writer != 0);
 		return rwsem_is_locked(&ip->i_iolock.mr_lock);
 	}
 
 	ASSERT(0);
-	return 0;
+	return false;
 }
 #endif
 
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index f72bffa..efebed6 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -346,7 +346,7 @@ void		xfs_ilock(xfs_inode_t *, uint);
 int		xfs_ilock_nowait(xfs_inode_t *, uint);
 void		xfs_iunlock(xfs_inode_t *, uint);
 void		xfs_ilock_demote(xfs_inode_t *, uint);
-int		xfs_isilocked(xfs_inode_t *, uint);
+bool		xfs_isilocked(xfs_inode_t *, uint);
 uint		xfs_ilock_data_map_shared(struct xfs_inode *);
 uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
 int		xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
-- 
1.9.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked

[Joe Perches]

On Fri, 2014-06-06 at 21:41 -0400, Pranith Kumar wrote:
> On 06/06/2014 08:59 PM, Pranith Kumar wrote:
> > On 06/06/2014 08:18 PM, Dave Chinner wrote:
> >> If you are going to change the return type to bool, then you should
> >> also remove the manual "!!" conversions to a boolean return and let
> >> the compiler do it in the most optimal way.
> > Agreed, please find patch below:
> Simplify the "!!" condition. This is much simpler. :)
[]
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c

> @@ -285,25 +285,25 @@ xfs_ilock_demote(
>  }
>  
>  #if defined(DEBUG) || defined(XFS_WARN)
> -int
> +bool
>  xfs_isilocked(
>  	xfs_inode_t		*ip,
>  	uint			lock_flags)
>  {
>  	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
>  		if (!(lock_flags & XFS_ILOCK_SHARED))
> -			return !!ip->i_lock.mr_writer;
> +			return (ip->i_lock.mr_writer != 0);

simpler still would be just removing the !! completely.
I presume in no case would it make an actual difference
in emitted code.

ie:
			return ip->i_lock.mr_writer;


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked

[Dave Chinner]

On Fri, Jun 06, 2014 at 07:39:30PM -0700, Joe Perches wrote:
> On Fri, 2014-06-06 at 21:41 -0400, Pranith Kumar wrote:
> > On 06/06/2014 08:59 PM, Pranith Kumar wrote:
> > > On 06/06/2014 08:18 PM, Dave Chinner wrote:
> > >> If you are going to change the return type to bool, then you should
> > >> also remove the manual "!!" conversions to a boolean return and let
> > >> the compiler do it in the most optimal way.
> > > Agreed, please find patch below:
> > Simplify the "!!" condition. This is much simpler. :)
> []
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> 
> > @@ -285,25 +285,25 @@ xfs_ilock_demote(
> >  }
> >  
> >  #if defined(DEBUG) || defined(XFS_WARN)
> > -int
> > +bool
> >  xfs_isilocked(
> >  	xfs_inode_t		*ip,
> >  	uint			lock_flags)
> >  {
> >  	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> >  		if (!(lock_flags & XFS_ILOCK_SHARED))
> > -			return !!ip->i_lock.mr_writer;
> > +			return (ip->i_lock.mr_writer != 0);
> 
> simpler still would be just removing the !! completely.
> I presume in no case would it make an actual difference
> in emitted code.
> 
> ie:
> 			return ip->i_lock.mr_writer;

Yup, that's exactly what I meant. Casting to a bool type does all
the work of squashing all non-zero values to 1...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked

[Pranith Kumar]

On 06/07/2014 07:44 PM, Dave Chinner wrote:
> On Fri, Jun 06, 2014 at 07:39:30PM -0700, Joe Perches wrote:
>> On Fri, 2014-06-06 at 21:41 -0400, Pranith Kumar wrote:
>>> On 06/06/2014 08:59 PM, Pranith Kumar wrote:
>>>> On 06/06/2014 08:18 PM, Dave Chinner wrote:
>>>>> If you are going to change the return type to bool, then you should
>>>>> also remove the manual "!!" conversions to a boolean return and let
>>>>> the compiler do it in the most optimal way.
>> simpler still would be just removing the !! completely.
>> I presume in no case would it make an actual difference
>> in emitted code.
>>
>> ie:
>> 			return ip->i_lock.mr_writer;
> 
> Yup, that's exactly what I meant. Casting to a bool type does all
> the work of squashing all non-zero values to 1...
> 

change return type if xfs_isilocked() to bool to follow rwsem_is_locked()

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 fs/xfs/xfs_inode.c | 8 ++++----
 fs/xfs/xfs_inode.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index a6115fe..c02ac49 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -285,25 +285,25 @@ xfs_ilock_demote(
 }
 
 #if defined(DEBUG) || defined(XFS_WARN)
-int
+bool
 xfs_isilocked(
 	xfs_inode_t		*ip,
 	uint			lock_flags)
 {
 	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
 		if (!(lock_flags & XFS_ILOCK_SHARED))
-			return !!ip->i_lock.mr_writer;
+			return ip->i_lock.mr_writer;
 		return rwsem_is_locked(&ip->i_lock.mr_lock);
 	}
 
 	if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) {
 		if (!(lock_flags & XFS_IOLOCK_SHARED))
-			return !!ip->i_iolock.mr_writer;
+			return ip->i_iolock.mr_writer;
 		return rwsem_is_locked(&ip->i_iolock.mr_lock);
 	}
 
 	ASSERT(0);
-	return 0;
+	return false;
 }
 #endif
 
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index f72bffa..efebed6 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -346,7 +346,7 @@ void		xfs_ilock(xfs_inode_t *, uint);
 int		xfs_ilock_nowait(xfs_inode_t *, uint);
 void		xfs_iunlock(xfs_inode_t *, uint);
 void		xfs_ilock_demote(xfs_inode_t *, uint);
-int		xfs_isilocked(xfs_inode_t *, uint);
+bool		xfs_isilocked(xfs_inode_t *, uint);
 uint		xfs_ilock_data_map_shared(struct xfs_inode *);
 uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
 int		xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
-- 
1.9.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs