On Fri, Sep 22, 2017 at 06:15:47PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
>
> While debugging some RCU issues with the stack tracer, it was discovered
> that the problem was much more than with the stack tracer itself, but with
> the saving of the stack trace, which could happen from any WARN() as well.
> The problem was fixed within kernel_text_address().
>
> One of the bugs that was discovered was that the stack tracer called
> rcu_enter_irq() unconditionally. Paul McKenney said that could cause issues
> as well. Instead of adding logic to only call rcu_enter_irq() if RCU is not
> watching from within the stack tracer, since the core issue has been fixed
> (within save_stack_trace()), we can simply remove all the logic in the stack
> tracer that deals with RCU work arounds.
I must confess that I am having some difficulty parsing this paragraph,
especially the last sentence...
Does this capture it?
One problem is that the stack tracer called rcu_irq_enter()
unconditionally, which is problematic if RCU's last
not-watching-to-watching transition was carried out by
rcu_nmi_enter. 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.
One further question... Can I now remove the rcu_irq_enter_disabled()
logic?
> Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 0be964be0 ("module: Sanitize RCU usage and locking")
> Suggested-by: "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
With the hard-to-parse paragraph fixed:
Acked-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> ---
> kernel/trace/trace_stack.c | 15 ---------------
> 1 file changed, 15 deletions(-)
>
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index a4df67cbc711..49cb41412eec 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -96,23 +96,9 @@ check_stack(unsigned long ip, unsigned long *stack)
> if (in_nmi())
> return;
>
> - /*
> - * There's a slight chance that we are tracing inside the
> - * RCU infrastructure, and rcu_irq_enter() will not work
> - * as expected.
> - */
> - if (unlikely(rcu_irq_enter_disabled()))
> - return;
> -
> local_irq_save(flags);
> arch_spin_lock(&stack_trace_max_lock);
>
> - /*
> - * RCU may not be watching, make it see us.
> - * The stack trace code uses rcu_sched.
> - */
> - rcu_irq_enter();
> -
> /* In case another CPU set the tracer_frame on us */
> if (unlikely(!frame_size))
> this_size -= tracer_frame;
> @@ -205,7 +191,6 @@ check_stack(unsigned long ip, unsigned long *stack)
> }
>
> out:
> - rcu_irq_exit();
> arch_spin_unlock(&stack_trace_max_lock);
> local_irq_restore(flags);
> }
> --
> 2.13.2
>
>