LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] tracepoint: rcuidle: use rcu_is_watching() and tree-rcu
@ 2020-03-10 20:20 Mathieu Desnoyers
  2020-03-10 20:53 ` Paul E. McKenney
  0 siblings, 1 reply; 6+ messages in thread
From: Mathieu Desnoyers @ 2020-03-10 20:20 UTC (permalink / raw)
  To: Thomas Gleixner, Steven Rostedt
  Cc: linux-kernel, Mathieu Desnoyers, Joel Fernandes,
	Paul E. McKenney, Peter Zijlstra, Frederic Weisbecker,
	Ingo Molnar

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.

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] tracepoint: rcuidle: use rcu_is_watching() and tree-rcu
  2020-03-10 20:20 [PATCH] tracepoint: rcuidle: use rcu_is_watching() and tree-rcu Mathieu Desnoyers
@ 2020-03-10 20:53 ` Paul E. McKenney
  2020-03-10 23:44   ` Mathieu Desnoyers
  0 siblings, 1 reply; 6+ messages in thread
From: Paul E. McKenney @ 2020-03-10 20:53 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Thomas Gleixner, Steven Rostedt, linux-kernel, Joel Fernandes,
	Peter Zijlstra, Frederic Weisbecker, Ingo Molnar

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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] tracepoint: rcuidle: use rcu_is_watching() and tree-rcu
  2020-03-10 20:53 ` Paul E. McKenney
@ 2020-03-10 23:44   ` Mathieu Desnoyers
  2020-03-10 23:52     ` Paul E. McKenney
  0 siblings, 1 reply; 6+ messages in thread
From: Mathieu Desnoyers @ 2020-03-10 23:44 UTC (permalink / raw)
  To: paulmck
  Cc: Thomas Gleixner, rostedt, linux-kernel, Joel Fernandes, Google,
	Peter Zijlstra, fweisbec, Ingo Molnar

----- On Mar 10, 2020, at 4:53 PM, paulmck paulmck@kernel.org wrote:

> 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.

Good point! Sorry I forgot to add Alexei to my CC list for that
patch.

> 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.

Do you mean standard tracepoints or rcuidle tracepoints ?

Are there any such tracepoints early/late in the cpu hotplug code today ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] tracepoint: rcuidle: use rcu_is_watching() and tree-rcu
  2020-03-10 23:44   ` Mathieu Desnoyers
@ 2020-03-10 23:52     ` Paul E. McKenney
  2020-03-10 23:56       ` Mathieu Desnoyers
  0 siblings, 1 reply; 6+ messages in thread
From: Paul E. McKenney @ 2020-03-10 23:52 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Thomas Gleixner, rostedt, linux-kernel, Joel Fernandes, Google,
	Peter Zijlstra, fweisbec, Ingo Molnar

On Tue, Mar 10, 2020 at 07:44:33PM -0400, Mathieu Desnoyers wrote:
> ----- On Mar 10, 2020, at 4:53 PM, paulmck paulmck@kernel.org wrote:
> 
> > 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.
> 
> Good point! Sorry I forgot to add Alexei to my CC list for that
> patch.
> 
> > 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.
> 
> Do you mean standard tracepoints or rcuidle tracepoints ?
> 
> Are there any such tracepoints early/late in the cpu hotplug code today ?

I have no idea.  You would know better than I.  However, I would expect
that the same common-code issue that applies to exception-entry/exit
code might also apply to the CPU hotplug code.

							Thanx, Paul

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] tracepoint: rcuidle: use rcu_is_watching() and tree-rcu
  2020-03-10 23:52     ` Paul E. McKenney
@ 2020-03-10 23:56       ` Mathieu Desnoyers
  2020-03-11  0:08         ` Paul E. McKenney
  0 siblings, 1 reply; 6+ messages in thread
From: Mathieu Desnoyers @ 2020-03-10 23:56 UTC (permalink / raw)
  To: paulmck
  Cc: Thomas Gleixner, rostedt, linux-kernel, Joel Fernandes, Google,
	Peter Zijlstra, fweisbec, Ingo Molnar

----- On Mar 10, 2020, at 7:52 PM, paulmck paulmck@kernel.org wrote:

> On Tue, Mar 10, 2020 at 07:44:33PM -0400, Mathieu Desnoyers wrote:
>> ----- On Mar 10, 2020, at 4:53 PM, paulmck paulmck@kernel.org wrote:
>> 
>> > 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.
>> 
>> Good point! Sorry I forgot to add Alexei to my CC list for that
>> patch.
>> 
>> > 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.
>> 
>> Do you mean standard tracepoints or rcuidle tracepoints ?
>> 
>> Are there any such tracepoints early/late in the cpu hotplug code today ?
> 
> I have no idea.  You would know better than I.  However, I would expect
> that the same common-code issue that applies to exception-entry/exit
> code might also apply to the CPU hotplug code.

I would also expect early/late CPU hotplug states to fall into the same
category of "low-level entry/exit" code which Thomas would like to hide
from instrumentation.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] tracepoint: rcuidle: use rcu_is_watching() and tree-rcu
  2020-03-10 23:56       ` Mathieu Desnoyers
@ 2020-03-11  0:08         ` Paul E. McKenney
  0 siblings, 0 replies; 6+ messages in thread
From: Paul E. McKenney @ 2020-03-11  0:08 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Thomas Gleixner, rostedt, linux-kernel, Joel Fernandes, Google,
	Peter Zijlstra, fweisbec, Ingo Molnar

On Tue, Mar 10, 2020 at 07:56:17PM -0400, Mathieu Desnoyers wrote:
> ----- On Mar 10, 2020, at 7:52 PM, paulmck paulmck@kernel.org wrote:
> 
> > On Tue, Mar 10, 2020 at 07:44:33PM -0400, Mathieu Desnoyers wrote:
> >> ----- On Mar 10, 2020, at 4:53 PM, paulmck paulmck@kernel.org wrote:
> >> 
> >> > 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.
> >> 
> >> Good point! Sorry I forgot to add Alexei to my CC list for that
> >> patch.
> >> 
> >> > 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.
> >> 
> >> Do you mean standard tracepoints or rcuidle tracepoints ?
> >> 
> >> Are there any such tracepoints early/late in the cpu hotplug code today ?
> > 
> > I have no idea.  You would know better than I.  However, I would expect
> > that the same common-code issue that applies to exception-entry/exit
> > code might also apply to the CPU hotplug code.
> 
> I would also expect early/late CPU hotplug states to fall into the same
> category of "low-level entry/exit" code which Thomas would like to hide
> from instrumentation.

That decision I must leave to you guys.

							Thanx, Paul

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-03-11  0:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-10 20:20 [PATCH] tracepoint: rcuidle: use rcu_is_watching() and tree-rcu Mathieu Desnoyers
2020-03-10 20:53 ` Paul E. McKenney
2020-03-10 23:44   ` Mathieu Desnoyers
2020-03-10 23:52     ` Paul E. McKenney
2020-03-10 23:56       ` Mathieu Desnoyers
2020-03-11  0:08         ` Paul E. McKenney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).