[PATCH] cpuidle/idle: move idle traces to cpuidle_enter_state

[PATCH] cpuidle/idle: move idle traces to cpuidle_enter_state

[Sandeep Tripathy]

 idle_exit event is the first event after a core exits
idle state. So this should be traced before local irq
is ebabled. Likewise idle_entry is the last event before
a core enters idle state. This will ease visualising the
cpu idle state from kernel traces.

Signed-off-by: Sandeep Tripathy <sandeep.tripathy@linaro.org>
---
 drivers/cpuidle/cpuidle.c | 3 +++
 kernel/sched/idle.c       | 4 ----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 8236746..97680d0 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -99,12 +99,15 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 	ktime_t time_start, time_end;
 	s64 diff;
 
+	trace_cpu_idle_rcuidle(index, dev->cpu);
 	time_start = ktime_get();
 
 	entered_state = target_state->enter(dev, drv, index);
 
 	time_end = ktime_get();
 
+	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
+
 	if (!cpuidle_state_is_coupled(dev, drv, entered_state))
 		local_irq_enable();
 
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 8f4390a..07c446a 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -141,7 +141,6 @@ static int cpuidle_idle_call(void)
 					&dev->cpu);
 
 			if (!ret) {
-				trace_cpu_idle_rcuidle(next_state, dev->cpu);
 
 				/*
 				 * Enter the idle state previously
@@ -154,9 +153,6 @@ static int cpuidle_idle_call(void)
 				entered_state = cpuidle_enter(drv, dev,
 							      next_state);
 
-				trace_cpu_idle_rcuidle(PWR_EVENT_EXIT,
-						       dev->cpu);
-
 				if (broadcast)
 					clockevents_notify(
 						CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
-- 
1.9.1

Re: [PATCH] cpuidle/idle: move idle traces to cpuidle_enter_state

[Daniel Lezcano]

On 07/02/2014 11:30 AM, Sandeep Tripathy wrote:
>   idle_exit event is the first event after a core exits
> idle state. So this should be traced before local irq
> is ebabled. Likewise idle_entry is the last event before
> a core enters idle state. This will ease visualising the
> cpu idle state from kernel traces.
>
> Signed-off-by: Sandeep Tripathy <sandeep.tripathy@linaro.org>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---
>   drivers/cpuidle/cpuidle.c | 3 +++
>   kernel/sched/idle.c       | 4 ----
>   2 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 8236746..97680d0 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -99,12 +99,15 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>   	ktime_t time_start, time_end;
>   	s64 diff;
>
> +	trace_cpu_idle_rcuidle(index, dev->cpu);
>   	time_start = ktime_get();
>
>   	entered_state = target_state->enter(dev, drv, index);
>
>   	time_end = ktime_get();
>
> +	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> +
>   	if (!cpuidle_state_is_coupled(dev, drv, entered_state))
>   		local_irq_enable();
>
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index 8f4390a..07c446a 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -141,7 +141,6 @@ static int cpuidle_idle_call(void)
>   					&dev->cpu);
>
>   			if (!ret) {
> -				trace_cpu_idle_rcuidle(next_state, dev->cpu);
>
>   				/*
>   				 * Enter the idle state previously
> @@ -154,9 +153,6 @@ static int cpuidle_idle_call(void)
>   				entered_state = cpuidle_enter(drv, dev,
>   							      next_state);
>
> -				trace_cpu_idle_rcuidle(PWR_EVENT_EXIT,
> -						       dev->cpu);
> -
>   				if (broadcast)
>   					clockevents_notify(
>   						CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] cpuidle/idle: move idle traces to cpuidle_enter_state

[Rafael J. Wysocki]

On Saturday, July 05, 2014 05:45:24 PM Daniel Lezcano wrote:
> On 07/02/2014 11:30 AM, Sandeep Tripathy wrote:
> >   idle_exit event is the first event after a core exits
> > idle state. So this should be traced before local irq
> > is ebabled. Likewise idle_entry is the last event before
> > a core enters idle state. This will ease visualising the
> > cpu idle state from kernel traces.
> >
> > Signed-off-by: Sandeep Tripathy <sandeep.tripathy@linaro.org>
> 
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

I'm going to take this for 3.17.

Peter, will there be any problems with it if I take it?

Rafael


> > ---
> >   drivers/cpuidle/cpuidle.c | 3 +++
> >   kernel/sched/idle.c       | 4 ----
> >   2 files changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > index 8236746..97680d0 100644
> > --- a/drivers/cpuidle/cpuidle.c
> > +++ b/drivers/cpuidle/cpuidle.c
> > @@ -99,12 +99,15 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
> >   	ktime_t time_start, time_end;
> >   	s64 diff;
> >
> > +	trace_cpu_idle_rcuidle(index, dev->cpu);
> >   	time_start = ktime_get();
> >
> >   	entered_state = target_state->enter(dev, drv, index);
> >
> >   	time_end = ktime_get();
> >
> > +	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> > +
> >   	if (!cpuidle_state_is_coupled(dev, drv, entered_state))
> >   		local_irq_enable();
> >
> > diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> > index 8f4390a..07c446a 100644
> > --- a/kernel/sched/idle.c
> > +++ b/kernel/sched/idle.c
> > @@ -141,7 +141,6 @@ static int cpuidle_idle_call(void)
> >   					&dev->cpu);
> >
> >   			if (!ret) {
> > -				trace_cpu_idle_rcuidle(next_state, dev->cpu);
> >
> >   				/*
> >   				 * Enter the idle state previously
> > @@ -154,9 +153,6 @@ static int cpuidle_idle_call(void)
> >   				entered_state = cpuidle_enter(drv, dev,
> >   							      next_state);
> >
> > -				trace_cpu_idle_rcuidle(PWR_EVENT_EXIT,
> > -						       dev->cpu);
> > -
> >   				if (broadcast)
> >   					clockevents_notify(
> >   						CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
> >
> 
> 
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] cpuidle/idle: move idle traces to cpuidle_enter_state

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 2048 bytes --]

On Mon, Jul 14, 2014 at 09:47:46PM +0200, Rafael J. Wysocki wrote:
> On Saturday, July 05, 2014 05:45:24 PM Daniel Lezcano wrote:
> > On 07/02/2014 11:30 AM, Sandeep Tripathy wrote:
> > >   idle_exit event is the first event after a core exits
> > > idle state. 

This

> > > So this should be traced before local irq
> > > is ebabled. 

does not follow, interrupt state does not correlate to events or not.

> > > Likewise idle_entry is the last event before
> > > a core enters idle state. This will ease visualising the
> > > cpu idle state from kernel traces.
> > >
> > > Signed-off-by: Sandeep Tripathy <sandeep.tripathy@linaro.org>
> > 
> > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> I'm going to take this for 3.17.
> 
> Peter, will there be any problems with it if I take it?

don't think so, if there's anything, I'll fix it up.

> > > ---
> > >   drivers/cpuidle/cpuidle.c | 3 +++
> > >   kernel/sched/idle.c       | 4 ----
> > >   2 files changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > > index 8236746..97680d0 100644
> > > --- a/drivers/cpuidle/cpuidle.c
> > > +++ b/drivers/cpuidle/cpuidle.c
> > > @@ -99,12 +99,15 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
> > >   	ktime_t time_start, time_end;
> > >   	s64 diff;
> > >
> > > +	trace_cpu_idle_rcuidle(index, dev->cpu);
> > >   	time_start = ktime_get();
> > >
> > >   	entered_state = target_state->enter(dev, drv, index);
> > >
> > >   	time_end = ktime_get();
> > >
> > > +	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);

If someone were to instrument ktime_get() the changelog is obviously
fail. Now I appreciate that instrumenting the timing infrastructure
might be challenging .. :-)

And I suppose the 'assumption' is that the target_state->enter()
function does not entail 'tracing' ? Is it made sure that these
functions do not generate __mcount or other function tracer stubs etc. ?

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] cpuidle/idle: move idle traces to cpuidle_enter_state

[Steven Rostedt]

On Mon, 14 Jul 2014 22:07:16 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> And I suppose the 'assumption' is that the target_state->enter()
> function does not entail 'tracing' ? Is it made sure that these
> functions do not generate __mcount or other function tracer stubs etc. ?

Lets not confuse events with function tracing. Function tracing has its
own infrastructure and is made to trace details of things like
target_state->enter() and such. And function tracing can trace even
outside of rcu scope, which trace events do not.

When people want to trace events, that assumption is just trace events,
not function tracing.

-- Steve