cleanup: use bool as return type for rwsem_is_locked

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 <[email protected]>

---

kernel/locking/rwsem-spinlock.c +0 -0

index 9be8a91..7374139 100644

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.

Quoted 4 lines · Peter Zijlstra

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 :(

Quoted 1 line

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.

Quoted 6 lines · Pranith Kumar

Quoted 4 lines · Peter Zijlstra

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.

Quoted 1 line

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.

Quoted 9 lines · Peter Zijlstra

Quoted 7 lines · Pranith Kumar

Quoted 4 lines · Peter Zijlstra

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 :(

Quoted 4 lines

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.

Quoted 4 lines · Peter Zijlstra

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 <[email protected]>

---
 include/linux/rwsem-spinlock.h  |    2 +-
 include/linux/rwsem.h           |    2 +-
 kernel/locking/rwsem-spinlock.c |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

include/linux/rwsem-spinlock.h +1 -1

index d5b13bc..9026d2a 100644
@@ -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 */

include/linux/rwsem.h +1 -1

index 03f3b05..b056780 100644
@@ -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;
 }

kernel/locking/rwsem-spinlock.c +0 -0

index 9be8a91..3f8adf8 100644
@@ -20,9 +20,9 @@ struct rwsem_waiter {
 	enum rwsem_waiter_type type;

Quoted 6 lines · Pranith Kumar

Quoted 4 lines · Peter Zijlstra

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.

Quoted 5 lines

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 <[email protected]>

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

Signed-off-by Pranith Kumar <[email protected]>

---
 fs/xfs/xfs_inode.c |    2 +-
 fs/xfs/xfs_inode.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

fs/xfs/xfs_inode.c +0 -0

index 768087b..9047eda 100644

Quoted 9 lines · Pranith Kumar

Quoted 8 lines · Pranith Kumar

Quoted 4 lines · Peter Zijlstra

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 :(

Quoted 2 lines

Quoted 2 lines

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.

Quoted 14 lines · Dave Chinner

Quoted 11 lines · Pranith Kumar

Quoted 10 lines · Pranith Kumar

Quoted 4 lines · Peter Zijlstra

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

Quoted 4 lines

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:

Quoted 5 lines · Pranith Kumar

Quoted 3 lines · Dave Chinner

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. :)

fs/xfs/xfs_inode.c +0 -0

@@ -285,25 +285,25 @@ xfs_ilock_demote(
 }
 
 #if defined(DEBUG) || defined(XFS_WARN)

Quoted 7 lines · Pranith Kumar

Quoted 5 lines · Pranith Kumar

Quoted 3 lines · Dave Chinner

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. :)

[]

Quoted 6 lines

fs/xfs/xfs_inode.c +0 -0

@@ -285,25 +285,25 @@ xfs_ilock_demote(
 }
 
 #if defined(DEBUG) || defined(XFS_WARN)

fs/xfs/xfs_inode.c xfs_ilock_demote( · line 295

Quoted 10 lines

fs/xfs/xfs_inode.c +2 -2

-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;

Quoted 10 lines · Joe Perches

Quoted 5 lines · Pranith Kumar

Quoted 4 lines · Pranith Kumar

Quoted 3 lines · Dave Chinner

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.

Quoted 2 lines

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...