Re: [PATCH printk v2 33/38] printk: introduce console_list_lock

[Paul E. McKenney]

On Fri, Oct 28, 2022 at 11:09:35AM -0700, Boqun Feng wrote:
> On Thu, Oct 27, 2022 at 11:50:07AM -0700, Paul E. McKenney wrote:
> > On Thu, Oct 27, 2022 at 12:09:32PM +0200, Petr Mladek wrote:
> > > Adding Paul into Cc so that he is aware of using a custom SRCU lockdep
> > > check in console_list_lock().
> > 
> > [ . . . ]
> > 
> > > > +/**
> > > > + * console_list_lock - Lock the console list
> > > > + *
> > > > + * For console list or console->flags updates
> > > > + */
> > > > +void console_list_lock(void)
> > > > +{
> > > > +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > > +	/*
> > > > +	 * In unregister_console(), synchronize_srcu() is called with the
> > > > +	 * console_list_lock held. Therefore it is not allowed that the
> > > > +	 * console_list_lock is taken with the srcu_lock held.
> > > > +	 *
> > > > +	 * Whether or not this context is in the read-side critical section
> > > > +	 * can only be detected if the appropriate debug options are enabled.
> > > > +	 */
> > > > +	WARN_ON_ONCE(debug_lockdep_rcu_enabled() &&
> > > > +		     srcu_read_lock_held(&console_srcu));
> > 
> > Yes, this is an interesting case.
> > 
> > The problem that John is facing is that srcu_read_lock_held() believes
> > that it is safer to err on the side of there being an SRCU reader.
> > This is because the standard use is to complain if there is -not-
> > an SRCU reader.  So as soon as there is a lockdep issue anywhere,
> > srcu_read_lock_held() switches to unconditionally returning true.
> > 
> > Which is exactly what John does not want in this case.
> > 
> > So he excludes the CONFIG_DEBUG_LOCK_ALLOC=n case and the
> > !debug_lockdep_rcu_enabled() case, both of which cause
> > srcu_read_lock_held() to unconditionally return true.
> > 
> > This can result in false-positive splats if some other CPU issues a
> > lockdep warning after debug_lockdep_rcu_enabled() is invoked but before
> > srcu_read_lock_held() is invoked.  But similar vulnerabilities are
> > present in RCU_LOCKDEP_WARN(), so unless and until there is a problem,
> > this code should suffice.
> > 
> > One way to save a line is as follows:
> > 
> > 	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_LOCK_ALLOC) &&
> > 		     debug_lockdep_rcu_enabled() &&
> > 		     srcu_read_lock_held(&console_srcu));
> > 
> > Longer term, it might be possible to teach lockdep about this sort of
> > SRCU deadlock.  This is not an issue for vanilla RCU because the RCU
> > reader context prohibits such deadlocks.  This isn't exactly the same
> > as reader-writer locking because this is perfectly OK with SRCU:
> > 
> > 	CPU 0:
> > 		spin_lock(&mylock);
> > 		idx = srcu_read_lock(&mysrcu);
> > 		do_something();
> > 		srcu_read_unlock(&mysrcu, idx);
> > 		spin_unlock(&mylock);
> > 
> > 	CPU 1:
> > 		idx = srcu_read_lock(&mysrcu);
> > 		spin_lock(&mylock);
> > 		do_something();
> > 		spin_unlock(&mylock);
> > 		srcu_read_unlock(&mysrcu, idx);
> > 
> > Adding Boqun on CC in case it is easier than I think.  ;-)
> 
> First I think reader-writer deadlock detection won't treat this as a
> deadlock, because srcu_read_lock() is a recursive read lock ;-) in other
> words, lockdep knows they don't block each other.

Nice!

> I was actually considering to equip SRCU with reader-writer deadlock
> detection when:
> 
> 	https://lore.kernel.org/lkml/20180412021233.ewncg5jjuzjw3x62@tardis/
> 
> The problem (for SRCU to use reader-writer deadlock detection) is
> letting lockdep know synchronize_srcu() doesn't block srcu_read_lock(), 
> so looks like I owe you a new lockdep annotation primitive ;-)

Even better!  ;-)

							Thanx, Paul