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
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;
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;
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!
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]>
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
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
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;