The commit cited below broke the build for PREEMPT_RT because
rwsem_assert_held_write_nolockdep() passes a struct rw_semaphore but
rw_base_assert_held_write() expects struct rwbase_rt. Fixing the type
alone leads to the problem that WARN_ON() is not found because bug.h is
missing.
In order to resolve this:
- Keep the assert (WARN_ON()) in rwsem.h (not rwbase_rt.h)
- Add the rwsem_held_write() which returns true if the lock is already
write-locked and no reader are around.
- Make rw_base_is_write_locked() do the implementation specific
(rw_base) writer check.
- Replace the "inline" with __always_inline which was used before.
Fixes: f70405afc99b1 ("locking: Add rwsem_assert_held() and rwsem_assert_held_write()")
Reported-by: Clark Williams <[email protected]>
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
include/linux/rwbase_rt.h | 4 ++--
include/linux/rwsem.h | 11 ++++++++---
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/include/linux/rwbase_rt.h b/include/linux/rwbase_rt.h
index 29c4e4f243e47..f2394a409c9d5 100644
--- a/include/linux/rwbase_rt.h
+++ b/include/linux/rwbase_rt.h
@@ -31,9 +31,9 @@ static __always_inline bool rw_base_is_locked(const struct rwbase_rt *rwb)
return atomic_read(&rwb->readers) != READER_BIAS;
}
-static inline void rw_base_assert_held_write(const struct rwbase_rt *rwb)
+static __always_inline bool rw_base_is_write_locked(const struct rwbase_rt *rwb)
{
- WARN_ON(atomic_read(&rwb->readers) != WRITER_BIAS);
+ return atomic_read(&rwb->readers) == WRITER_BIAS;
}
static __always_inline bool rw_base_is_contended(const struct rwbase_rt *rwb)
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 4f1c18992f768..c81630d81018d 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -167,14 +167,19 @@ 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)
+static __always_inline void rwsem_assert_held_nolockdep(const struct rw_semaphore *sem)
{
WARN_ON(!rwsem_is_locked(sem));
}
-static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
+static __always_inline bool rwsem_held_write(const struct rw_semaphore *sem)
{
- rw_base_assert_held_write(sem);
+ return rw_base_is_write_locked(&sem->rwbase);
+}
+
+static __always_inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
+{
+ WARN_ON(!rwsem_held_write(sem));
}
static __always_inline int rwsem_is_contended(struct rw_semaphore *sem)
--
2.43.0
On Tue, Mar 19, 2024 at 08:05:50AM +0100, Sebastian Andrzej Siewior wrote:
> -static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
> +static __always_inline bool rwsem_held_write(const struct rw_semaphore *sem)
The locking maintainers were very clear that this predicate Should Not
Exist. It encourages people to write bad code. Assertions only!
> {
> - rw_base_assert_held_write(sem);
> + return rw_base_is_write_locked(&sem->rwbase);
> +}
> +
> +static __always_inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
> +{
> + WARN_ON(!rwsem_held_write(sem));
> }
>
> static __always_inline int rwsem_is_contended(struct rw_semaphore *sem)
> --
> 2.43.0
>
On 2024-03-19 13:38:06 [+0000], Matthew Wilcox wrote:
> On Tue, Mar 19, 2024 at 08:05:50AM +0100, Sebastian Andrzej Siewior wrote:
> > -static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
> > +static __always_inline bool rwsem_held_write(const struct rw_semaphore *sem)
>
> The locking maintainers were very clear that this predicate Should Not
> Exist. It encourages people to write bad code. Assertions only!
What do you refer to? The inline vs __always_inline or
rwsem_held_write() should not exists and it should invoke directly
rw_base_is_write_locked()?
> > {
> > - rw_base_assert_held_write(sem);
> > + return rw_base_is_write_locked(&sem->rwbase);
> > +}
> > +
> > +static __always_inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
> > +{
> > + WARN_ON(!rwsem_held_write(sem));
> > }
> >
> > static __always_inline int rwsem_is_contended(struct rw_semaphore *sem)
Sebastian
On 3/19/24 10:15, Sebastian Andrzej Siewior wrote:
> On 2024-03-19 13:38:06 [+0000], Matthew Wilcox wrote:
>> On Tue, Mar 19, 2024 at 08:05:50AM +0100, Sebastian Andrzej Siewior wrote:
>>> -static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
>>> +static __always_inline bool rwsem_held_write(const struct rw_semaphore *sem)
>> The locking maintainers were very clear that this predicate Should Not
>> Exist. It encourages people to write bad code. Assertions only!
> What do you refer to? The inline vs __always_inline or
> rwsem_held_write() should not exists and it should invoke directly
> rw_base_is_write_locked()?
Just merge rwsem_held_write() into rwsem_assert_held_write_nolockdep()
and we should be all set.
Cheers,
Longman
>>> {
>>> - rw_base_assert_held_write(sem);
>>> + return rw_base_is_write_locked(&sem->rwbase);
>>> +}
>>> +
>>> +static __always_inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
>>> +{
>>> + WARN_ON(!rwsem_held_write(sem));
>>> }
>>>
>>> static __always_inline int rwsem_is_contended(struct rw_semaphore *sem)
> Sebastian
>
On 2024-03-19 16:01:26 [+0000], Matthew Wilcox wrote:
> I see Waiman already gave you the substantial answer ... but why did you
> change inline to __always_inline?
To align with the all the other functions in the file which were defined
like that.
Sebastian
The commit cited below broke the build for PREEMPT_RT because
rwsem_assert_held_write_nolockdep() passes a struct rw_semaphore but
rw_base_assert_held_write() expects struct rwbase_rt. Fixing the type
alone leads to the problem that WARN_ON() is not found because bug.h is
missing.
In order to resolve this:
- Keep the assert (WARN_ON()) in rwsem.h (not rwbase_rt.h)
- Make rwsem_assert_held_write_nolockdep() do the implementation
specific (rw_base) writer check.
- Replace the "inline" with __always_inline which was used before.
Fixes: f70405afc99b1 ("locking: Add rwsem_assert_held() and rwsem_assert_held_write()")
Reported-by: Clark Williams <[email protected]>
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
v1…v2:
- Merge rwsem_held_write() into rwsem_assert_held_write_nolockdep() as
per Waiman Long / Matthew Wilcox.
include/linux/rwbase_rt.h | 4 ++--
include/linux/rwsem.h | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/linux/rwbase_rt.h b/include/linux/rwbase_rt.h
index 29c4e4f243e47..f2394a409c9d5 100644
--- a/include/linux/rwbase_rt.h
+++ b/include/linux/rwbase_rt.h
@@ -31,9 +31,9 @@ static __always_inline bool rw_base_is_locked(const struct rwbase_rt *rwb)
return atomic_read(&rwb->readers) != READER_BIAS;
}
-static inline void rw_base_assert_held_write(const struct rwbase_rt *rwb)
+static __always_inline bool rw_base_is_write_locked(const struct rwbase_rt *rwb)
{
- WARN_ON(atomic_read(&rwb->readers) != WRITER_BIAS);
+ return atomic_read(&rwb->readers) == WRITER_BIAS;
}
static __always_inline bool rw_base_is_contended(const struct rwbase_rt *rwb)
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 4f1c18992f768..c8b543d428b0a 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -167,14 +167,14 @@ 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)
+static __always_inline void rwsem_assert_held_nolockdep(const struct rw_semaphore *sem)
{
WARN_ON(!rwsem_is_locked(sem));
}
-static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
+static __always_inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
{
- rw_base_assert_held_write(sem);
+ WARN_ON(!rw_base_is_write_locked(&sem->rwbase));
}
static __always_inline int rwsem_is_contended(struct rw_semaphore *sem)
--
2.43.0
On Tue, Mar 19, 2024 at 03:15:06PM +0100, Sebastian Andrzej Siewior wrote:
> On 2024-03-19 13:38:06 [+0000], Matthew Wilcox wrote:
> > On Tue, Mar 19, 2024 at 08:05:50AM +0100, Sebastian Andrzej Siewior wrote:
> > > -static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
> > > +static __always_inline bool rwsem_held_write(const struct rw_semaphore *sem)
> >
> > The locking maintainers were very clear that this predicate Should Not
> > Exist. It encourages people to write bad code. Assertions only!
>
> What do you refer to? The inline vs __always_inline or
> rwsem_held_write() should not exists and it should invoke directly
> rw_base_is_write_locked()?
I see Waiman already gave you the substantial answer ... but why did you
change inline to __always_inline?
On 3/19/24 14:20, Sebastian Andrzej Siewior wrote:
> The commit cited below broke the build for PREEMPT_RT because
> rwsem_assert_held_write_nolockdep() passes a struct rw_semaphore but
> rw_base_assert_held_write() expects struct rwbase_rt. Fixing the type
> alone leads to the problem that WARN_ON() is not found because bug.h is
> missing.
>
> In order to resolve this:
> - Keep the assert (WARN_ON()) in rwsem.h (not rwbase_rt.h)
> - Make rwsem_assert_held_write_nolockdep() do the implementation
> specific (rw_base) writer check.
> - Replace the "inline" with __always_inline which was used before.
>
> Fixes: f70405afc99b1 ("locking: Add rwsem_assert_held() and rwsem_assert_held_write()")
> Reported-by: Clark Williams <[email protected]>
> Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
> ---
> v1…v2:
> - Merge rwsem_held_write() into rwsem_assert_held_write_nolockdep() as
> per Waiman Long / Matthew Wilcox.
>
> include/linux/rwbase_rt.h | 4 ++--
> include/linux/rwsem.h | 6 +++---
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/rwbase_rt.h b/include/linux/rwbase_rt.h
> index 29c4e4f243e47..f2394a409c9d5 100644
> --- a/include/linux/rwbase_rt.h
> +++ b/include/linux/rwbase_rt.h
> @@ -31,9 +31,9 @@ static __always_inline bool rw_base_is_locked(const struct rwbase_rt *rwb)
> return atomic_read(&rwb->readers) != READER_BIAS;
> }
>
> -static inline void rw_base_assert_held_write(const struct rwbase_rt *rwb)
> +static __always_inline bool rw_base_is_write_locked(const struct rwbase_rt *rwb)
> {
> - WARN_ON(atomic_read(&rwb->readers) != WRITER_BIAS);
> + return atomic_read(&rwb->readers) == WRITER_BIAS;
> }
>
> static __always_inline bool rw_base_is_contended(const struct rwbase_rt *rwb)
> diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
> index 4f1c18992f768..c8b543d428b0a 100644
> --- a/include/linux/rwsem.h
> +++ b/include/linux/rwsem.h
> @@ -167,14 +167,14 @@ 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)
> +static __always_inline void rwsem_assert_held_nolockdep(const struct rw_semaphore *sem)
> {
> WARN_ON(!rwsem_is_locked(sem));
> }
>
> -static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
> +static __always_inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
> {
> - rw_base_assert_held_write(sem);
> + WARN_ON(!rw_base_is_write_locked(&sem->rwbase));
> }
>
> static __always_inline int rwsem_is_contended(struct rw_semaphore *sem)
Reviewed-by: Waiman Long <[email protected]>
The following commit has been merged into the locking/urgent branch of tip:
Commit-ID: fa1f51162338b3e2f520d4bfedc42b3b2e00da6d
Gitweb: https://git.kernel.org/tip/fa1f51162338b3e2f520d4bfedc42b3b2e00da6d
Author: Sebastian Andrzej Siewior <[email protected]>
AuthorDate: Tue, 19 Mar 2024 19:20:50 +01:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Mon, 08 Apr 2024 16:39:16 +02:00
locking: Make rwsem_assert_held_write_nolockdep() build with PREEMPT_RT=y
The commit cited below broke the build for PREEMPT_RT because
rwsem_assert_held_write_nolockdep() passes a struct rw_semaphore but
rw_base_assert_held_write() expects struct rwbase_rt. Fixing the type alone
leads to the problem that WARN_ON() is not found because bug.h is missing.
In order to resolve this:
- Keep the assert (WARN_ON()) in rwsem.h (not rwbase_rt.h)
- Make rwsem_assert_held_write_nolockdep() do the implementation
specific (rw_base) writer check.
- Replace the "inline" with __always_inline which was used before.
Fixes: f70405afc99b1 ("locking: Add rwsem_assert_held() and rwsem_assert_held_write()")
Reported-by: Clark Williams <[email protected]>
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Waiman Long <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
include/linux/rwbase_rt.h | 4 ++--
include/linux/rwsem.h | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/linux/rwbase_rt.h b/include/linux/rwbase_rt.h
index 29c4e4f..f2394a4 100644
--- a/include/linux/rwbase_rt.h
+++ b/include/linux/rwbase_rt.h
@@ -31,9 +31,9 @@ static __always_inline bool rw_base_is_locked(const struct rwbase_rt *rwb)
return atomic_read(&rwb->readers) != READER_BIAS;
}
-static inline void rw_base_assert_held_write(const struct rwbase_rt *rwb)
+static __always_inline bool rw_base_is_write_locked(const struct rwbase_rt *rwb)
{
- WARN_ON(atomic_read(&rwb->readers) != WRITER_BIAS);
+ return atomic_read(&rwb->readers) == WRITER_BIAS;
}
static __always_inline bool rw_base_is_contended(const struct rwbase_rt *rwb)
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 4f1c189..c8b543d 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -167,14 +167,14 @@ 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)
+static __always_inline void rwsem_assert_held_nolockdep(const struct rw_semaphore *sem)
{
WARN_ON(!rwsem_is_locked(sem));
}
-static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
+static __always_inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
{
- rw_base_assert_held_write(sem);
+ WARN_ON(!rw_base_is_write_locked(&sem->rwbase));
}
static __always_inline int rwsem_is_contended(struct rw_semaphore *sem)