| Message ID | 20231110204119.3692023-2-willy@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove the XFS mrlock | expand |
On 11/10/23 15:41, Matthew Wilcox (Oracle) wrote: > Modelled after lockdep_assert_held() and lockdep_assert_held_write(), > but are always active, even when lockdep is disabled. Of course, they > don't test that _this_ thread is the owner, but it's sufficient to catch > many bugs and doesn't incur the same performance penalty as lockdep. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > include/linux/rwbase_rt.h | 9 ++++++-- > include/linux/rwsem.h | 46 ++++++++++++++++++++++++++++++++++----- > 2 files changed, 48 insertions(+), 7 deletions(-) > > diff --git a/include/linux/rwbase_rt.h b/include/linux/rwbase_rt.h > index 1d264dd08625..a04acd85705b 100644 > --- a/include/linux/rwbase_rt.h > +++ b/include/linux/rwbase_rt.h > @@ -26,12 +26,17 @@ struct rwbase_rt { > } while (0) > > > -static __always_inline bool rw_base_is_locked(struct rwbase_rt *rwb) > +static __always_inline bool rw_base_is_locked(const struct rwbase_rt *rwb) > { > return atomic_read(&rwb->readers) != READER_BIAS; > } > > -static __always_inline bool rw_base_is_contended(struct rwbase_rt *rwb) > +static inline void rw_base_assert_held_write(const struct rwbase_rt *rwb) > +{ > + BUG_ON(atomic_read(&rwb->readers) != WRITER_BIAS); > +} > + > +static __always_inline bool rw_base_is_contended(const struct rwbase_rt *rwb) > { > return atomic_read(&rwb->readers) > 0; > } > diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h > index 1dd530ce8b45..b5b34cca86f3 100644 > --- a/include/linux/rwsem.h > +++ b/include/linux/rwsem.h > @@ -66,14 +66,24 @@ struct rw_semaphore { > #endif > }; > > -/* In all implementations count != 0 means locked */ > +#define RWSEM_UNLOCKED_VALUE 0UL > +#define RWSEM_WRITER_LOCKED (1UL << 0) > +#define __RWSEM_COUNT_INIT(name) .count = ATOMIC_LONG_INIT(RWSEM_UNLOCKED_VALUE) > + > static inline int rwsem_is_locked(struct rw_semaphore *sem) > { > - return atomic_long_read(&sem->count) != 0; > + return atomic_long_read(&sem->count) != RWSEM_UNLOCKED_VALUE; > } > > -#define RWSEM_UNLOCKED_VALUE 0L > -#define __RWSEM_COUNT_INIT(name) .count = ATOMIC_LONG_INIT(RWSEM_UNLOCKED_VALUE) > +static inline void rwsem_assert_held_nolockdep(const struct rw_semaphore *sem) > +{ > + WARN_ON(atomic_long_read(&sem->count) == RWSEM_UNLOCKED_VALUE); > +} That is not correct. You mean "!= RWSEM_UNLOCKED_VALUE". Right? > + > +static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem) > +{ > + WARN_ON(!(atomic_long_read(&sem->count) & RWSEM_WRITER_LOCKED)); > +} > > /* Common initializer macros and functions */ > > @@ -152,11 +162,21 @@ do { \ > __init_rwsem((sem), #sem, &__key); \ > } while (0) > > -static __always_inline int rwsem_is_locked(struct rw_semaphore *sem) > +static __always_inline int rwsem_is_locked(const struct rw_semaphore *sem) > { > return rw_base_is_locked(&sem->rwbase); > } > > +static inline void rwsem_assert_held_nolockdep(const struct rw_semaphore *sem) > +{ > + BUG_ON(!rwsem_is_locked(sem)); > +} > + There are some inconsistency in the use of WARN_ON() and BUG_ON() in the assertions. For PREEMPT_RT, held_write is a BUG_ON. For non-PREEMPT_RT, held is a BUG_ON. It is not clear why one is BUG_ON and other one is WARN_ON. Is there a rationale for that? BTW, we can actually check if the current process is the write-lock owner of a rwsem, but not for a reader-owned rwsem. Cheers, Longman
On Fri, Nov 10, 2023 at 08:41:16PM +0000, Matthew Wilcox (Oracle) wrote: > +static inline void rwsem_assert_held_nolockdep(const struct rw_semaphore *sem) > +{ > + WARN_ON(atomic_long_read(&sem->count) == RWSEM_UNLOCKED_VALUE); > +} > + > +static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem) > +{ > + WARN_ON(!(atomic_long_read(&sem->count) & RWSEM_WRITER_LOCKED)); > +} > +static inline void rwsem_assert_held_nolockdep(const struct rw_semaphore *sem) > +{ > + BUG_ON(!rwsem_is_locked(sem)); > +} What's with the WARN_ON() vs BUG_ON() thing?
On Fri, Nov 10, 2023 at 05:21:22PM -0500, Waiman Long wrote: > On 11/10/23 15:41, Matthew Wilcox (Oracle) wrote: > > static inline int rwsem_is_locked(struct rw_semaphore *sem) > > { > > - return atomic_long_read(&sem->count) != 0; > > + return atomic_long_read(&sem->count) != RWSEM_UNLOCKED_VALUE; > > } > > -#define RWSEM_UNLOCKED_VALUE 0L > > -#define __RWSEM_COUNT_INIT(name) .count = ATOMIC_LONG_INIT(RWSEM_UNLOCKED_VALUE) > > +static inline void rwsem_assert_held_nolockdep(const struct rw_semaphore *sem) > > +{ > > + WARN_ON(atomic_long_read(&sem->count) == RWSEM_UNLOCKED_VALUE); > > +} > That is not correct. You mean "!= RWSEM_UNLOCKED_VALUE". Right? Uhhh ... I always get confused between assert and BUG_ON being opposite polarity, but I think it's correct. We are asserting that the rwsem is locked (either for read or write). That is, it is a bug if the rwsem is unlocked. So WARN_ON(sem->count == UNLOCKED_VALUE) is correct. No? > There are some inconsistency in the use of WARN_ON() and BUG_ON() in the > assertions. For PREEMPT_RT, held_write is a BUG_ON. For non-PREEMPT_RT, held > is a BUG_ON. It is not clear why one is BUG_ON and other one is WARN_ON. Is > there a rationale for that? I'll fix that up. > BTW, we can actually check if the current process is the write-lock owner of > a rwsem, but not for a reader-owned rwsem. We actually don't want to do that. See patches 3/4 where I explain how XFS takes the XFS_ILOCK for write, then passes control to a workqueue which asserts that the XFS_ILOCK is held for write. The thread which took the rwsem for write waits for the workqueue and unlocks the rwsem.
On 11/14/23 16:26, Matthew Wilcox wrote: > On Fri, Nov 10, 2023 at 05:21:22PM -0500, Waiman Long wrote: >> On 11/10/23 15:41, Matthew Wilcox (Oracle) wrote: >>> static inline int rwsem_is_locked(struct rw_semaphore *sem) >>> { >>> - return atomic_long_read(&sem->count) != 0; >>> + return atomic_long_read(&sem->count) != RWSEM_UNLOCKED_VALUE; >>> } >>> -#define RWSEM_UNLOCKED_VALUE 0L >>> -#define __RWSEM_COUNT_INIT(name) .count = ATOMIC_LONG_INIT(RWSEM_UNLOCKED_VALUE) >>> +static inline void rwsem_assert_held_nolockdep(const struct rw_semaphore *sem) >>> +{ >>> + WARN_ON(atomic_long_read(&sem->count) == RWSEM_UNLOCKED_VALUE); >>> +} >> That is not correct. You mean "!= RWSEM_UNLOCKED_VALUE". Right? > Uhhh ... I always get confused between assert and BUG_ON being opposite > polarity, but I think it's correct. > > We are asserting that the rwsem is locked (either for read or write). > That is, it is a bug if the rwsem is unlocked. > So WARN_ON(sem->count == UNLOCKED_VALUE) is correct. No? You are right. I got confused too. > >> There are some inconsistency in the use of WARN_ON() and BUG_ON() in the >> assertions. For PREEMPT_RT, held_write is a BUG_ON. For non-PREEMPT_RT, held >> is a BUG_ON. It is not clear why one is BUG_ON and other one is WARN_ON. Is >> there a rationale for that? > I'll fix that up. The check for write lock ownership is accurate. OTOH, the locked check can have false positive and so is less reliable. > >> BTW, we can actually check if the current process is the write-lock owner of >> a rwsem, but not for a reader-owned rwsem. > We actually don't want to do that. See patches 3/4 where I explain how > XFS takes the XFS_ILOCK for write, then passes control to a workqueue > which asserts that the XFS_ILOCK is held for write. The thread which > took the rwsem for write waits for the workqueue and unlocks the rwsem. > I see. Thanks for the explanation. Cheers, Longman
On Tue, Nov 14, 2023 at 08:17:32PM -0500, Waiman Long wrote: > > > There are some inconsistency in the use of WARN_ON() and BUG_ON() in the > > > assertions. For PREEMPT_RT, held_write is a BUG_ON. For non-PREEMPT_RT, held > > > is a BUG_ON. It is not clear why one is BUG_ON and other one is WARN_ON. Is > > > there a rationale for that? > > I'll fix that up. > The check for write lock ownership is accurate. OTOH, the locked check can > have false positive and so is less reliable. When you say 'false positive', do you mean it might report the lock as being held, when it actually isn't, or report the lock as not being held when it actually is? The differing polarities of assert and BUG_ON make this confusing as usual. Obviously, for an assert, we're OK with it reporting that the lock is held when actually it's not. The caller is expected to hold the lock, so failing to trip the assert when the caller doesn't hold the lock isn't great, but we can live with it. OTOH, if the assert fires when the caller does hold the lock, that is not tolerable.
On 11/16/23 11:12, Matthew Wilcox wrote: > On Tue, Nov 14, 2023 at 08:17:32PM -0500, Waiman Long wrote: >>>> There are some inconsistency in the use of WARN_ON() and BUG_ON() in the >>>> assertions. For PREEMPT_RT, held_write is a BUG_ON. For non-PREEMPT_RT, held >>>> is a BUG_ON. It is not clear why one is BUG_ON and other one is WARN_ON. Is >>>> there a rationale for that? >>> I'll fix that up. >> The check for write lock ownership is accurate. OTOH, the locked check can >> have false positive and so is less reliable. > When you say 'false positive', do you mean it might report the lock as > being held, when it actually isn't, or report the lock as not being held > when it actually is? The differing polarities of assert and BUG_ON > make this confusing as usual. It means there may be no active lock owner even though the count isn't zero. If there is one or more owners, the count will always be non-zero. > > Obviously, for an assert, we're OK with it reporting that the lock is > held when actually it's not. The caller is expected to hold the lock, > so failing to trip the assert when the caller doesn't hold the lock > isn't great, but we can live with it. OTOH, if the assert fires when > the caller does hold the lock, that is not tolerable. The second case shouldn't happen. So the assert should be OK. Cheers, Longman
diff --git a/include/linux/rwbase_rt.h b/include/linux/rwbase_rt.h index 1d264dd08625..a04acd85705b 100644 --- a/include/linux/rwbase_rt.h +++ b/include/linux/rwbase_rt.h @@ -26,12 +26,17 @@ struct rwbase_rt { } while (0) -static __always_inline bool rw_base_is_locked(struct rwbase_rt *rwb) +static __always_inline bool rw_base_is_locked(const struct rwbase_rt *rwb) { return atomic_read(&rwb->readers) != READER_BIAS; } -static __always_inline bool rw_base_is_contended(struct rwbase_rt *rwb) +static inline void rw_base_assert_held_write(const struct rwbase_rt *rwb) +{ + BUG_ON(atomic_read(&rwb->readers) != WRITER_BIAS); +} + +static __always_inline bool rw_base_is_contended(const struct rwbase_rt *rwb) { return atomic_read(&rwb->readers) > 0; } diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h index 1dd530ce8b45..b5b34cca86f3 100644 --- a/include/linux/rwsem.h +++ b/include/linux/rwsem.h @@ -66,14 +66,24 @@ struct rw_semaphore { #endif }; -/* In all implementations count != 0 means locked */ +#define RWSEM_UNLOCKED_VALUE 0UL +#define RWSEM_WRITER_LOCKED (1UL << 0) +#define __RWSEM_COUNT_INIT(name) .count = ATOMIC_LONG_INIT(RWSEM_UNLOCKED_VALUE) + static inline int rwsem_is_locked(struct rw_semaphore *sem) { - return atomic_long_read(&sem->count) != 0; + return atomic_long_read(&sem->count) != RWSEM_UNLOCKED_VALUE; } -#define RWSEM_UNLOCKED_VALUE 0L -#define __RWSEM_COUNT_INIT(name) .count = ATOMIC_LONG_INIT(RWSEM_UNLOCKED_VALUE) +static inline void rwsem_assert_held_nolockdep(const struct rw_semaphore *sem) +{ + WARN_ON(atomic_long_read(&sem->count) == RWSEM_UNLOCKED_VALUE); +} + +static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem) +{ + WARN_ON(!(atomic_long_read(&sem->count) & RWSEM_WRITER_LOCKED)); +} /* Common initializer macros and functions */ @@ -152,11 +162,21 @@ do { \ __init_rwsem((sem), #sem, &__key); \ } while (0) -static __always_inline int rwsem_is_locked(struct rw_semaphore *sem) +static __always_inline int rwsem_is_locked(const struct rw_semaphore *sem) { return rw_base_is_locked(&sem->rwbase); } +static inline void rwsem_assert_held_nolockdep(const struct rw_semaphore *sem) +{ + BUG_ON(!rwsem_is_locked(sem)); +} + +static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem) +{ + rw_base_assert_held_write(sem); +} + static __always_inline int rwsem_is_contended(struct rw_semaphore *sem) { return rw_base_is_contended(&sem->rwbase); @@ -169,6 +189,22 @@ static __always_inline int rwsem_is_contended(struct rw_semaphore *sem) * the RT specific variant. */ +static inline void rwsem_assert_held(const struct rw_semaphore *sem) +{ + if (IS_ENABLED(CONFIG_LOCKDEP)) + lockdep_assert_held(sem); + else + rwsem_assert_held_nolockdep(sem); +} + +static inline void rwsem_assert_held_write(const struct rw_semaphore *sem) +{ + if (IS_ENABLED(CONFIG_LOCKDEP)) + lockdep_assert_held_write(sem); + else + rwsem_assert_held_write_nolockdep(sem); +} + /* * lock for reading */
Modelled after lockdep_assert_held() and lockdep_assert_held_write(), but are always active, even when lockdep is disabled. Of course, they don't test that _this_ thread is the owner, but it's sufficient to catch many bugs and doesn't incur the same performance penalty as lockdep. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- include/linux/rwbase_rt.h | 9 ++++++-- include/linux/rwsem.h | 46 ++++++++++++++++++++++++++++++++++----- 2 files changed, 48 insertions(+), 7 deletions(-)