[PATCH RFC idle 2/3] arm: Avoid invoking RCU when CPU is idle

"Paul E. McKenney" [off-list ref] writes:

[...]

quoted

quoted

The two options I see are:

1.	Rip tracing out of the inner idle loops and everything that
	they invoke.

What I suggested above.  But as I said I know sh*t about that tracing 
implementation so that's an easy suggestion for me to make.

Works for me as well.  ;-)

While I must admit not having a better suggestion, I for one would vote
strongly against removing tracing from the idle path.

Being a PM developer and maintainer, much of the code I work on and
maintain happens to be run in the bowels of the idle path.  Not having
the ability to trace this code would be a major step backwards IMO.

Kevin

On 02/02/2012 04:20 PM, Kevin Hilman wrote:

"Paul E. McKenney" [off-list ref] writes:

[...]

quoted

quoted

quoted

The two options I see are:

1.	Rip tracing out of the inner idle loops and everything that
	they invoke.

What I suggested above.  But as I said I know sh*t about that tracing 
implementation so that's an easy suggestion for me to make.

Works for me as well.  ;-)

While I must admit not having a better suggestion, I for one would vote
strongly against removing tracing from the idle path.

Being a PM developer and maintainer, much of the code I work on and
maintain happens to be run in the bowels of the idle path.  Not having
the ability to trace this code would be a major step backwards IMO.

How is it a step backwards if it is already broken. Obviously you
haven't actually used any tracing here because it doesn't work right
with things as is.

What exactly do you want to trace at this level. By the point you are in
this code, the path is somewhat known and problems you have are likely
h/w issues. If you are trying to go thru a very precise sequence of
saving cpu state and flushing caches, you don't want calls out to
tracing code that could very easily change the behavior.

It would be nice to understand a real example of how to hit this
problem. Why type of tracing? Are there other known examples?

Rob

On Thu, Feb 02, 2012 at 02:20:26PM -0800, Kevin Hilman wrote:

"Paul E. McKenney" [off-list ref] writes:

[...]

quoted

quoted

quoted

The two options I see are:

1.	Rip tracing out of the inner idle loops and everything that
	they invoke.

What I suggested above.  But as I said I know sh*t about that tracing 
implementation so that's an easy suggestion for me to make.

Works for me as well.  ;-)

While I must admit not having a better suggestion, I for one would vote
strongly against removing tracing from the idle path.

Being a PM developer and maintainer, much of the code I work on and
maintain happens to be run in the bowels of the idle path.  Not having
the ability to trace this code would be a major step backwards IMO.

OK...

What if the tracing code between the rcu_idle_enter() and the
rcu_idle_exit() had to be enclosed in a wrapper?  For example,
the tracing in cpuidle_idle_call() might appear as follows:

	RCU_NONIDLE(
		trace_power_start(POWER_CSTATE, next_state, dev->cpu);
		trace_cpu_idle(next_state, dev->cpu);
	);

	entered_state = target_state->enter(dev, drv, next_state);

	RCU_NONIDLE(
		trace_power_end(dev->cpu);
		trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
	);

The RCU_NONIDLE() macro would do rcu_idle_exit(), execute its
argument, then do rcu_idle_enter().  (Credit to Steven Rostedt
for suggesting this.)  Given the possibility of code invoked both
from idle and not-idle, I have some changes to rcu to allow nesting
of rcu_idle_enter() and rcu_idle_exit().

Would that work for you?

							Thanx, Paul

On Thu, 2012-02-02 at 16:49 -0600, Rob Herring wrote:

On 02/02/2012 04:20 PM, Kevin Hilman wrote:

quoted

"Paul E. McKenney" [off-list ref] writes:

[...]

quoted

quoted

quoted

The two options I see are:

1.	Rip tracing out of the inner idle loops and everything that
	they invoke.

What I suggested above.  But as I said I know sh*t about that tracing 
implementation so that's an easy suggestion for me to make.

Works for me as well.  ;-)

While I must admit not having a better suggestion, I for one would vote
strongly against removing tracing from the idle path.

