Re: [PATCH 4/4] tracing: Remove RCU work arounds from stack tracer

Paul E. McKenney
2017-09-23T17:16:02Z

On Sat, Sep 23, 2017 at 07:22:04AM -0400, Steven Rostedt wrote:
> On Fri, 22 Sep 2017 23:07:37 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
> > OK, how about the following?
> >
> > It turns out that functions called from rcu_irq_enter() can
> > be subject to various kinds of tracing, which can result in
> > rcu_irq_enter() being invoked recursively. This recursion
> > causes RCU to not have been watching when it should have,
> > resulting in lockdep-RCU splats. Switching from rcu_irq_enter()
> > to rcu_nmi_enter() still resulted in failures because of calls
> > to rcu_irq_enter() between the rcu_nmi_enter() and its matching
> > rcu_nmi_exit(). Such calls again cause RCU to not be watching
> > when it should have been.
> >
> > In particular, the stack tracer called rcu_irq_enter()
> > unconditionally, which is problematic when RCU's last
> > not-watching-to-watching transition was carried out by
> > rcu_nmi_enter(), as will be the case when tracing uses
> > rcu_nmi_enter() to cause RCU to start watching the current CPU.
> > In that case, rcu_irq_enter() actually switches RCU back to
> > the not-watching state for this CPU, which results in lockdep
> > splats complaining about rcu_read_lock() being used on an idle
> > (not-watched) CPU. The first patch of this series addressed
> > this problem by having rcu_irq_enter() and rcu_irq_exit()
> > refrain from doing anything when rcu_nmi_enter() caused RCU to
> > start watching this CPU. The third patch in this series caused
> > save_stack_trace() to invoke rcu_nmi_enter() and rcu_nmi_exit()
> > as needed, so this fourth patch now removes the rcu_irq_enter()
> > and rcu_irq_exit() from within the stack tracer.
>
> I think it's a bit too much ;-) I believe it talks too much about the
> RCU internals, and the bug will be lost on us mere mortals.
>
> >
> > > Actually, thinking about this more, this doesn't need to go in stable.
> > > As recursive rcu_irq_enter() calls should not hurt, and you now allow
> > > rcu_irq_enter() to be called even after a rcu_nmi_enter() right?
> >
> > Yes, it is now the case that rcu_irq_enter() can be called even after
> > an rcu_nmi_enter() exited idle, because rcu_irq_enter() now checks for
> > this and takes an early exit if so.
> >
> > But what is it about older kernels prevents the tracing-induced recursive
> > calls to rcu_irq_enter()?
>
> Ah, the bug is if rcu_irq_enter() is called, and the stack trace
> triggers then. OK, lets keep it simple, and just say this.
>
>
> Currently the stack tracer calls rcu_irq_enter() to make sure RCU
> is watching when it records a stack trace. But if the stack tracer
> is triggered while tracing inside of a rcu_irq_enter(), calling
> rcu_irq_enter() unconditionally can be problematic.
>
> The reason for having rcu_irq_enter() in the first place has been
> fixed from within the saving of the stack trace code, and there's no
> reason for doing it in the stack tracer itself. Just remove it.
>
> Simple and to the point.

Works for me!

Thanx, Paul