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

[Boqun Feng <boqun.feng@gmail.com>]

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
>