Being a PM developer and maintainer, much of the code I work on and
maintain happens to be run in the bowels of the idle path.  Not having
the ability to trace this code would be a major step backwards IMO.

How is it a step backwards if it is already broken. Obviously you
haven't actually used any tracing here because it doesn't work right
with things as is.

What exactly do you want to trace at this level. By the point you are in
this code, the path is somewhat known and problems you have are likely
h/w issues. If you are trying to go thru a very precise sequence of
saving cpu state and flushing caches, you don't want calls out to
tracing code that could very easily change the behavior.

It would be nice to understand a real example of how to hit this
problem. Why type of tracing? Are there other known examples?

Well, these are the tracepoints in question:

trace_power_start(), trace_cpu_idle(), and trace_power_end()

But this is only used by powertop, and I'm sure nobody would mind if you
rip these tracepoints out, causing powertop to no longer work. (that's
why we have 4 bytes per tracepoint wasting space in every trace event).

Anyway, one answer is (and I was talking with Paul about this on IRC) is
that we can create a special "TRACE_EVENT_IDLE()" that will explicitly
call "rcu_idle_exit/enter()" as it expects to be called with it off.

This should solve most issues I believe.

-- Steve

On Thu, Feb 02, 2012 at 06:03:39PM -0500, Steven Rostedt wrote:

[ . . . ]

Anyway, one answer is (and I was talking with Paul about this on IRC) is
that we can create a special "TRACE_EVENT_IDLE()" that will explicitly
call "rcu_idle_exit/enter()" as it expects to be called with it off.

This should solve most issues I believe.

You OK with something like RCU_NONIDLE() rather than RCU_EVENT_IDLE()?
I have this funny feeling that tracing won't be the only thing using
RCU from idle.  :-/

Something like this, perhaps?

	#define RCU_NONIDLE(a) \
		do { \
			rcu_idle_exit(); \
			do { a; } while (0); \
			rcu_idle_enter(); \
		}

							Thanx, Paul

On 02/02/2012 05:03 PM, Steven Rostedt wrote:

On Thu, 2012-02-02 at 16:49 -0600, Rob Herring wrote:

quoted

On 02/02/2012 04:20 PM, Kevin Hilman wrote:

quoted

"Paul E. McKenney" [off-list ref] writes:

[...]

quoted

quoted

quoted

The two options I see are:

1.	Rip tracing out of the inner idle loops and everything that
	they invoke.

What I suggested above.  But as I said I know sh*t about that tracing 
implementation so that's an easy suggestion for me to make.

Works for me as well.  ;-)

While I must admit not having a better suggestion, I for one would vote
strongly against removing tracing from the idle path.

Being a PM developer and maintainer, much of the code I work on and
maintain happens to be run in the bowels of the idle path.  Not having
the ability to trace this code would be a major step backwards IMO.

How is it a step backwards if it is already broken. Obviously you
haven't actually used any tracing here because it doesn't work right
with things as is.

What exactly do you want to trace at this level. By the point you are in
this code, the path is somewhat known and problems you have are likely
h/w issues. If you are trying to go thru a very precise sequence of
saving cpu state and flushing caches, you don't want calls out to
tracing code that could very easily change the behavior.

It would be nice to understand a real example of how to hit this
problem. Why type of tracing? Are there other known examples?

Well, these are the tracepoints in question:

trace_power_start(), trace_cpu_idle(), and trace_power_end()

But this is only used by powertop, and I'm sure nobody would mind if you
rip these tracepoints out, causing powertop to no longer work. (that's
why we have 4 bytes per tracepoint wasting space in every trace event).

Anyway, one answer is (and I was talking with Paul about this on IRC) is
that we can create a special "TRACE_EVENT_IDLE()" that will explicitly
call "rcu_idle_exit/enter()" as it expects to be called with it off.

This should solve most issues I believe.

