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

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

[Pranith Kumar]

I see that there are functions like this which basically say:

return 1 if true else return 0. Is it worth cleaning them up? Or is there any reason why this convention is followed?

use bool as the return type. No reason for return type to be 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 |    2 +-
 3 files changed, 3 insertions(+), 3 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 091d993..04faf87 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -49,7 +49,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..7374139 100644
--- a/kernel/locking/rwsem-spinlock.c
+++ b/kernel/locking/rwsem-spinlock.c
@@ -20,7 +20,7 @@ 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;
 	unsigned long flags;
-- 
1.7.9.5

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

[Peter Zijlstra]

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

On Thu, Jun 05, 2014 at 04:49:37PM -0400, Pranith Kumar wrote:
> I see that there are functions like this which basically say:
> 
> return 1 if true else return 0. Is it worth cleaning them up? Or is
> there any reason why this convention is followed?

Hysterical raisins, a lot of people learnt C before it grew bool,
including me.

> use bool as the return type. No reason for return type to be int.
> 
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---

> diff --git a/kernel/locking/rwsem-spinlock.c b/kernel/locking/rwsem-spinlock.c
> index 9be8a91..7374139 100644
> --- a/kernel/locking/rwsem-spinlock.c
> +++ b/kernel/locking/rwsem-spinlock.c
> @@ -20,7 +20,7 @@ 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;
>  	unsigned long flags;

Now, see that's a half arsed change, if you change the function return
value, you should also change the value we actually return, @ret above
to bool, and you should then also change the values used to 'true' and
'false'.

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.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Pranith Kumar]

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)) {
-- 
1.7.9.5

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

[Peter Zijlstra]

On Fri, Jun 06, 2014 at 01:53:01PM -0400, 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 :(

still 2 bytes, so sure.

Which gcc did you use and what arch did you build? That might be useful
info for the changelog.

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

[Pranith Kumar]

On Fri, Jun 6, 2014 at 1:56 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
>
> still 2 bytes, so sure.
>
> Which gcc did you use and what arch did you build? That might be useful
> info for the changelog.

I am using gcc (Ubuntu/Linaro 4.7.3-2ubuntu1~12.04) 4.7.3. I tested
this using ARCH=um with a Pentium processor to turn on
CONFIG_RWSEM_GENERIC_SPINLOCK.

-- 
Pranith

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

[Davidlohr Bueso]

On Fri, 2014-06-06 at 19:56 +0200, Peter Zijlstra wrote:
> On Fri, Jun 06, 2014 at 01:53:01PM -0400, 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 :(
> 
> still 2 bytes, so sure.
> 
> Which gcc did you use and what arch did you build? That might be useful
> info for the changelog.

Yeah, please attach the output of 'size kernel/locking/rwsem.o' for both
before and after. I think there's similar opportunity in other locking
code as well.

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

[Pranith Kumar]

On Fri, Jun 6, 2014 at 2:41 PM, Davidlohr Bueso <davidlohr@hp.com> wrote:
> On Fri, 2014-06-06 at 19:56 +0200, Peter Zijlstra wrote:
>> On Fri, Jun 06, 2014 at 01:53:01PM -0400, 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 :(
>>
>> still 2 bytes, so sure.
>>
>> Which gcc did you use and what arch did you build? That might be useful
>> info for the changelog.
>
> Yeah, please attach the output of 'size kernel/locking/rwsem.o' for both
> before and after. I think there's similar opportunity in other locking
> code as well.
>

Ok. Here is the data.

size kernel/locking/rwsem-spinlock.o

Before change to bool:

   text       data        bss        dec        hex
   1336          0          0       1336        538

After change to bool:

   text       data        bss        dec        hex
   1334          0          0       1334        536

-- 
Pranith

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

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

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

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

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;

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

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