[PATCH 0/3][RFC] tracing/rcu: Add _rcuidle() tracepoint to handle rcu_idle_exit() tracepoints

[PATCH 0/3][RFC] tracing/rcu: Add _rcuidle() tracepoint to handle rcu_idle_exit() tracepoints

[Steven Rostedt]

This is an RFC patch set for what I plan on pushing out to handle
the tracepoints that are called from within rcu_idle_exit() sections.

The first patch is the tracepoint infrastructure,
the second is the changes in x86
the third is the changes in the cpuidle driver.

I see there's use cases in the arm arch, but as that is in flux, I'll
let the arm folks deal with those.

-- Steve


Steven Rostedt (3):
      tracing/rcu: Add trace_##name##__rcuidle() static tracepoint for inside rcu_idle_exit() sections
      x86/tracing: Denote the power and cpuidle tracepoints as _rcuidle()
      cpuidle/tracing: Denote the tracepoints as being in rcu_idle_exit() section

----
 arch/x86/kernel/process.c  |   24 ++++++++++++------------
 drivers/cpuidle/cpuidle.c  |    8 ++++----
 include/linux/tracepoint.h |   20 ++++++++++++++++----
 3 files changed, 32 insertions(+), 20 deletions(-)

[PATCH 1/3][RFC] tracing/rcu: Add trace_##name##__rcuidle() static tracepoint for inside rcu_idle_exit() sections

[Steven Rostedt]