Seems like these could all be placed common cpuidle code. If you're not
using cpuidle, then you probably aren't doing much in the default idle
code. You would still need the wrappers within the cpuidle code or push
rcu_idle_exit/enter down into a common default idle (which I think we
now have with Nico's clean-up).

Rob

On Thu, Feb 02, 2012 at 03:27:36PM -0800, Paul E. McKenney wrote:

On Thu, Feb 02, 2012 at 06:03:39PM -0500, Steven Rostedt wrote:

[ . . . ]

quoted

Anyway, one answer is (and I was talking with Paul about this on IRC) is
that we can create a special "TRACE_EVENT_IDLE()" that will explicitly
call "rcu_idle_exit/enter()" as it expects to be called with it off.

This should solve most issues I believe.

You OK with something like RCU_NONIDLE() rather than RCU_EVENT_IDLE()?
I have this funny feeling that tracing won't be the only thing using
RCU from idle.  :-/

Something like this, perhaps?

Or perhaps with the trailing "while (0)" included:

	#define RCU_NONIDLE(a) \
		do { \
			rcu_idle_exit(); \
			do { a; } while (0); \
			rcu_idle_enter(); \
		} while (0)

							Thanx, Paul

On Thu, 2012-02-02 at 15:27 -0800, Paul E. McKenney wrote:

On Thu, Feb 02, 2012 at 06:03:39PM -0500, Steven Rostedt wrote:

[ . . . ]

quoted

Anyway, one answer is (and I was talking with Paul about this on IRC) is
that we can create a special "TRACE_EVENT_IDLE()" that will explicitly
call "rcu_idle_exit/enter()" as it expects to be called with it off.

This should solve most issues I believe.

You OK with something like RCU_NONIDLE() rather than RCU_EVENT_IDLE()?
I have this funny feeling that tracing won't be the only thing using
RCU from idle.  :-/

Something like this, perhaps?

	#define RCU_NONIDLE(a) \
		do { \
			rcu_idle_exit(); \
			do { a; } while (0); \
			rcu_idle_enter(); \
		}

I'm fine with this. But what is the overhead for doing such a thing.
Remember, to encapsulate a tracepoint means that you will be doing that
work everytime regardless if the tracepoint is ever activated or not. Or
even worse, not even compiled into the kernel.

Now this is entering and exiting idle, so maybe it's not that critical?

-- Steve

On Thu, Feb 02, 2012 at 09:45:31PM -0500, Steven Rostedt wrote:

On Thu, 2012-02-02 at 15:27 -0800, Paul E. McKenney wrote:

quoted

On Thu, Feb 02, 2012 at 06:03:39PM -0500, Steven Rostedt wrote:

[ . . . ]

quoted

Anyway, one answer is (and I was talking with Paul about this on IRC) is
that we can create a special "TRACE_EVENT_IDLE()" that will explicitly
call "rcu_idle_exit/enter()" as it expects to be called with it off.

This should solve most issues I believe.

You OK with something like RCU_NONIDLE() rather than RCU_EVENT_IDLE()?
I have this funny feeling that tracing won't be the only thing using
RCU from idle.  :-/

Something like this, perhaps?

	#define RCU_NONIDLE(a) \
		do { \
			rcu_idle_exit(); \
			do { a; } while (0); \
			rcu_idle_enter(); \
		}

I'm fine with this. But what is the overhead for doing such a thing.
Remember, to encapsulate a tracepoint means that you will be doing that
work everytime regardless if the tracepoint is ever activated or not. Or
even worse, not even compiled into the kernel.

Now this is entering and exiting idle, so maybe it's not that critical?

It is an atomic instruction or two, plus some memory barriers.  Entering
idle is more heavyweight for RCU_FAST_NO_HZ.  But as you say, it is
entering and exiting idle.

But should I make an empty definition of RCU_NONIDLE() for some #define
or another?

	#ifdef CONFIG_YOU_TELL_ME
	#define RCU_NONIDLE(a) \
		do { \
			rcu_idle_exit(); \
			do { a; } while (0); \
			rcu_idle_enter(); \
		} while (0)
	#else /* #ifdef CONFIG_YOU_TELL_ME */
	#define RCU_NONIDLE(a) do { } while (0);
	#endif /* #else #ifdef CONFIG_YOU_TELL_ME */

Or is event tracing unconditional these days?

							Thanx, Paul

On Thu, 2012-02-02 at 22:04 -0800, Paul E. McKenney wrote:

On Thu, Feb 02, 2012 at 09:45:31PM -0500, Steven Rostedt wrote:

It is an atomic instruction or two, plus some memory barriers.  Entering
idle is more heavyweight for RCU_FAST_NO_HZ.  But as you say, it is
entering and exiting idle.

But should I make an empty definition of RCU_NONIDLE() for some #define
or another?

	#ifdef CONFIG_YOU_TELL_ME
	#define RCU_NONIDLE(a) \
		do { \
			rcu_idle_exit(); \
			do { a; } while (0); \
			rcu_idle_enter(); \
		} while (0)
	#else /* #ifdef CONFIG_YOU_TELL_ME */
	#define RCU_NONIDLE(a) do { } while (0);
	#endif /* #else #ifdef CONFIG_YOU_TELL_ME */

Or is event tracing unconditional these days?

I don't like it. As it binds the RCU_NONIDLE to tracepoints only without
any annotation that they are bound. Still doesn't help when tracepoints
are configured but not enabled.

I have no problem in making a special TRACE_EVENT_IDLE() that does this
inside the jump label. Basically what we have today is:


	if (static_branch(tracepoint_key)) {
		rcu_read_lock_sched_notrace();
		for (all attached tracepoints) {
			[...]
		}
		rcu_read_unlock_sched_notrace();
	}

Ideally we want the enter/exit idle inside that static_branch()
condition:

	if (static_branch(tracepoint_key)) {
		rcu_idle_exit();
		rcu_read_lock_sched_notrace();
		for (all attached tracepoints) {
			[...]
		}
		rcu_read_unlock_sched_notrace();
		rcu_idle_enter();
	}

The static_branch() is the jump label code when it's a nop when disabled
and a jump to the tracing code when enabled:

	nop; /* or jmp 2f */  <<--- jump label
1:	[ normal code ]
	ret;

2:	[trace code]
	jmp 1b


The jump label when disabled is just a nop that ignores the trace code
(although current gcc has a bug that it currently doesn't do it this
elegantly). When tracing is enabled the nop is converted to a jump to
the tracing code. This makes tracepoints very light weight in hot paths.

Ideally, we want the exit/enter rcu idle with in the [trace code], which
makes it not used when not needed.

-- Steve

On Fri, Feb 03, 2012 at 01:55:09PM -0500, Steven Rostedt wrote:

On Thu, 2012-02-02 at 22:04 -0800, Paul E. McKenney wrote:

quoted

On Thu, Feb 02, 2012 at 09:45:31PM -0500, Steven Rostedt wrote:

quoted

It is an atomic instruction or two, plus some memory barriers.  Entering
idle is more heavyweight for RCU_FAST_NO_HZ.  But as you say, it is
entering and exiting idle.

But should I make an empty definition of RCU_NONIDLE() for some #define
or another?

	#ifdef CONFIG_YOU_TELL_ME
	#define RCU_NONIDLE(a) \
		do { \
			rcu_idle_exit(); \
			do { a; } while (0); \
			rcu_idle_enter(); \
		} while (0)
	#else /* #ifdef CONFIG_YOU_TELL_ME */
	#define RCU_NONIDLE(a) do { } while (0);
	#endif /* #else #ifdef CONFIG_YOU_TELL_ME */

Or is event tracing unconditional these days?

I don't like it. As it binds the RCU_NONIDLE to tracepoints only without
any annotation that they are bound. Still doesn't help when tracepoints
are configured but not enabled.

I have no problem in making a special TRACE_EVENT_IDLE() that does this
inside the jump label. Basically what we have today is:


	if (static_branch(tracepoint_key)) {
		rcu_read_lock_sched_notrace();
		for (all attached tracepoints) {
			[...]
		}
		rcu_read_unlock_sched_notrace();
	}

Ideally we want the enter/exit idle inside that static_branch()
condition:

	if (static_branch(tracepoint_key)) {
		rcu_idle_exit();
		rcu_read_lock_sched_notrace();
		for (all attached tracepoints) {
			[...]
		}
		rcu_read_unlock_sched_notrace();
		rcu_idle_enter();
	}

The static_branch() is the jump label code when it's a nop when disabled
and a jump to the tracing code when enabled:

	nop; /* or jmp 2f */  <<--- jump label
1:	[ normal code ]
	ret;

2:	[trace code]
	jmp 1b


The jump label when disabled is just a nop that ignores the trace code
(although current gcc has a bug that it currently doesn't do it this
elegantly). When tracing is enabled the nop is converted to a jump to
the tracing code. This makes tracepoints very light weight in hot paths.

Ideally, we want the exit/enter rcu idle with in the [trace code], which
makes it not used when not needed.

So the idea is that if you have a trace event that is to be used in idle,
you use TRACE_EVENT_IDLE() rather than TRACE_EVENT() to declare that
trace event?  That would work for me, and might make for fewer changes
for the architecture guys.  Also, this should address the code-size
concerns we discussed yesterday.

So sounds good!

Is a DEFINE_EVENT_IDLE() also needed?  Or prehaps a
DECLARE_EVENT_CLASS_IDLE()?  My guess is "yes" for at least one of the
two based on include/trace/events/power.h.

I will keep RCU_NONIDLE() for at least a little while (reworking comments
to point out TRACE_EVENT_IDLE() and friends) in case there turn out to
be non-tracepoint uses of RCU in the idle loop.

							Thanx, Paul

On Fri, 2012-02-03 at 11:40 -0800, Paul E. McKenney wrote:

So the idea is that if you have a trace event that is to be used in idle,
you use TRACE_EVENT_IDLE() rather than TRACE_EVENT() to declare that
trace event?  That would work for me, and might make for fewer changes
for the architecture guys.  Also, this should address the code-size
concerns we discussed yesterday.

So sounds good!

Is a DEFINE_EVENT_IDLE() also needed?  Or prehaps a
DECLARE_EVENT_CLASS_IDLE()?  My guess is "yes" for at least one of the
two based on include/trace/events/power.h.

I'll have to take a look. I may even find a better way to do this too.

I will keep RCU_NONIDLE() for at least a little while (reworking comments
to point out TRACE_EVENT_IDLE() and friends) in case there turn out to
be non-tracepoint uses of RCU in the idle loop.

OK, I'll take a crack at this next Monday.

-- Steve

On Fri, Feb 03, 2012 at 03:02:55PM -0500, Steven Rostedt wrote:

On Fri, 2012-02-03 at 11:40 -0800, Paul E. McKenney wrote:

quoted

So the idea is that if you have a trace event that is to be used in idle,
you use TRACE_EVENT_IDLE() rather than TRACE_EVENT() to declare that
trace event?  That would work for me, and might make for fewer changes
for the architecture guys.  Also, this should address the code-size
concerns we discussed yesterday.

So sounds good!

Is a DEFINE_EVENT_IDLE() also needed?  Or prehaps a
DECLARE_EVENT_CLASS_IDLE()?  My guess is "yes" for at least one of the
two based on include/trace/events/power.h.

I'll have to take a look. I may even find a better way to do this too.

Better is always better.  ;-)

