Messages in this thread | | | Date | Tue, 10 Mar 2020 13:53:19 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH] tracepoint: rcuidle: use rcu_is_watching() and tree-rcu |
| |
On Tue, Mar 10, 2020 at 04:20:54PM -0400, Mathieu Desnoyers wrote: > commit e6753f23d961 ("tracepoint: Make rcuidle tracepoint callers use > SRCU") aimed at improving performance of rcuidle tracepoints by using > SRCU rather than temporarily enabling tree-rcu every time. > > commit 865e63b04e9b ("tracing: Add back in rcu_irq_enter/exit_irqson() > for rcuidle tracepoints") adds back the high-overhead enabling of > tree-rcu because perf expects RCU to be watching when called from > rcuidle tracepoints. > > It turns out that by using "rcu_is_watching()" and conditionally > calling the high-overhead rcu_irq_enter/exit_irqson(), the original > motivation for using SRCU in the first place disappears.
Adding Alexei on CC for his thoughts, given that these were his benchmarks. I believe that he also has additional use cases.
But given the use cases you describe, this seems plausible. This does mean that tracepoints cannot be attached to the CPU-hotplug code that runs on the incoming/outgoing CPU early/late in that process, though that might be OK.
Thanx, Paul
> I suspect that the original benchmarks justifying the introduction > of SRCU to handle rcuidle tracepoints was caused by preempt/irq > tracepoints, which are typically invoked from contexts that have > RCU watching. > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: Steven Rostedt <rostedt@goodmis.org> > CC: Joel Fernandes <joel@joelfernandes.org> > CC: "Paul E. McKenney" <paulmck@kernel.org> > CC: Peter Zijlstra <peterz@infradead.org> > CC: Frederic Weisbecker <fweisbec@gmail.com> > CC: Ingo Molnar <mingo@kernel.org> > --- > include/linux/tracepoint.h | 25 ++++++++++--------------- > 1 file changed, 10 insertions(+), 15 deletions(-) > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h > index 1fb11daa5c53..8e0e94fee29a 100644 > --- a/include/linux/tracepoint.h > +++ b/include/linux/tracepoint.h > @@ -165,25 +165,22 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) > void *it_func; \ > void *__data; \ > int __maybe_unused __idx = 0; \ > + bool __exit_rcu = false; \ > \ > if (!(cond)) \ > return; \ > \ > - /* srcu can't be used from NMI */ \ > - WARN_ON_ONCE(rcuidle && in_nmi()); \ > - \ > - /* keep srcu and sched-rcu usage consistent */ \ > - preempt_disable_notrace(); \ > - \ > /* \ > - * For rcuidle callers, use srcu since sched-rcu \ > - * doesn't work from the idle path. \ > + * For rcuidle callers, temporarily enable RCU if \ > + * it is not currently watching. \ > */ \ > - if (rcuidle) { \ > - __idx = srcu_read_lock_notrace(&tracepoint_srcu);\ > + if (rcuidle && !rcu_is_watching()) { \ > rcu_irq_enter_irqson(); \ > + __exit_rcu = true; \ > } \ > \ > + preempt_disable_notrace(); \ > + \ > it_func_ptr = rcu_dereference_raw((tp)->funcs); \ > \ > if (it_func_ptr) { \ > @@ -194,12 +191,10 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) > } while ((++it_func_ptr)->func); \ > } \ > \ > - if (rcuidle) { \ > - rcu_irq_exit_irqson(); \ > - srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\ > - } \ > - \ > preempt_enable_notrace(); \ > + \ > + if (__exit_rcu) \ > + rcu_irq_exit_irqson(); \ > } while (0) > > #ifndef MODULE > -- > 2.17.1 >
| |