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@xxxxxxxxxxxxx/

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