cpuidle/idle: move idle traces to cpuidle_enter_state

Message ID 1404293458-9799-1-git-send-email-sandeep.tripathy@linaro.org
State New
Headers show

Commit Message

sandeep.tripathy@linaro.org July 2, 2014, 9:30 a.m.
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(-)

Comments

Daniel Lezcano July 5, 2014, 3:45 p.m. | #1
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,
>
Rafael J. Wysocki July 14, 2014, 7:47 p.m. | #2
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,
> >
> 
> 
>
Peter Zijlstra July 14, 2014, 8:07 p.m. | #3
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. ?
Steven Rostedt July 14, 2014, 9 p.m. | #4
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

Patch

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,