quoted

I will keep RCU_NONIDLE() for at least a little while (reworking comments
to point out TRACE_EVENT_IDLE() and friends) in case there turn out to
be non-tracepoint uses of RCU in the idle loop.

OK, I'll take a crack at this next Monday.

Sounds good!

I plan to push my stack to -next later today, but will yank my cpuidle
commit as soon as your approach is available.  They do not conflict
because the rcu_idle_enter()s nest.

							Thanx, Paul

As I have said, I may find a better solution than to create a
TRACE_EVENT_IDLE(), and I believe I have :-)

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

Ideally, this patch would be broken up into two commits. The first would
change the tracepoint.h to introduce the new trace_foobar_rcuidle()
static inline, and the second patch would change the power tracepoints
to use this tracepoint function. For the RFC, I'm keeping it as a single
patch.

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.

-- Steve

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Index: linux-trace.git/include/linux/tracepoint.h
===================================================================

--- linux-trace.git.orig/include/linux/tracepoint.h
+++ linux-trace.git/include/linux/tracepoint.h

@@ -114,7 +114,7 @@ static inline void tracepoint_synchroniz
  * 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, pre, post)			\
 	do {								\
 		struct tracepoint_func *it_func_ptr;			\
 		void *it_func;						\

@@ -123,6 +123,7 @@ static inline void tracepoint_synchroniz
 		if (!(cond))						\
 			return;						\
 		rcu_read_lock_sched_notrace();				\
+		pre;							\
 		it_func_ptr = rcu_dereference_sched((tp)->funcs);	\
 		if (it_func_ptr) {					\
 			do {						\

@@ -132,6 +133,7 @@ static inline void tracepoint_synchroniz
 			} while ((++it_func_ptr)->func);		\
 		}							\
 		rcu_read_unlock_sched_notrace();			\
+		post;							\
 	} while (0)
 
 /*

@@ -139,7 +141,7 @@ static inline void tracepoint_synchroniz
  * 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_synchroniz
 			__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_synchroniz
 	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						\

Index: linux-trace.git/arch/x86/kernel/process.c
===================================================================

--- linux-trace.git.orig/arch/x86/kernel/process.c
+++ linux-trace.git/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,7 +391,7 @@ void default_idle(void)
 		else
 			local_irq_enable();
 		current_thread_info()->status |= TS_POLLING;
-		trace_power_end(smp_processor_id());
+		trace_power_end_rcuidle(smp_processor_id());
 		trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
 	} else {
 		local_irq_enable();

@@ -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());
 }
 
 /*

On Mon, Feb 06, 2012 at 04:18:33PM -0500, Steven Rostedt wrote:

As I have said, I may find a better solution than to create a
TRACE_EVENT_IDLE(), and I believe I have :-)

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

Ideally, this patch would be broken up into two commits. The first would
change the tracepoint.h to introduce the new trace_foobar_rcuidle()
static inline, and the second patch would change the power tracepoints
to use this tracepoint function. For the RFC, I'm keeping it as a single
patch.

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.

-- Steve

Aside from the "pre;" below needing to precede rcu_read_lock_sched_notrace()
as Josh noted, this looks reasonable to me.

							Thanx, Paul

On Mon, 2012-02-06 at 15:38 -0800, Paul E. McKenney wrote:

Aside from the "pre;" below needing to precede rcu_read_lock_sched_notrace()
as Josh noted, this looks reasonable to me.

Does this mean I can add an Acked-by from you and Josh? Obviously with
the post "pre" change.

-- Steve

On Tue, Feb 07, 2012 at 07:32:56AM -0500, Steven Rostedt wrote:

On Mon, 2012-02-06 at 15:38 -0800, Paul E. McKenney wrote:

quoted

Aside from the "pre;" below needing to precede rcu_read_lock_sched_notrace()
as Josh noted, this looks reasonable to me.

Does this mean I can add an Acked-by from you and Josh? Obviously with
the post "pre" change.

With the "pre" change,

Acked-by: Paul E. McKenney <redacted>

							Thanx, Paul

On Tue, Feb 07, 2012 at 07:32:56AM -0500, Steven Rostedt wrote:

On Mon, 2012-02-06 at 15:38 -0800, Paul E. McKenney wrote:

quoted

Aside from the "pre;" below needing to precede rcu_read_lock_sched_notrace()
as Josh noted, this looks reasonable to me.

Does this mean I can add an Acked-by from you and Josh? Obviously with
the post "pre" change.

With the pre change:

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

On Tue, Feb 07, 2012 at 06:11:42AM -0800, Paul E. McKenney wrote:

On Tue, Feb 07, 2012 at 07:32:56AM -0500, Steven Rostedt wrote:

quoted

On Mon, 2012-02-06 at 15:38 -0800, Paul E. McKenney wrote:

quoted

Aside from the "pre;" below needing to precede rcu_read_lock_sched_notrace()
as Josh noted, this looks reasonable to me.

Does this mean I can add an Acked-by from you and Josh? Obviously with
the post "pre" change.

With the "pre" change,

Acked-by: Paul E. McKenney <redacted>

Same for me! Good idea.

Acked-by: Frederic Weisbecker <redacted>