linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read
@ 2021-09-01 16:22 Sebastian Andrzej Siewior
  2021-09-01 16:43 ` Boqun Feng
  2021-09-01 18:32 ` Waiman Long
  0 siblings, 2 replies; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-09-01 16:22 UTC (permalink / raw)
  To: linux-kernel
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
	Boqun Feng, Thomas Gleixner

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read
  2021-09-01 16:22 [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read Sebastian Andrzej Siewior
@ 2021-09-01 16:43 ` Boqun Feng
  2021-09-03 10:45   ` Sebastian Andrzej Siewior
  2021-09-01 18:32 ` Waiman Long
  1 sibling, 1 reply; 13+ messages in thread
From: Boqun Feng @ 2021-09-01 16:43 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Thomas Gleixner

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
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read
  2021-09-01 16:22 [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read Sebastian Andrzej Siewior
  2021-09-01 16:43 ` Boqun Feng
@ 2021-09-01 18:32 ` Waiman Long
  2021-09-03  8:40   ` [PATCH v2] " Sebastian Andrzej Siewior
  1 sibling, 1 reply; 13+ messages in thread
From: Waiman Long @ 2021-09-01 18:32 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, linux-kernel
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng, Thomas Gleixner

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2] lockdep: Let lock_is_held_type() detect recursive read as read
  2021-09-01 18:32 ` Waiman Long
@ 2021-09-03  8:40   ` Sebastian Andrzej Siewior
  2021-09-08  2:16     ` Boqun Feng
                       ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-09-03  8:40 UTC (permalink / raw)
  To: Waiman Long
  Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Boqun Feng, Thomas Gleixner

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;

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read
  2021-09-01 16:43 ` Boqun Feng
@ 2021-09-03 10:45   ` Sebastian Andrzej Siewior
  2021-09-03 14:15     ` Boqun Feng
  0 siblings, 1 reply; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-09-03 10:45 UTC (permalink / raw)
  To: Boqun Feng
  Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Thomas Gleixner

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read
  2021-09-03 10:45   ` Sebastian Andrzej Siewior
@ 2021-09-03 14:15     ` Boqun Feng
  2021-09-03 14:30       ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 13+ messages in thread
From: Boqun Feng @ 2021-09-03 14:15 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Thomas Gleixner

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read
  2021-09-03 14:15     ` Boqun Feng
@ 2021-09-03 14:30       ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-09-03 14:30 UTC (permalink / raw)
  To: Boqun Feng
  Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Waiman Long, Thomas Gleixner

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] lockdep: Let lock_is_held_type() detect recursive read as read
  2021-09-03  8:40   ` [PATCH v2] " Sebastian Andrzej Siewior
@ 2021-09-08  2:16     ` Boqun Feng
  2021-09-08 10:17       ` Peter Zijlstra
  2021-09-08 14:40       ` [PATCH v2] " Waiman Long
  2021-09-08 14:35     ` Waiman Long
  2021-09-17 13:17     ` [tip: locking/core] " tip-bot2 for Sebastian Andrzej Siewior
  2 siblings, 2 replies; 13+ messages in thread
From: Boqun Feng @ 2021-09-08  2:16 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Waiman Long, linux-kernel, Peter Zijlstra, Ingo Molnar,
	Will Deacon, Thomas Gleixner

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;

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] lockdep: Let lock_is_held_type() detect recursive read as read
  2021-09-08  2:16     ` Boqun Feng
@ 2021-09-08 10:17       ` Peter Zijlstra
  2021-09-10 13:53         ` [PATCH v3] " Sebastian Andrzej Siewior
  2021-09-08 14:40       ` [PATCH v2] " Waiman Long
  1 sibling, 1 reply; 13+ messages in thread
From: Peter Zijlstra @ 2021-09-08 10:17 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Sebastian Andrzej Siewior, Waiman Long, linux-kernel,
	Ingo Molnar, Will Deacon, Thomas Gleixner

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!

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] lockdep: Let lock_is_held_type() detect recursive read as read
  2021-09-03  8:40   ` [PATCH v2] " Sebastian Andrzej Siewior
  2021-09-08  2:16     ` Boqun Feng
@ 2021-09-08 14:35     ` Waiman Long
  2021-09-17 13:17     ` [tip: locking/core] " tip-bot2 for Sebastian Andrzej Siewior
  2 siblings, 0 replies; 13+ messages in thread
From: Waiman Long @ 2021-09-08 14:35 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, Waiman Long
  Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Boqun Feng, Thomas Gleixner

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>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] lockdep: Let lock_is_held_type() detect recursive read as read
  2021-09-08  2:16     ` Boqun Feng
  2021-09-08 10:17       ` Peter Zijlstra
@ 2021-09-08 14:40       ` Waiman Long
  1 sibling, 0 replies; 13+ messages in thread
From: Waiman Long @ 2021-09-08 14:40 UTC (permalink / raw)
  To: Boqun Feng, Sebastian Andrzej Siewior
  Cc: Waiman Long, linux-kernel, Peter Zijlstra, Ingo Molnar,
	Will Deacon, Thomas Gleixner

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3] lockdep: Let lock_is_held_type() detect recursive read as read
  2021-09-08 10:17       ` Peter Zijlstra
@ 2021-09-10 13:53         ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-09-10 13:53 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Boqun Feng, Waiman Long, linux-kernel, Ingo Molnar, Will Deacon,
	Thomas Gleixner

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [tip: locking/core] lockdep: Let lock_is_held_type() detect recursive read as read
  2021-09-03  8:40   ` [PATCH v2] " Sebastian Andrzej Siewior
  2021-09-08  2:16     ` Boqun Feng
  2021-09-08 14:35     ` Waiman Long
@ 2021-09-17 13:17     ` tip-bot2 for Sebastian Andrzej Siewior
  2 siblings, 0 replies; 13+ messages in thread
From: tip-bot2 for Sebastian Andrzej Siewior @ 2021-09-17 13:17 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Sebastian Andrzej Siewior, Peter Zijlstra (Intel),
	Boqun Feng, Waiman Long, x86, linux-kernel

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;

^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-09-17 13:17 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-01 16:22 [PATCH] lockdep: Let lock_is_held_type() detect recursive read as read Sebastian Andrzej Siewior
2021-09-01 16:43 ` Boqun Feng
2021-09-03 10:45   ` Sebastian Andrzej Siewior
2021-09-03 14:15     ` Boqun Feng
2021-09-03 14:30       ` Sebastian Andrzej Siewior
2021-09-01 18:32 ` Waiman Long
2021-09-03  8:40   ` [PATCH v2] " Sebastian Andrzej Siewior
2021-09-08  2:16     ` Boqun Feng
2021-09-08 10:17       ` Peter Zijlstra
2021-09-10 13:53         ` [PATCH v3] " Sebastian Andrzej Siewior
2021-09-08 14:40       ` [PATCH v2] " Waiman Long
2021-09-08 14:35     ` Waiman Long
2021-09-17 13:17     ` [tip: locking/core] " tip-bot2 for Sebastian Andrzej Siewior

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).