Date:	Tue, 3 Sep 2013 08:22:53 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Jiri Olsa <jolsa@...hat.com>
Subject: Re: [RFC][PATCH 11/18 v2] ftrace: Adde infrastructure to stop RCU
 unsafe checker from checking

On Tue, Sep 03, 2013 at 09:43:52AM -0400, Steven Rostedt wrote:
> On Sat, 31 Aug 2013 12:52:58 -0700
> "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com> wrote:
> 
> > On Sat, Aug 31, 2013 at 01:11:28AM -0400, Steven Rostedt wrote:
> > > From: "Steven Rostedt (Red Hat)" <rostedt@...dmis.org>
> > > 
> > > This is a light weight way to keep the rcu checker from checking
> > > RCU safety. It adds a ftrace_unsafe_rcu_checker_disable/enable()
> > > that increments or decrements a counter respectively. When the
> > > counter is set, the RCU unsafe checker callback does not run the
> > > tests to see if RCU is safe or not.
> > 
> > Please add something saying what we do instead of testing RCU safety.
> > Looks to me like it skips not only the tests, but also invoking the
> > callback, but I could easily be wrong.
> 
> Now that I'm awake and also not on holiday, I can reply with a clear
> mind.
> 
> When disabled, it forces the callback to return immediately. I may in
> the future, have it disable the callback altogether.
> 
> Yes, we can miss checks, but it's better than the test live locking
> the system :-)
> 
> > 
> > > This is required by the graph tracer because the checks can cause
> > > the graph tracer to live lock the system by its own calls.
> 
> I added this (I quoted the added text):
> 
>     This is a light weight way to keep the rcu checker from checking
>     RCU safety. It adds a ftrace_unsafe_rcu_checker_disable/enable()
>     that increments or decrements a counter respectively. When the
>     counter is set, the RCU unsafe checker callback does not run the
>     tests to see if RCU is safe or not. "But the callback still gets
>     called. It just does not call rcu_read_(un)lock()."
>     
>     This is required by the graph tracer because the checks can cause
>     the graph tracer to live lock the system by its own calls. "That is,
>     the graph tracer will still trace rcu and the rcu debugging, and
>     this will slow down the checker, which is still calling all other
>     functions (in interrupts and faults), which can cause the timer
>     interrupt to take oven a millisecond to complete, and it will then
>     trigger once it finishes, live locking the system."
> 
> 
> > > 
> > > It's also needed by the irqsoff tracer, because it may be called
> > > in RCU unsafe regions and if its internal functions get traced
> > > then the RCU unsafe checker may have some false positives.
> > > 
> > > Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
> > 
> > With the augmented commit log as noted above:
> 
> Is the above OK?

Works for me!

							Thanx, Paul

> -- Steve
> 
> > 
> > Acked-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> > 
> > > ---
> > >  kernel/trace/trace.h           |   12 +++++++++---
> > >  kernel/trace/trace_functions.c |   15 +++++++++++++++
> > >  2 files changed, 24 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > > index e551316..58e4c37 100644
> > > --- a/kernel/trace/trace.h
> > > +++ b/kernel/trace/trace.h
> > > @@ -760,9 +760,6 @@ static inline int ftrace_graph_addr(unsigned long addr)
> > > 
> > >  	return 0;
> > >  }
> > > -#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> > > -extern bool ftrace_rcu_unsafe(unsigned long addr);
> > > -#endif
> > >  #else
> > >  static inline int ftrace_graph_addr(unsigned long addr)
> > >  {
> > > @@ -1061,4 +1058,13 @@ int perf_ftrace_event_register(struct ftrace_event_call *call,
> > >  #define perf_ftrace_event_register NULL
> > >  #endif
> > > 
> > > +#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> > > +extern bool ftrace_rcu_unsafe(unsigned long addr);
> > > +extern void ftrace_unsafe_rcu_checker_disable(void);
> > > +extern void ftrace_unsafe_rcu_checker_enable(void);
> > > +#else
> > > +static inline void ftrace_unsafe_rcu_checker_disable(void) { }
> > > +static inline void ftrace_unsafe_rcu_checker_enable(void) { }
> > > +#endif
> > > +
> > >  #endif /* _LINUX_KERNEL_TRACE_H */
> > > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> > > index 9dd4627..1d5f951 100644
> > > --- a/kernel/trace/trace_functions.c
> > > +++ b/kernel/trace/trace_functions.c
> > > @@ -560,12 +560,27 @@ static inline int init_func_cmd_traceon(void)
> > >  #endif /* CONFIG_DYNAMIC_FTRACE */
> > > 
> > >  #ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> > > +static atomic_t ftrace_unsafe_rcu_disabled;
> > > +
> > > +void ftrace_unsafe_rcu_checker_disable(void)
> > > +{
> > > +	atomic_inc(&ftrace_unsafe_rcu_disabled);
> > > +}
> > > +
> > > +void ftrace_unsafe_rcu_checker_enable(void)
> > > +{
> > > +	atomic_dec(&ftrace_unsafe_rcu_disabled);
> > > +}
> > > +
> > >  static void
> > >  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> > >  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
> > >  {
> > >  	int bit;
> > > 
> > > +	if (atomic_read(&ftrace_unsafe_rcu_disabled))
> > > +		return;
> > > +
> > >  	preempt_disable_notrace();
> > > 
> > >  	bit = trace_test_and_set_recursion(TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> > > -- 
> > > 1.7.10.4
> > > 
> > > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists