Re: [PATCH tip/core/rcu 01/48] rcu: optionally leave lockdep enabled after RCU lockdep splat

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

* Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
> On Wed, May 05, 2010 at 06:46:41PM -0400, Mathieu Desnoyers wrote:
> > * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
> > > From: Lai Jiangshan <laijs@cn.fujitsu.com>
> > > 
> > > There is no need to disable lockdep after an RCU lockdep splat,
> > > so remove the debug_lockdeps_off() from lockdep_rcu_dereference().
> > > To avoid repeated lockdep splats, use a static variable in the inlined
> > > rcu_dereference_check() and rcu_dereference_protected() macros so that
> > > a given instance splats only once, but so that multiple instances can
> > > be detected per boot.
> > > 
> > > This is controlled by a new config variable CONFIG_PROVE_RCU_REPEATEDLY,
> > > which is disabled by default.  This provides the normal lockdep behavior
> > > by default, but permits people who want to find multiple RCU-lockdep
> > > splats per boot to easily do so.
> > 
> > I'll play the devil's advocate here. (just because that's so much fun)
> > ;-)
> > 
> > If we look at:
> > 
> > include/linux/debug_locks.h:
> > 
> > static inline int __debug_locks_off(void)
> > {
> >         return xchg(&debug_locks, 0);
> > }
> > 
> > We see that all code following a call to "debug_locks_off()" can assume
> > that it cannot possibly run concurrently with other code following
> > "debug_locks_off()". Now, I'm not saying that the code we currently have
> > will necessarily break, but I think it is worth asking if there is any
> > assumption in lockdep (or RCU lockdep more specifically) about mutual
> > exclusion after debug_locks_off() ?
> > 
> > Because if there is, then this patch is definitely breaking something by
> > not protecting lockdep against multiple concurrent executions.
> 
> So what in lockdep_rcu_dereference() needs to be protected from concurrent
> execution?  All that happens beyond that point is a bunch of printk()s,
> printing the locks held by this task, and dumping this task's stack.
> 
> 							Thanx, Paul

I agree with you that printing the current task information should be safe.
However, I am not sure that concurrent updates to the lock_class while printk()
is showing its information will end up doing what we expect it to do.

It could be acceptable to have unreliable information in these rare cases, but
the important thing would be to ensure that the kernel does not OOPS.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com