[-- Attachment #1: 0001-tracing-rcu-Add-trace_-name-__rcuidle-static-tracepo.patch --]
[-- Type: text/plain, Size: 4119 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Added is a new static inline function that lets *any* tracepoint be used
inside a rcu_idle_exit() section. And this also solves the problem where
the same tracepoint may be used inside a rcu_idle_exit() section as well
as outside of one.

I added a new tracepoint function with a "_rcuidle" extension. All
tracepoints can be used with either the normal "trace_foobar()"
function, or the "trace_foobar_rcuidle()" function when inside a
rcu_idle_exit() section.

All tracepoints defined by TRACE_EVENT() or any of the derivatives
will have a "_rcuidle()" function also defined. When a tracepoint is
used within an rcu_idle_exit() section, the "_rcuidle()" version must
be used. This denotes that the tracepoint is within rcu_idle_exit()
and it allows the rcu read locks within the tracepoint to still
be valid, as this version takes us out of rcu_idle_exit().

Another nice aspect about this patch is that "static inline"s are not
compiled into text when not used. So only the tracepoints that actually
use the _rcuidle() version will have them defined in the actual text
that is booted.

Link: http://lkml.kernel.org/r/1328563113.2200.39.camel@gandalf.stny.rr.com>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/tracepoint.h |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index df0a779..00d073a 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -114,7 +114,7 @@ static inline void tracepoint_synchronize_unregister(void)
  * as "(void *, void)". The DECLARE_TRACE_NOARGS() will pass in just
  * "void *data", where as the DECLARE_TRACE() will pass in "void *data, proto".
  */
-#define __DO_TRACE(tp, proto, args, cond)				\
+#define __DO_TRACE(tp, proto, args, cond, prercu, postrcu)		\
 	do {								\
 		struct tracepoint_func *it_func_ptr;			\
 		void *it_func;						\
@@ -122,6 +122,7 @@ static inline void tracepoint_synchronize_unregister(void)
 									\
 		if (!(cond))						\
 			return;						\
+		prercu;							\
 		rcu_read_lock_sched_notrace();				\
 		it_func_ptr = rcu_dereference_sched((tp)->funcs);	\
 		if (it_func_ptr) {					\
@@ -132,6 +133,7 @@ static inline void tracepoint_synchronize_unregister(void)
 			} while ((++it_func_ptr)->func);		\
 		}							\
 		rcu_read_unlock_sched_notrace();			\
+		postrcu;						\
 	} while (0)
 
 /*
@@ -139,7 +141,7 @@ static inline void tracepoint_synchronize_unregister(void)
  * not add unwanted padding between the beginning of the section and the
  * structure. Force alignment to the same alignment as the section start.
  */
-#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args)	\
+#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
 	extern struct tracepoint __tracepoint_##name;			\
 	static inline void trace_##name(proto)				\
 	{								\
@@ -147,7 +149,17 @@ static inline void tracepoint_synchronize_unregister(void)
 			__DO_TRACE(&__tracepoint_##name,		\
 				TP_PROTO(data_proto),			\
 				TP_ARGS(data_args),			\
-				TP_CONDITION(cond));			\
+				TP_CONDITION(cond),,);			\
+	}								\
+	static inline void trace_##name##_rcuidle(proto)		\
+	{								\
+		if (static_branch(&__tracepoint_##name.key))		\
+			__DO_TRACE(&__tracepoint_##name,		\
+				TP_PROTO(data_proto),			\
+				TP_ARGS(data_args),			\
+				TP_CONDITION(cond),			\
+				rcu_idle_exit(),			\
+				rcu_idle_enter());			\
 	}								\
 	static inline int						\
 	register_trace_##name(void (*probe)(data_proto), void *data)	\
@@ -190,7 +202,7 @@ static inline void tracepoint_synchronize_unregister(void)
 	EXPORT_SYMBOL(__tracepoint_##name)
 
 #else /* !CONFIG_TRACEPOINTS */
-#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args)	\
+#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
 	static inline void trace_##name(proto)				\
 	{ }								\
 	static inline int						\
-- 
1.7.8.3

[PATCH 2/3][RFC] x86/tracing: Denote the power and cpuidle tracepoints as _rcuidle()

[Steven Rostedt]

[-- Attachment #1: 0002-x86-tracing-Denote-the-power-and-cpuidle-tracepoints.patch --]
[-- Type: text/plain, Size: 2779 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The power and cpuidle tracepoints are called within a rcu_idle_exit()
section, and must be denoted with the _rcuidle() version of the tracepoint.

Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/process.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 15763af..44eefde 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -377,8 +377,8 @@ static inline int hlt_use_halt(void)
 void default_idle(void)
 {
 	if (hlt_use_halt()) {
-		trace_power_start(POWER_CSTATE, 1, smp_processor_id());
-		trace_cpu_idle(1, smp_processor_id());
+		trace_power_start_rcuidle(POWER_CSTATE, 1, smp_processor_id());
+		trace_cpu_idle_rcuidle(1, smp_processor_id());
 		current_thread_info()->status &= ~TS_POLLING;
 		/*
 		 * TS_POLLING-cleared state must be visible before we
@@ -391,8 +391,8 @@ void default_idle(void)
 		else
 			local_irq_enable();
 		current_thread_info()->status |= TS_POLLING;
-		trace_power_end(smp_processor_id());
-		trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
+		trace_power_end_rcuidle(smp_processor_id());
+		trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
 	} else {
 		local_irq_enable();
 		/* loop is done by the caller */
@@ -450,8 +450,8 @@ EXPORT_SYMBOL_GPL(cpu_idle_wait);
 static void mwait_idle(void)
 {
 	if (!need_resched()) {
-		trace_power_start(POWER_CSTATE, 1, smp_processor_id());
-		trace_cpu_idle(1, smp_processor_id());
+		trace_power_start_rcuidle(POWER_CSTATE, 1, smp_processor_id());
+		trace_cpu_idle_rcuidle(1, smp_processor_id());
 		if (this_cpu_has(X86_FEATURE_CLFLUSH_MONITOR))
 			clflush((void *)&current_thread_info()->flags);
 
@@ -461,8 +461,8 @@ static void mwait_idle(void)
 			__sti_mwait(0, 0);
 		else
 			local_irq_enable();
-		trace_power_end(smp_processor_id());
-		trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
+		trace_power_end_rcuidle(smp_processor_id());
+		trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
 	} else
 		local_irq_enable();
 }
@@ -474,13 +474,13 @@ static void mwait_idle(void)
  */
 static void poll_idle(void)
 {
-	trace_power_start(POWER_CSTATE, 0, smp_processor_id());
-	trace_cpu_idle(0, smp_processor_id());
+	trace_power_start_rcuidle(POWER_CSTATE, 0, smp_processor_id());
+	trace_cpu_idle_rcuidle(0, smp_processor_id());
 	local_irq_enable();
 	while (!need_resched())
 		cpu_relax();
-	trace_power_end(smp_processor_id());
-	trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
+	trace_power_end_rcuidle(smp_processor_id());
+	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
 }
 
 /*
-- 
1.7.8.3

[PATCH 3/3][RFC] cpuidle/tracing: Denote the tracepoints as being in rcu_idle_exit() section

[Steven Rostedt]

[-- Attachment #1: 0003-cpuidle-tracing-Denote-the-tracepoints-as-being-in-r.patch --]
[-- Type: text/plain, Size: 1253 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

As the tracepoints in the cpuidle code are called when rcu_idle_exit() is in
effect, the _rcuidle() version must be used, otherwise the rcu_read_lock()s
that protect the tracepoint will not be honored.

Cc: Len Brown <len.brown@intel.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 drivers/cpuidle/cpuidle.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 59f4261..6588f43 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -94,13 +94,13 @@ int cpuidle_idle_call(void)
 
 	target_state = &drv->states[next_state];
 
-	trace_power_start(POWER_CSTATE, next_state, dev->cpu);
-	trace_cpu_idle(next_state, dev->cpu);
+	trace_power_start_rcuidle(POWER_CSTATE, next_state, dev->cpu);
+	trace_cpu_idle_rcuidle(next_state, dev->cpu);
 
 	entered_state = target_state->enter(dev, drv, next_state);
 
-	trace_power_end(dev->cpu);
-	trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
+	trace_power_end_rcuidle(dev->cpu);
+	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
 
 	if (entered_state >= 0) {
 		/* Update cpuidle counters */
-- 
1.7.8.3

Re: [PATCH 0/3][RFC] tracing/rcu: Add _rcuidle() tracepoint to handle rcu_idle_exit() tracepoints

[Paul E. McKenney]

On Tue, Feb 07, 2012 at 09:56:53AM -0500, Steven Rostedt wrote:
> This is an RFC patch set for what I plan on pushing out to handle
> the tracepoints that are called from within rcu_idle_exit() sections.
> 
> The first patch is the tracepoint infrastructure,
> the second is the changes in x86
> the third is the changes in the cpuidle driver.
> 
> I see there's use cases in the arm arch, but as that is in flux, I'll
> let the arm folks deal with those.

Please note that the __rcudle() tracepoints depend on patch #45 from
my series (https://lkml.org/lkml/2012/2/3/471), though only for code
containing tracepoints that are called both from idle and non-idle.
I believe that the tracepoints in this patch series are only invoked
from idle, so should be no problem.

							Thanx, Paul

> -- Steve
> 
> 
> Steven Rostedt (3):
>       tracing/rcu: Add trace_##name##__rcuidle() static tracepoint for inside rcu_idle_exit() sections
>       x86/tracing: Denote the power and cpuidle tracepoints as _rcuidle()
>       cpuidle/tracing: Denote the tracepoints as being in rcu_idle_exit() section
> 
> ----
>  arch/x86/kernel/process.c  |   24 ++++++++++++------------
>  drivers/cpuidle/cpuidle.c  |    8 ++++----
>  include/linux/tracepoint.h |   20 ++++++++++++++++----
>  3 files changed, 32 insertions(+), 20 deletions(-)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 3/3][RFC] cpuidle/tracing: Denote the tracepoints as being in rcu_idle_exit() section

[Paul E. McKenney]

On Tue, Feb 07, 2012 at 09:56:56AM -0500, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> As the tracepoints in the cpuidle code are called when rcu_idle_exit() is in
> effect, the _rcuidle() version must be used, otherwise the rcu_read_lock()s
> that protect the tracepoint will not be honored.
> 
> Cc: Len Brown <len.brown@intel.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Looks good to me!

Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  drivers/cpuidle/cpuidle.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 59f4261..6588f43 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -94,13 +94,13 @@ int cpuidle_idle_call(void)
> 
>  	target_state = &drv->states[next_state];
> 
> -	trace_power_start(POWER_CSTATE, next_state, dev->cpu);
> -	trace_cpu_idle(next_state, dev->cpu);
> +	trace_power_start_rcuidle(POWER_CSTATE, next_state, dev->cpu);
> +	trace_cpu_idle_rcuidle(next_state, dev->cpu);
> 
>  	entered_state = target_state->enter(dev, drv, next_state);
> 
> -	trace_power_end(dev->cpu);
> -	trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
> +	trace_power_end_rcuidle(dev->cpu);
> +	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> 
>  	if (entered_state >= 0) {
>  		/* Update cpuidle counters */
> -- 
> 1.7.8.3
> 
> 

Re: [PATCH 0/3][RFC] tracing/rcu: Add _rcuidle() tracepoint to handle rcu_idle_exit() tracepoints

[Steven Rostedt]

On Tue, 2012-02-07 at 07:37 -0800, Paul E. McKenney wrote:

> Please note that the __rcudle() tracepoints depend on patch #45 from
> my series (https://lkml.org/lkml/2012/2/3/471), though only for code
> containing tracepoints that are called both from idle and non-idle.
> I believe that the tracepoints in this patch series are only invoked
> from idle, so should be no problem.

Right, as I tested these without your patch series.

-- Steve

Re: [PATCH 0/3][RFC] tracing/rcu: Add _rcuidle() tracepoint to handle rcu_idle_exit() tracepoints

[Josh Triplett]

On Tue, Feb 07, 2012 at 09:56:53AM -0500, Steven Rostedt wrote:
> This is an RFC patch set for what I plan on pushing out to handle
> the tracepoints that are called from within rcu_idle_exit() sections.
> 
> The first patch is the tracepoint infrastructure,
> the second is the changes in x86
> the third is the changes in the cpuidle driver.

Patches 2 and 3 look sensible to me as well.

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

Re: [PATCH 3/3][RFC] cpuidle/tracing: Denote the tracepoints as being in rcu_idle_exit() section

[Steven Rostedt]

On Tue, 2012-02-07 at 07:40 -0800, Paul E. McKenney wrote:
> On Tue, Feb 07, 2012 at 09:56:56AM -0500, Steven Rostedt wrote:
> > From: Steven Rostedt <srostedt@redhat.com>
> > 
> > As the tracepoints in the cpuidle code are called when rcu_idle_exit() is in
> > effect, the _rcuidle() version must be used, otherwise the rcu_read_lock()s
> > that protect the tracepoint will not be honored.
> > 
> > Cc: Len Brown <len.brown@intel.com>
> > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Looks good to me!
> 
> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Thanks, but does this include patch 2/3 as well?

-- Steve

Re: [PATCH 2/3][RFC] x86/tracing: Denote the power and cpuidle tracepoints as _rcuidle()

[Paul E. McKenney]

On Tue, Feb 07, 2012 at 09:56:55AM -0500, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> The power and cpuidle tracepoints are called within a rcu_idle_exit()
> section, and must be denoted with the _rcuidle() version of the tracepoint.
> 
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

(Sorry, fat-fingered this one the first time around.)

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  arch/x86/kernel/process.c |   24 ++++++++++++------------
>  1 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 15763af..44eefde 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -377,8 +377,8 @@ static inline int hlt_use_halt(void)
>  void default_idle(void)
>  {
>  	if (hlt_use_halt()) {
> -		trace_power_start(POWER_CSTATE, 1, smp_processor_id());
> -		trace_cpu_idle(1, smp_processor_id());
> +		trace_power_start_rcuidle(POWER_CSTATE, 1, smp_processor_id());
> +		trace_cpu_idle_rcuidle(1, smp_processor_id());
>  		current_thread_info()->status &= ~TS_POLLING;
>  		/*
>  		 * TS_POLLING-cleared state must be visible before we
> @@ -391,8 +391,8 @@ void default_idle(void)
>  		else
>  			local_irq_enable();
>  		current_thread_info()->status |= TS_POLLING;
> -		trace_power_end(smp_processor_id());
> -		trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
> +		trace_power_end_rcuidle(smp_processor_id());
> +		trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
>  	} else {
>  		local_irq_enable();
>  		/* loop is done by the caller */
> @@ -450,8 +450,8 @@ EXPORT_SYMBOL_GPL(cpu_idle_wait);
>  static void mwait_idle(void)
>  {
>  	if (!need_resched()) {
> -		trace_power_start(POWER_CSTATE, 1, smp_processor_id());
> -		trace_cpu_idle(1, smp_processor_id());
> +		trace_power_start_rcuidle(POWER_CSTATE, 1, smp_processor_id());
> +		trace_cpu_idle_rcuidle(1, smp_processor_id());
>  		if (this_cpu_has(X86_FEATURE_CLFLUSH_MONITOR))
>  			clflush((void *)&current_thread_info()->flags);
> 
> @@ -461,8 +461,8 @@ static void mwait_idle(void)
>  			__sti_mwait(0, 0);
>  		else
>  			local_irq_enable();
> -		trace_power_end(smp_processor_id());
> -		trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
> +		trace_power_end_rcuidle(smp_processor_id());
> +		trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
>  	} else
>  		local_irq_enable();
>  }
> @@ -474,13 +474,13 @@ static void mwait_idle(void)
>   */
>  static void poll_idle(void)
>  {
> -	trace_power_start(POWER_CSTATE, 0, smp_processor_id());
> -	trace_cpu_idle(0, smp_processor_id());
> +	trace_power_start_rcuidle(POWER_CSTATE, 0, smp_processor_id());
> +	trace_cpu_idle_rcuidle(0, smp_processor_id());
>  	local_irq_enable();
>  	while (!need_resched())
>  		cpu_relax();
> -	trace_power_end(smp_processor_id());
> -	trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
> +	trace_power_end_rcuidle(smp_processor_id());
> +	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
>  }
> 
>  /*
> -- 
> 1.7.8.3
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>