[PATCH] lockdep: Let lock_is_held_type() detect recursive read as read

[PATCH] lockdep: Let lock_is_held_type() detect recursive read as read

[Sebastian Andrzej Siewior]

lock_is_held_type(, 1) detects acquired read locks. It only recognized
locks acquired with lock_acquire_shared(). Read locks acquired with
lock_acquire_shared_recursive() are not recognized because a `2' is
stored as the read value.

Rework the check to additionally recognise lock's read value one and two
as a read held lock.

Fixes: e918188611f07 ("locking: More accurate annotations for read_lock()")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

On a related note: What exactly means read_lock_is_recursive() in terms
of recursive locking? The second items mentions QRW locks. Does this
mean that a pending WRITER blocks further READER from acquiring the
lock?

 kernel/locking/lockdep.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index f15df3fd7c5a6..39f98454a8827 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -5366,7 +5366,9 @@ int __lock_is_held(const struct lockdep_map *lock, int read)
 		struct held_lock *hlock = curr->held_locks + i;
 
 		if (match_held_lock(hlock, lock)) {
-			if (read == -1 || hlock->read == read)
+			if (read == -1 ||
+			    (read == 0 && hlock->read == 0) ||
+			    (read == 1 && hlock->read > 0))
 				return LOCK_STATE_HELD;
 
 			return LOCK_STATE_NOT_HELD;
-- 
2.33.0

Re: [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read

[Boqun Feng]

On Wed, Sep 01, 2021 at 06:22:55PM +0200, Sebastian Andrzej Siewior wrote:
> lock_is_held_type(, 1) detects acquired read locks. It only recognized
> locks acquired with lock_acquire_shared(). Read locks acquired with
> lock_acquire_shared_recursive() are not recognized because a `2' is
> stored as the read value.
> 
> Rework the check to additionally recognise lock's read value one and two
> as a read held lock.
> 
> Fixes: e918188611f07 ("locking: More accurate annotations for read_lock()")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> 
> On a related note: What exactly means read_lock_is_recursive() in terms
> of recursive locking? The second items mentions QRW locks. Does this
> mean that a pending WRITER blocks further READER from acquiring the
> lock?
> 

If a reader is recursive, then a pending writer doesn't block the
recursive reader, otherwise, a pending write blocks the reader. IOW, a
pending writer blocks non-recursive readers but not recursive readers.

In most case for a lock, all the readers are either recursive or
non-recursive, but queued rwlock is a little different, readers in the
interrupt are recursive while readers in normal context are not. So
lockdep needs to handle them differently.

However, one special case is that in the selftest cases for lockdep, we
want to make all read_lock() recursive (for the purpose of testing),
that's why read_lock_is_recursive() has a force_read_lock_recursive bit.

Does the above make things answer your question?

Regards,
Boqun

