* Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked [not found] ` <5391FFFD.1060901@gatech.edu> @ 2014-06-06 18:11 ` Pranith Kumar 2014-06-07 0:18 ` Dave Chinner 0 siblings, 1 reply; 7+ messages in thread From: Pranith Kumar @ 2014-06-06 18:11 UTC (permalink / raw) To: peterz; +Cc: linux-kernel, xfs, mingo, davidlohr, tim.c.chen 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked 2014-06-06 18:11 ` [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked Pranith Kumar @ 2014-06-07 0:18 ` Dave Chinner 2014-06-07 0:59 ` Pranith Kumar 0 siblings, 1 reply; 7+ messages in thread From: Dave Chinner @ 2014-06-07 0:18 UTC (permalink / raw) To: Pranith Kumar; +Cc: peterz, linux-kernel, xfs, mingo, davidlohr, tim.c.chen 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked 2014-06-07 0:18 ` Dave Chinner @ 2014-06-07 0:59 ` Pranith Kumar 2014-06-07 1:41 ` Pranith Kumar 0 siblings, 1 reply; 7+ messages in thread From: Pranith Kumar @ 2014-06-07 0:59 UTC (permalink / raw) To: Dave Chinner, Pranith Kumar Cc: peterz, linux-kernel, xfs, mingo, davidlohr, tim.c.chen 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked 2014-06-07 0:59 ` Pranith Kumar @ 2014-06-07 1:41 ` Pranith Kumar 2014-06-07 2:39 ` Joe Perches 0 siblings, 1 reply; 7+ messages in thread From: Pranith Kumar @ 2014-06-07 1:41 UTC (permalink / raw) To: Dave Chinner, Pranith Kumar Cc: peterz, linux-kernel, xfs, mingo, davidlohr, tim.c.chen 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked 2014-06-07 1:41 ` Pranith Kumar @ 2014-06-07 2:39 ` Joe Perches 2014-06-07 23:44 ` Dave Chinner 0 siblings, 1 reply; 7+ messages in thread From: Joe Perches @ 2014-06-07 2:39 UTC (permalink / raw) To: Pranith Kumar Cc: peterz, tim.c.chen, linux-kernel, xfs, mingo, davidlohr, Pranith Kumar 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked 2014-06-07 2:39 ` Joe Perches @ 2014-06-07 23:44 ` Dave Chinner 2014-06-08 2:57 ` Pranith Kumar 0 siblings, 1 reply; 7+ messages in thread From: Dave Chinner @ 2014-06-07 23:44 UTC (permalink / raw) To: Joe Perches Cc: peterz, Pranith Kumar, linux-kernel, xfs, mingo, davidlohr, Pranith Kumar, tim.c.chen 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked 2014-06-07 23:44 ` Dave Chinner @ 2014-06-08 2:57 ` Pranith Kumar 0 siblings, 0 replies; 7+ messages in thread From: Pranith Kumar @ 2014-06-08 2:57 UTC (permalink / raw) To: Dave Chinner, Joe Perches Cc: peterz, Pranith Kumar, linux-kernel, xfs, mingo, davidlohr, tim.c.chen 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-06-08 2:57 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <5390D7E1.7060606@gatech.edu>
[not found] ` <5391FFFD.1060901@gatech.edu>
2014-06-06 18:11 ` [RFC PATCH 1/1] cleanup: use bool as return type for rwsem_is_locked Pranith Kumar
2014-06-07 0:18 ` Dave Chinner
2014-06-07 0:59 ` Pranith Kumar
2014-06-07 1:41 ` Pranith Kumar
2014-06-07 2:39 ` Joe Perches
2014-06-07 23:44 ` Dave Chinner
2014-06-08 2:57 ` Pranith Kumar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox