2021-09-01 18:35:28

by Waiman Long

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

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 <[email protected]>
> ---
>
> 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

2021-09-03 10:53:18

by Sebastian Andrzej Siewior

Subject: [PATCH v2] 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 <[email protected]>
---
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;

2021-09-08 02:38:53

by Boqun Feng

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

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 <[email protected]>
> ---
> 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 <[email protected]>

Regards,
Boqun

> return LOCK_STATE_HELD;
>
> return LOCK_STATE_NOT_HELD;

2021-09-08 12:23:06

by Peter Zijlstra

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

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 <[email protected]>
> > ---
> > 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 <[email protected]>

Thanks!

2021-09-08 14:36:59

by Waiman Long

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

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 <[email protected]>
> ---
> 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 <[email protected]>

2021-09-08 14:46:02

by Waiman Long

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

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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
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

2021-09-10 13:56:39

by Sebastian Andrzej Siewior

Subject: [PATCH v3] 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 <[email protected]>
Acked-by: Waiman Long <[email protected]>
Acked-by: Boqun Feng <[email protected]>
---
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

2021-09-17 22:04:07

by tip-bot2 for Jacob Pan

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

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 <[email protected]>
AuthorDate: Fri, 03 Sep 2021 10:40:01 +02:00
Committer: Peter Zijlstra <[email protected]>
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 <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Acked-by: Boqun Feng <[email protected]>
Acked-by: Waiman Long <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
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;