>  kernel/locking/lockdep.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index f15df3fd7c5a6..39f98454a8827 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -5366,7 +5366,9 @@ int __lock_is_held(const struct lockdep_map *lock, int read)
>  		struct held_lock *hlock = curr->held_locks + i;
>  
>  		if (match_held_lock(hlock, lock)) {
> -			if (read == -1 || hlock->read == read)
> +			if (read == -1 ||
> +			    (read == 0 && hlock->read == 0) ||
> +			    (read == 1 && hlock->read > 0))
>  				return LOCK_STATE_HELD;
>  
>  			return LOCK_STATE_NOT_HELD;
> -- 
> 2.33.0
> 

Re: [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read

[Sebastian Andrzej Siewior]

On 2021-09-02 00:43:45 [+0800], Boqun Feng wrote:
> If a reader is recursive, then a pending writer doesn't block the
> recursive reader, otherwise, a pending write blocks the reader. IOW, a
> pending writer blocks non-recursive readers but not recursive readers.

Puh. So I would describe it as writer fair but maybe I'm not fluent in
locking. But you don't mean recursive reader as in 

   T1			T2
   read_lock(a);
			write_lock(a);
   read_lock(a);

which results in a deadlock (but T1 recursively acquired the `a' lock). 

However, PREEMPT_RT's locking implementation always blocks further
reader from entering locked section once a writer is pending so that
would then ask for something like this:

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -5572,16 +5572,19 @@ static bool lockdep_nmi(void)
 }
 
 /*
- * read_lock() is recursive if:
- * 1. We force lockdep think this way in selftests or
- * 2. The implementation is not queued read/write lock or
- * 3. The locker is at an in_interrupt() context.
+ * read_lock() is recursive if the implementation allows readers to enter the
+ * locked section even if a writer is pending. This is case if:
+ * - We force lockdep think this way in selftests
+ * - The implementation is queued read/write lock and the locker is in
+ *   in_interrupt() context.
+ * - Non queued read/write implementation allow it unconditionally.
+ * - PREEMPT_RT's implementation never allows it.
  */
 bool read_lock_is_recursive(void)
 {
 	return force_read_lock_recursive ||
-	       !IS_ENABLED(CONFIG_QUEUED_RWLOCKS) ||
-	       in_interrupt();
+	       (IS_ENABLED(CONFIG_QUEUED_RWLOCKS) && in_interrupt()) ||
+	       !IS_ENABLED(CONFIG_PREEMPT_RT);
 }
 EXPORT_SYMBOL_GPL(read_lock_is_recursive);
 
Sebastian

Re: [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read

[Boqun Feng]

On Fri, Sep 03, 2021 at 12:45:57PM +0200, Sebastian Andrzej Siewior wrote:
> On 2021-09-02 00:43:45 [+0800], Boqun Feng wrote:
> > If a reader is recursive, then a pending writer doesn't block the
> > recursive reader, otherwise, a pending write blocks the reader. IOW, a
> > pending writer blocks non-recursive readers but not recursive readers.
> 
> Puh. So I would describe it as writer fair but maybe I'm not fluent in
> locking. But you don't mean recursive reader as in 
> 
>    T1			T2
>    read_lock(a);
> 			write_lock(a);
>    read_lock(a);
> 
> which results in a deadlock (but T1 recursively acquired the `a' lock). 
> 
> However, PREEMPT_RT's locking implementation always blocks further
> reader from entering locked section once a writer is pending so that
> would then ask for something like this:
> 

But the rwlock in PREEMPT_RT is writer unfair, isn't it? As per:

	https://lore.kernel.org/lkml/20210815211302.957920571@linutronix.de/

also in __rwbase_read_lock():

	/*
	 * Allow readers, as long as the writer has not completely
	 * acquired the semaphore for write.
	 */
	if (atomic_read(&rwb->readers) != WRITER_BIAS) {
		atomic_inc(&rwb->readers);
		raw_spin_unlock_irq(&rtm->wait_lock);
		return 0;
	}

that means the readers of rwlock in PREEMPT_RT are always recursive,
right? Am I missing something subtle?

Regards,
Boqun

> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -5572,16 +5572,19 @@ static bool lockdep_nmi(void)
>  }
>  
>  /*
> - * read_lock() is recursive if:
> - * 1. We force lockdep think this way in selftests or
> - * 2. The implementation is not queued read/write lock or
> - * 3. The locker is at an in_interrupt() context.
> + * read_lock() is recursive if the implementation allows readers to enter the
> + * locked section even if a writer is pending. This is case if:
> + * - We force lockdep think this way in selftests
> + * - The implementation is queued read/write lock and the locker is in
> + *   in_interrupt() context.
> + * - Non queued read/write implementation allow it unconditionally.
> + * - PREEMPT_RT's implementation never allows it.
>   */
>  bool read_lock_is_recursive(void)
>  {
>  	return force_read_lock_recursive ||
> -	       !IS_ENABLED(CONFIG_QUEUED_RWLOCKS) ||
> -	       in_interrupt();
> +	       (IS_ENABLED(CONFIG_QUEUED_RWLOCKS) && in_interrupt()) ||
> +	       !IS_ENABLED(CONFIG_PREEMPT_RT);
>  }
>  EXPORT_SYMBOL_GPL(read_lock_is_recursive);
>  
> Sebastian

Re: [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read

[Sebastian Andrzej Siewior]

On 2021-09-03 22:15:04 [+0800], Boqun Feng wrote:
> But the rwlock in PREEMPT_RT is writer unfair, isn't it? As per:
> 
> 	https://lore.kernel.org/lkml/20210815211302.957920571@linutronix.de/
> 
> also in __rwbase_read_lock():
> 
> 	/*
> 	 * Allow readers, as long as the writer has not completely
> 	 * acquired the semaphore for write.
> 	 */
> 	if (atomic_read(&rwb->readers) != WRITER_BIAS) {
> 		atomic_inc(&rwb->readers);
> 		raw_spin_unlock_irq(&rtm->wait_lock);
> 		return 0;
> 	}
> 
> that means the readers of rwlock in PREEMPT_RT are always recursive,
> right? Am I missing something subtle?

huch. You are right. I take everything backt then. I do remember it
differently but it has been like since this implementation has been
introduced…

> Regards,
> Boqun

Sebastian

Re: [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read

[Waiman Long]

On 9/1/21 12:22 PM, Sebastian Andrzej Siewior wrote:
> lock_is_held_type(, 1) detects acquired read locks. It only recognized
> locks acquired with lock_acquire_shared(). Read locks acquired with
> lock_acquire_shared_recursive() are not recognized because a `2' is
> stored as the read value.
>
> Rework the check to additionally recognise lock's read value one and two
> as a read held lock.
>
> Fixes: e918188611f07 ("locking: More accurate annotations for read_lock()")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>
> On a related note: What exactly means read_lock_is_recursive() in terms
> of recursive locking? The second items mentions QRW locks. Does this
> mean that a pending WRITER blocks further READER from acquiring the
> lock?
>
>   kernel/locking/lockdep.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index f15df3fd7c5a6..39f98454a8827 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -5366,7 +5366,9 @@ int __lock_is_held(const struct lockdep_map *lock, int read)
>   		struct held_lock *hlock = curr->held_locks + i;
>   
>   		if (match_held_lock(hlock, lock)) {
> -			if (read == -1 || hlock->read == read)
> +			if (read == -1 ||
> +			    (read == 0 && hlock->read == 0) ||
> +			    (read == 1 && hlock->read > 0))
>   				return LOCK_STATE_HELD;
>   
>   			return LOCK_STATE_NOT_HELD;

I think the check can be simplified as

     if (read == -1 || read == !!hlock->read)

Cheers,
Longman

[PATCH v2] lockdep: Let lock_is_held_type() detect recursive read as read

[Sebastian Andrzej Siewior]

lock_is_held_type(, 1) detects acquired read locks. It only recognized
locks acquired with lock_acquire_shared(). Read locks acquired with
lock_acquire_shared_recursive() are not recognized because a `2' is
stored as the read value.

Rework the check to additionally recognise lock's read value one and two
as a read held lock.

Fixes: e918188611f07 ("locking: More accurate annotations for read_lock()")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2:
  - simplify the read check to !!read as suggested by Waiman Long.

 kernel/locking/lockdep.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -5366,7 +5366,7 @@ int __lock_is_held(const struct lockdep_
 		struct held_lock *hlock = curr->held_locks + i;
 
 		if (match_held_lock(hlock, lock)) {
-			if (read == -1 || hlock->read == read)
+			if (read == -1 || hlock->read == !!read)
 				return LOCK_STATE_HELD;
 
 			return LOCK_STATE_NOT_HELD;

Re: [PATCH v2] lockdep: Let lock_is_held_type() detect recursive read as read

[Boqun Feng]

On Fri, Sep 03, 2021 at 10:40:01AM +0200, Sebastian Andrzej Siewior wrote:
> lock_is_held_type(, 1) detects acquired read locks. It only recognized
> locks acquired with lock_acquire_shared(). Read locks acquired with
> lock_acquire_shared_recursive() are not recognized because a `2' is
> stored as the read value.
> 
> Rework the check to additionally recognise lock's read value one and two
> as a read held lock.
> 
> Fixes: e918188611f07 ("locking: More accurate annotations for read_lock()")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> v1…v2:
>   - simplify the read check to !!read as suggested by Waiman Long.
> 
>  kernel/locking/lockdep.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -5366,7 +5366,7 @@ int __lock_is_held(const struct lockdep_
>  		struct held_lock *hlock = curr->held_locks + i;
>  
>  		if (match_held_lock(hlock, lock)) {
> -			if (read == -1 || hlock->read == read)
> +			if (read == -1 || hlock->read == !!read)

I think this should be:

	!!hlock->read == read

With that,

Acked-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

>  				return LOCK_STATE_HELD;
>  
>  			return LOCK_STATE_NOT_HELD;

Re: [PATCH v2] lockdep: Let lock_is_held_type() detect recursive read as read

[Peter Zijlstra]

On Wed, Sep 08, 2021 at 10:16:19AM +0800, Boqun Feng wrote:
> On Fri, Sep 03, 2021 at 10:40:01AM +0200, Sebastian Andrzej Siewior wrote:
> > lock_is_held_type(, 1) detects acquired read locks. It only recognized
> > locks acquired with lock_acquire_shared(). Read locks acquired with
> > lock_acquire_shared_recursive() are not recognized because a `2' is
> > stored as the read value.
> > 
> > Rework the check to additionally recognise lock's read value one and two
> > as a read held lock.
> > 
> > Fixes: e918188611f07 ("locking: More accurate annotations for read_lock()")
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > v1…v2:
> >   - simplify the read check to !!read as suggested by Waiman Long.
> > 
> >  kernel/locking/lockdep.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -5366,7 +5366,7 @@ int __lock_is_held(const struct lockdep_
> >  		struct held_lock *hlock = curr->held_locks + i;
> >  
> >  		if (match_held_lock(hlock, lock)) {
> > -			if (read == -1 || hlock->read == read)
> > +			if (read == -1 || hlock->read == !!read)
> 
> I think this should be:
> 
> 	!!hlock->read == read
> 
> With that,
> 
> Acked-by: Boqun Feng <boqun.feng@gmail.com>

Thanks!

[PATCH v3] lockdep: Let lock_is_held_type() detect recursive read as read

[Sebastian Andrzej Siewior]

lock_is_held_type(, 1) detects acquired read locks. It only recognized
locks acquired with lock_acquire_shared(). Read locks acquired with
lock_acquire_shared_recursive() are not recognized because a `2' is
stored as the read value.

Rework the check to additionally recognise lock's read value one and two
as a read held lock.

Fixes: e918188611f07 ("locking: More accurate annotations for read_lock()")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Waiman Long <longman@redhat.com>
Acked-by: Boqun Feng <boqun.feng@gmail.com>
---
v3: move the !! to the right spot so it actually works.

 kernel/locking/lockdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index bf1c00c881e48..bfa0a347f27c4 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -5366,7 +5366,7 @@ int __lock_is_held(const struct lockdep_map *lock, int read)
 		struct held_lock *hlock = curr->held_locks + i;
 
 		if (match_held_lock(hlock, lock)) {
-			if (read == -1 || hlock->read == read)
+			if (read == -1 || !!hlock->read == read)
 				return LOCK_STATE_HELD;
 
 			return LOCK_STATE_NOT_HELD;
-- 
2.33.0

Re: [PATCH v2] lockdep: Let lock_is_held_type() detect recursive read as read

[Waiman Long]

On 9/7/21 10:16 PM, Boqun Feng wrote:
> On Fri, Sep 03, 2021 at 10:40:01AM +0200, Sebastian Andrzej Siewior wrote:
>> lock_is_held_type(, 1) detects acquired read locks. It only recognized
>> locks acquired with lock_acquire_shared(). Read locks acquired with
>> lock_acquire_shared_recursive() are not recognized because a `2' is
>> stored as the read value.
>>
>> Rework the check to additionally recognise lock's read value one and two
>> as a read held lock.
>>
>> Fixes: e918188611f07 ("locking: More accurate annotations for read_lock()")
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> ---
>> v1…v2:
>>    - simplify the read check to !!read as suggested by Waiman Long.
>>
>>   kernel/locking/lockdep.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> --- a/kernel/locking/lockdep.c
>> +++ b/kernel/locking/lockdep.c
>> @@ -5366,7 +5366,7 @@ int __lock_is_held(const struct lockdep_
>>   		struct held_lock *hlock = curr->held_locks + i;
>>   
>>   		if (match_held_lock(hlock, lock)) {
>> -			if (read == -1 || hlock->read == read)
>> +			if (read == -1 || hlock->read == !!read)
> I think this should be:
>
> 	!!hlock->read == read
>
> With that,
>
> Acked-by: Boqun Feng <boqun.feng@gmail.com>
>
You are right. It should be the other way around. read can only be -1, 
0, 1 while hlock->read can be 0, 1, 2.

Cheers,
Longman

Re: [PATCH v2] lockdep: Let lock_is_held_type() detect recursive read as read

[Waiman Long]

On 9/3/21 4:40 AM, Sebastian Andrzej Siewior wrote:
> lock_is_held_type(, 1) detects acquired read locks. It only recognized
> locks acquired with lock_acquire_shared(). Read locks acquired with
> lock_acquire_shared_recursive() are not recognized because a `2' is
> stored as the read value.
>
> Rework the check to additionally recognise lock's read value one and two
> as a read held lock.
>
> Fixes: e918188611f07 ("locking: More accurate annotations for read_lock()")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> v1…v2:
>    - simplify the read check to !!read as suggested by Waiman Long.
>
>   kernel/locking/lockdep.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -5366,7 +5366,7 @@ int __lock_is_held(const struct lockdep_
>   		struct held_lock *hlock = curr->held_locks + i;
>   
>   		if (match_held_lock(hlock, lock)) {
> -			if (read == -1 || hlock->read == read)
> +			if (read == -1 || hlock->read == !!read)
>   				return LOCK_STATE_HELD;
>   
>   			return LOCK_STATE_NOT_HELD;
>
Thanks for accepting my suggestion.

Acked-by: Waiman Long <longman@redhat.com>

[tip: locking/core] lockdep: Let lock_is_held_type() detect recursive read as read

[tip-bot2 for Sebastian Andrzej Siewior]

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     2507003a1d10917c9158077bf6030719d02c941e
Gitweb:        https://git.kernel.org/tip/2507003a1d10917c9158077bf6030719d02c941e
Author:        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate:    Fri, 03 Sep 2021 10:40:01 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 17 Sep 2021 15:08:44 +02:00

lockdep: Let lock_is_held_type() detect recursive read as read

lock_is_held_type(, 1) detects acquired read locks. It only recognized
locks acquired with lock_acquire_shared(). Read locks acquired with
lock_acquire_shared_recursive() are not recognized because a `2' is
stored as the read value.

Rework the check to additionally recognise lock's read value one and two
as a read held lock.

Fixes: e918188611f07 ("locking: More accurate annotations for read_lock()")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Boqun Feng <boqun.feng@gmail.com>
Acked-by: Waiman Long <longman@redhat.com>
Link: https://lkml.kernel.org/r/20210903084001.lblecrvz4esl4mrr@linutronix.de
---
 kernel/locking/lockdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index bf1c00c..bfa0a34 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -5366,7 +5366,7 @@ int __lock_is_held(const struct lockdep_map *lock, int read)
 		struct held_lock *hlock = curr->held_locks + i;
 
 		if (match_held_lock(hlock, lock)) {
-			if (read == -1 || hlock->read == read)
+			if (read == -1 || !!hlock->read == read)
 				return LOCK_STATE_HELD;
 
 			return LOCK_STATE_NOT_HELD;