[PATCH 1/9] acpi: Use ACPI C-state type instead of enumeration value to export cpuidle state name

[PATCH 1/9] acpi: Use ACPI C-state type instead of enumeration value to export cpuidle state name

[Thomas Renninger]

In the former /proc/acpi/processor/power/* there were Cx showing the
enumerated number/amount of C-states and type[Cy] which is
what should get shown as the cpuidle state name.

Typically on latest Nehalem and later CPUs, BIOS vendors miss
out C2 and C3 wrongly shows up as C2.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: arjan@linux.intel.com
CC: lenb@kernel.org
CC: linux-acpi@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/acpi/processor_idle.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index dcb38f8..104ae77 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1008,7 +1008,6 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
 #endif
 		cpuidle_set_statedata(state, cx);
 
-		snprintf(state->name, CPUIDLE_NAME_LEN, "C%d", i);
 		strncpy(state->desc, cx->desc, CPUIDLE_DESC_LEN);
 		state->exit_latency = cx->latency;
 		state->target_residency = cx->latency * latency_factor;
@@ -1016,6 +1015,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
 		state->flags = 0;
 		switch (cx->type) {
 			case ACPI_STATE_C1:
+			snprintf(state->name, CPUIDLE_NAME_LEN, "C1");
 			state->flags |= CPUIDLE_FLAG_SHALLOW;
 			if (cx->entry_method == ACPI_CSTATE_FFH)
 				state->flags |= CPUIDLE_FLAG_TIME_VALID;
@@ -1025,6 +1025,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
 			break;
 
 			case ACPI_STATE_C2:
+			snprintf(state->name, CPUIDLE_NAME_LEN, "C2");
 			state->flags |= CPUIDLE_FLAG_BALANCED;
 			state->flags |= CPUIDLE_FLAG_TIME_VALID;
 			state->enter = acpi_idle_enter_simple;
@@ -1032,6 +1033,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
 			break;
 
 			case ACPI_STATE_C3:
+			snprintf(state->name, CPUIDLE_NAME_LEN, "C3");
 			state->flags |= CPUIDLE_FLAG_DEEP;
 			state->flags |= CPUIDLE_FLAG_TIME_VALID;
 			state->flags |= CPUIDLE_FLAG_CHECK_BM;
-- 
1.7.3.1

Re: [PATCH 1/9] acpi: Use ACPI C-state type instead of enumeration value to export cpuidle state name

[Len Brown]

On Fri, 7 Jan 2011, Thomas Renninger wrote:

> In the former /proc/acpi/processor/power/* there were Cx showing the
> enumerated number/amount of C-states and type[Cy] which is
> what should get shown as the cpuidle state name.
> 
> Typically on latest Nehalem and later CPUs, BIOS vendors miss
> out C2 and C3 wrongly shows up as C2.

I think this patch will cause more confusion than it will fix.

It assumes that all states of type C2 should have the name "C2"
and all the states of type "C3" should have the name "C3".
But some systems may have more than one state of each type...

also, the state->name is somewhat arbitrary.

You'll notice that intel_idle uses the hardware C-state names
such as NHM-C1, NHM-C3, NHM-C6 - to match the (arbitrary)
names in the hardware documentation.  We could call those
states Moe/Larry/Curley just as well.

If somebody wants to know what the state _is_,
then the state->desc field shows them in un-ambiguous
terms, "MWAIT 0x10" etc.

if somebody wants to simply enumerate the states,
so they can use acpi.max_cstate=N, for example, then
cpuidle already enumerates them in 
/sys/devices/system/cpu/cpu0/cpuidle/state%d

thanks,
Len Brown, Intel Open Source Technology Center

ps. the thing about "mising C2".  That isn't a bug.
That is actually BIOS writer's attempt to not break
the installed base of Linux.  NHM doesn't actually
have any ACPI C3-type states, just C2-type states.
But in NHM, the LAPIC timer stops in C2-type states,
so the BIOS advertises them as C2-type to make sure
that an old version of Linux doesn't attempt to
use the LAPIC timer in those states.
(we switched to not trusting the LAPIC timer even
in C2-type states a while back, but the installed
base never gets updated)

pps. I do think we need to be able to dump
the _CST, as we lost the ACPI type info that
was in /proc/acpi/processor/*/power and we
may need it to debug some systems.

> Signed-off-by: Thomas Renninger <trenn@suse.de>
> CC: arjan@linux.intel.com
> CC: lenb@kernel.org
> CC: linux-acpi@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  drivers/acpi/processor_idle.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index dcb38f8..104ae77 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -1008,7 +1008,6 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
>  #endif
>  		cpuidle_set_statedata(state, cx);
>  
> -		snprintf(state->name, CPUIDLE_NAME_LEN, "C%d", i);
>  		strncpy(state->desc, cx->desc, CPUIDLE_DESC_LEN);
>  		state->exit_latency = cx->latency;
>  		state->target_residency = cx->latency * latency_factor;
> @@ -1016,6 +1015,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
>  		state->flags = 0;
>  		switch (cx->type) {
>  			case ACPI_STATE_C1:
> +			snprintf(state->name, CPUIDLE_NAME_LEN, "C1");
>  			state->flags |= CPUIDLE_FLAG_SHALLOW;
>  			if (cx->entry_method == ACPI_CSTATE_FFH)
>  				state->flags |= CPUIDLE_FLAG_TIME_VALID;
> @@ -1025,6 +1025,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
>  			break;
>  
>  			case ACPI_STATE_C2:
> +			snprintf(state->name, CPUIDLE_NAME_LEN, "C2");
>  			state->flags |= CPUIDLE_FLAG_BALANCED;
>  			state->flags |= CPUIDLE_FLAG_TIME_VALID;
>  			state->enter = acpi_idle_enter_simple;
> @@ -1032,6 +1033,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
>  			break;
>  
>  			case ACPI_STATE_C3:
> +			snprintf(state->name, CPUIDLE_NAME_LEN, "C3");
>  			state->flags |= CPUIDLE_FLAG_DEEP;
>  			state->flags |= CPUIDLE_FLAG_TIME_VALID;
>  			state->flags |= CPUIDLE_FLAG_CHECK_BM;
> -- 
> 1.7.3.1
> 

Re: [PATCH 1/9] acpi: Use ACPI C-state type instead of enumeration value to export cpuidle state name

[Thomas Renninger]

On Friday 07 January 2011 21:45:18 Len Brown wrote:
> On Fri, 7 Jan 2011, Thomas Renninger wrote:
> 
> > In the former /proc/acpi/processor/power/* there were Cx showing the
> > enumerated number/amount of C-states and type[Cy] which is
> > what should get shown as the cpuidle state name.
> > 
> > Typically on latest Nehalem and later CPUs, BIOS vendors miss
> > out C2 and C3 wrongly shows up as C2.
> 
> I think this patch will cause more confusion than it will fix.
> 
> It assumes that all states of type C2 should have the name "C2"
> and all the states of type "C3" should have the name "C3".
Right.
> But some systems may have more than one state of each type...
Exported through ACPI tables?
 
> also, the state->name is somewhat arbitrary.
> 
> You'll notice that intel_idle uses the hardware C-state names
> such as NHM-C1, NHM-C3, NHM-C6 - to match the (arbitrary)
> names in the hardware documentation.  We could call those
> states Moe/Larry/Curley just as well.
That works out with HW specific intel_idle.c driver, but not
with the generic acpi/processor one.

> If somebody wants to know what the state _is_,
> then the state->desc field shows them in un-ambiguous
> terms, "MWAIT 0x10" etc.
Right, people can/should use it for additional info.

> if somebody wants to simply enumerate the states,
> so they can use acpi.max_cstate=N, for example, then
> cpuidle already enumerates them in 
> /sys/devices/system/cpu/cpu0/cpuidle/state%d
Not sure I get this.
Currently you have:
/sys/../stateX/...
/sys/devices/system/cpu/cpu0/cpuidle/state%d/name
and stateX (the directory name) and stateX/name contain
duplicate info:
X and CX.
in acpi cpuidle driver case.
While the C-state type as exported by BIOS may not always
be correct, it contains more info as the current approach, which
has zero info.

The other ACPI related patches are ok?
What do you think of cpu_idle events thrown as real cpuidle
subsystem events to make them architecture independent and
break up the unnecessary complexity (intel_idle throws the
idle start and cpuidle the end event, etc.).

> thanks,
> Len Brown, Intel Open Source Technology Center
> 
> ps. the thing about "mising C2".  That isn't a bug.
> That is actually BIOS writer's attempt to not break
> the installed base of Linux.  NHM doesn't actually
> have any ACPI C3-type states, just C2-type states.
> But in NHM, the LAPIC timer stops in C2-type states,
> so the BIOS advertises them as C2-type to make sure
> that an old version of Linux doesn't attempt to
> use the LAPIC timer in those states.
> (we switched to not trusting the LAPIC timer even
> in C2-type states a while back, but the installed
> base never gets updated)
> 
> pps. I do think we need to be able to dump
> the _CST, as we lost the ACPI type info that
> was in /proc/acpi/processor/*/power and we
> may need it to debug some systems.
?
I restore the ACPI type info as exported by ACPI tables
with this patch, should be the same as done with
/proc/acpi/processor/*/power
Do I miss something?
Do you want to export additional ACPI table C-state
info?

Something else, but related:
You recently said that it might be a good idea to get
C-/P- state ACPI tables which are often in a separate
SSDT loaded at runtime via "load" ACPI command, dumped
with the acpidump  tool. This would be a cool feature.
Does dynamically loaded SSDT dumping
via acpidump work already or is/has someone looked at this?

Thanks,

     Thomas

> 
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > CC: arjan@linux.intel.com
> > CC: lenb@kernel.org
> > CC: linux-acpi@vger.kernel.org
> > CC: linux-kernel@vger.kernel.org
> > ---
> >  drivers/acpi/processor_idle.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> > index dcb38f8..104ae77 100644
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -1008,7 +1008,6 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
> >  #endif
> >  		cpuidle_set_statedata(state, cx);
> >  
> > -		snprintf(state->name, CPUIDLE_NAME_LEN, "C%d", i);
> >  		strncpy(state->desc, cx->desc, CPUIDLE_DESC_LEN);
> >  		state->exit_latency = cx->latency;
> >  		state->target_residency = cx->latency * latency_factor;
> > @@ -1016,6 +1015,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
> >  		state->flags = 0;
> >  		switch (cx->type) {
> >  			case ACPI_STATE_C1:
> > +			snprintf(state->name, CPUIDLE_NAME_LEN, "C1");
> >  			state->flags |= CPUIDLE_FLAG_SHALLOW;
> >  			if (cx->entry_method == ACPI_CSTATE_FFH)
> >  				state->flags |= CPUIDLE_FLAG_TIME_VALID;
> > @@ -1025,6 +1025,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
> >  			break;
> >  
> >  			case ACPI_STATE_C2:
> > +			snprintf(state->name, CPUIDLE_NAME_LEN, "C2");
> >  			state->flags |= CPUIDLE_FLAG_BALANCED;
> >  			state->flags |= CPUIDLE_FLAG_TIME_VALID;
> >  			state->enter = acpi_idle_enter_simple;
> > @@ -1032,6 +1033,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
> >  			break;
> >  
> >  			case ACPI_STATE_C3:
> > +			snprintf(state->name, CPUIDLE_NAME_LEN, "C3");
> >  			state->flags |= CPUIDLE_FLAG_DEEP;
> >  			state->flags |= CPUIDLE_FLAG_TIME_VALID;
> >  			state->flags |= CPUIDLE_FLAG_CHECK_BM;
> > -- 
> > 1.7.3.1
> > 
> 

Re: [PATCH 1/9] acpi: Use ACPI C-state type instead of enumeration value to export cpuidle state name

[Len Brown]

> > But some systems may have more than one state of each type...

> Exported through ACPI tables?

Yes.

It is quite common, for example in the Pentium M
generations, for a BIOS to export C3 and C4 -
both of which are ACPI  C3-type.

> > also, the state->name is somewhat arbitrary.
> > 
> > You'll notice that intel_idle uses the hardware C-state names
> > such as NHM-C1, NHM-C3, NHM-C6 - to match the (arbitrary)
> > names in the hardware documentation.  We could call those
> > states Moe/Larry/Curley just as well.

> That works out with HW specific intel_idle.c driver, but not
> with the generic acpi/processor one.

Actually, C0..Cn work out fine for the acpi/processor driver.
the names are unique, and they also correspond to max_cstate
if somebody wants to use that.


> I restore the ACPI type info as exported by ACPI tables
> with this patch, should be the same as done with
> /proc/acpi/processor/*/power
> Do I miss something?

/proc/acpi/processor/*/power had a type field.
Per the example above, the type field didn't always
match the C-state number.

> Do you want to export additional ACPI table C-state
> info?

I have a 1 line patch someplace to print out the type
for debug info when needed, that should be sufficient
for debugging, which is the only time we care about type.

> Something else, but related:
> You recently said that it might be a good idea to get
> C-/P- state ACPI tables which are often in a separate
> SSDT loaded at runtime via "load" ACPI command, dumped
> with the acpidump  tool. This would be a cool feature.
> Does dynamically loaded SSDT dumping
> via acpidump work already or is/has someone looked at this?

Yakui fixed this a while back.
acpidump in the latest pmtools in
http://userweb.kernel.org/~lenb/acpi/utils/
picks up the dynamic tables from /sys/firmware/acpi/tables.

cheers,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 1/9] acpi: Use ACPI C-state type instead of enumeration value to export cpuidle state name

[Thomas Renninger]

On Wednesday 12 January 2011 07:36:17 Len Brown wrote:
> > > But some systems may have more than one state of each type...
> 
> > Exported through ACPI tables?
> 
> Yes.
> 
> It is quite common, for example in the Pentium M
> generations, for a BIOS to export C3 and C4 -
> both of which are ACPI  C3-type.
Then you have two C3 types/names, this doesn't hurt.
It's even "more" correct than enumeration (expecting
that C2 is missing which can happen, right?):
C1 -> C1
C2 -> C3
C3 -> C4

As you already said, people have to look closer at
the description file anyway. There is not much we can/should
do about that in the generic acpi_idle driver (beside
a note in Documentation/...).
 
> > > also, the state->name is somewhat arbitrary.
> > > 
> > > You'll notice that intel_idle uses the hardware C-state names
> > > such as NHM-C1, NHM-C3, NHM-C6 - to match the (arbitrary)
> > > names in the hardware documentation.  We could call those
> > > states Moe/Larry/Curley just as well.
> 
> > That works out with HW specific intel_idle.c driver, but not
> > with the generic acpi/processor one.
> 
> Actually, C0..Cn work out fine for the acpi/processor driver.
> the names are unique, and they also correspond to max_cstate
> if somebody wants to use that.
I do not like the fact that the name contains zero info
and still is wrong and will still cause confusion
(enumerating from C0-Cx will also not match
C-states as described in documentations, but at least match
the type as exported by BIOS vendors).

if [ `cat /sys/devices/system/cpu/cpuidle/current_driver` = "acpi_idle" ];then
  for ((x=1; x<10; x++));do
    [ ! -d /sys/devices/system/cpu/cpu0/cpuidle/state${x} ] && break
    if [ `cat /sys/devices/system/cpu/cpu0/cpuidle/state${x}/name` = C${x} ];then
        echo "The same"
    fi
  done
fi
The same
The same
The same
Currently it will always be "The same".

> > I restore the ACPI type info as exported by ACPI tables
> > with this patch, should be the same as done with
> > /proc/acpi/processor/*/power
> > Do I miss something?
> 
> /proc/acpi/processor/*/power had a type field.
> Per the example above, the type field didn't always
> match the C-state number.
But compared to plain enumeration it has at least
some use (see your own comment below).
And if OEMs care (and some probably would) they can
export the type they like to see and it gets passed
from the BIOS tables up to the very high end
userspace/customer tools.

I know Intel uses intel_idle, but there are others
which make use of ACPI C-states.
 
> > Do you want to export additional ACPI table C-state
> > info?
> 
> I have a 1 line patch someplace to print out the type
> for debug info when needed, that should be sufficient
> for debugging, which is the only time we care about type.
To be honest I do not care about this detail that much.
I think it would be nicer to export the C-state type in
the name. Name and description is cpuidle driver (in this case
acpi_idle) specific info, why shouldn't processor_idle
fill it with something which has at least some meaning.

> > Something else, but related:
> > You recently said that it might be a good idea to get
> > C-/P- state ACPI tables which are often in a separate
> > SSDT loaded at runtime via "load" ACPI command, dumped
> > with the acpidump  tool. This would be a cool feature.
> > Does dynamically loaded SSDT dumping
> > via acpidump work already or is/has someone looked at this?
> 
> Yakui fixed this a while back.
> acpidump in the latest pmtools in
> http://userweb.kernel.org/~lenb/acpi/utils/
> picks up the dynamic tables from /sys/firmware/acpi/tables.
Great!
I didn't update acpidump for quite some time as I hoped it
shows up in the acpica project and also did some adjustance
for this already. I'll grab it for 11.4.

     Thomas

Re: [PATCH 1/9] acpi: Use ACPI C-state type instead of enumeration value to export cpuidle state name

[Len Brown]

Thomas,

I think the ACPI C-state names are fine the way they are.

I don't understand the value proposition you have presented
to replace the C-state names with ACPI C-state types.

I think this would be extremely confusing, particularly
since it would mean that multiple states can then appear
with duplicate names.

Thus I do not recommend changing the format of the name
that we have been exporting since cpuidle went
upstream 3 years ago.

thanks,
Len Brown, Intel Open Source Technology Center

[PATCH 2/9] cpuidle: Rename X86 specific idle poll state[0] from C0 to POLL

[Thomas Renninger]

C0 means and is well know as "not idle".
All documentation out there uses this term as "running"/"not idle"
state. Also Linux userspace tools (e.g. cpufreq-aperf and turbostat)
show C0 residency which there is correct, but means something totally
else than cpuidle "POLL" state.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: arjan@linux.intel.com
CC: lenb@kernel.org
CC: linux-acpi@vger.kernel.org
CC: Ingo Molnar <mingo@elte.hu>
CC: linux-kernel@vger.kernel.org
---
 drivers/cpuidle/cpuidle.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 08d5f05..99cc8fc 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -260,7 +260,7 @@ static void poll_idle_init(struct cpuidle_device *dev)
 
 	cpuidle_set_statedata(state, NULL);
 
-	snprintf(state->name, CPUIDLE_NAME_LEN, "C0");
+	snprintf(state->name, CPUIDLE_NAME_LEN, "POLL");
 	snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE");
 	state->exit_latency = 0;
 	state->target_residency = 0;
-- 
1.7.3.1

Re: [PATCH 2/9] cpuidle: Rename X86 specific idle poll state[0] from C0 to POLL

[Len Brown]

applied to idle-test

thanks,
Len Brown, Intel Open Source Technology Center

[PATCH 3/9] X86/perf: fix power:cpu_idle double end events and throw cpu_idle events from the cpuidle layer

[Thomas Renninger]

Currently intel_idle and acpi_idle driver show double cpu_idle "exit idle"
events -> this patch fixes it and makes cpu_idle events throwing less complex.

It also introduces cpu_idle events for all architectures which use
the cpuidle subsystem, namely:
  - arch/arm/mach-at91/cpuidle.c
  - arch/arm/mach-davinci/cpuidle.c
  - arch/arm/mach-kirkwood/cpuidle.c
  - arch/arm/mach-omap2/cpuidle34xx.c
  - arch/drivers/acpi/processor_idle.c (for all cases, not only mwait)
  - arch/x86/kernel/process.c (did throw events before, but was a mess)
  - drivers/idle/intel_idle.c (did throw events before)

Convention should be:
Fire cpu_idle events inside the current pm_idle function (not somewhere
down the the callee tree) to keep things easy.

Current possible pm_idle functions in X86:
c1e_idle, poll_idle, cpuidle_idle_call, mwait_idle, default_idle
-> this is really easy is now.

This affects userspace:
The type field of the cpu_idle power event can now direclty get
mapped to:
/sys/devices/system/cpu/cpuX/cpuidle/stateX/{name,desc,usage,time,...}
instead of throwing very CPU/mwait specific values.
This change is not visible for the intel_idle driver.
For the acpi_idle driver it should only be visible if the vendor
misses out C-states in his BIOS.
Another (perf timechart) patch reads out cpuidle info of cpu_idle
events from:
/sys/.../cpuidle/stateX/*, then the cpuidle events are mapped
to the correct C-/cpuidle state again, even if e.g. vendors miss
out C-states in their BIOS and for example only export C1 and C3.
-> everything is fine.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: Robert Schoene <robert.schoene@tu-dresden.de>
CC: Jean Pihet <j-pihet@ti.com>
CC: Arjan van de Ven <arjan@linux.intel.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Len Brown <lenb@kernel.org>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: linux-pm@lists.linux-foundation.org
CC: linux-acpi@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-perf-users@vger.kernel.org
CC: linux-omap@vger.kernel.org
---
 arch/x86/kernel/process.c    |    6 ++++--
 arch/x86/kernel/process_32.c |    4 ----
 arch/x86/kernel/process_64.c |    6 ------
 drivers/cpuidle/cpuidle.c    |   10 ++++++++--
 drivers/idle/intel_idle.c    |    2 --
 5 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 8b0ad65..b4f9ee7 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -385,6 +385,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());
 	} else {
 		local_irq_enable();
 		/* loop is done by the caller */
@@ -442,8 +444,6 @@ EXPORT_SYMBOL_GPL(cpu_idle_wait);
  */
 void mwait_idle_with_hints(unsigned long ax, unsigned long cx)
 {
-	trace_power_start(POWER_CSTATE, (ax>>4)+1, smp_processor_id());
-	trace_cpu_idle((ax>>4)+1, smp_processor_id());
 	if (!need_resched()) {
 		if (cpu_has(&current_cpu_data, X86_FEATURE_CLFLUSH_MONITOR))
 			clflush((void *)&current_thread_info()->flags);
@@ -470,6 +470,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());
 	} else
 		local_irq_enable();
 }
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 4b9befa..8d12878 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -57,8 +57,6 @@
 #include <asm/syscalls.h>
 #include <asm/debugreg.h>
 
-#include <trace/events/power.h>
-
 asmlinkage void ret_from_fork(void) __asm__("ret_from_fork");
 
 /*
@@ -113,8 +111,6 @@ void cpu_idle(void)
 			stop_critical_timings();
 			pm_idle();
 			start_critical_timings();
-			trace_power_end(smp_processor_id());
-			trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
 		}
 		tick_nohz_restart_sched_tick();
 		preempt_enable_no_resched();
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 4c818a7..bd387e8 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -51,8 +51,6 @@
 #include <asm/syscalls.h>
 #include <asm/debugreg.h>
 
-#include <trace/events/power.h>
-
 asmlinkage extern void ret_from_fork(void);
 
 DEFINE_PER_CPU(unsigned long, old_rsp);
@@ -141,10 +139,6 @@ void cpu_idle(void)
 			pm_idle();
 			start_critical_timings();
 
-			trace_power_end(smp_processor_id());
-			trace_cpu_idle(PWR_EVENT_EXIT,
-				       smp_processor_id());
-
 			/* In many cases the interrupt that ended idle
 			   has already called exit_idle. But some idle
 			   loops can be woken up without interrupt. */
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 99cc8fc..4649495 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -96,7 +96,15 @@ static void cpuidle_idle_call(void)
 
 	/* enter the state and update stats */
 	dev->last_state = target_state;
+
+	trace_power_start(POWER_CSTATE, next_state, dev->cpu);
+	trace_cpu_idle(next_state, dev->cpu);
+
 	dev->last_residency = target_state->enter(dev, target_state);
+
+	trace_power_end(dev->cpu);
+	trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
+
 	if (dev->last_state)
 		target_state = dev->last_state;
 
@@ -106,8 +114,6 @@ static void cpuidle_idle_call(void)
 	/* give the governor an opportunity to reflect on the outcome */
 	if (cpuidle_curr_governor->reflect)
 		cpuidle_curr_governor->reflect(dev);
-	trace_power_end(smp_processor_id());
-	trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
 }
 
 /**
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 56ac09d..60fa6ec 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -220,8 +220,6 @@ static int intel_idle(struct cpuidle_device *dev, struct cpuidle_state *state)
 	kt_before = ktime_get_real();
 
 	stop_critical_timings();
-	trace_power_start(POWER_CSTATE, (eax >> 4) + 1, cpu);
-	trace_cpu_idle((eax >> 4) + 1, cpu);
 	if (!need_resched()) {
 
 		__monitor((void *)&current_thread_info()->flags, 0, 0);
-- 
1.7.3.1

Re: [PATCH 3/9] X86/perf: fix power:cpu_idle double end events and throw cpu_idle events from the cpuidle layer

[Len Brown]

I'm happy to see the trace point move up into cpuidle from intel_idle.

If somebody is picking this up in a perf tree,
Acked-by: Len Brown <len.brown@intel.com>

else I can put it in the idle tree, let me know.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 3/9] X86/perf: fix power:cpu_idle double end events and throw cpu_idle events from the cpuidle layer

[Thomas Renninger]

On Wednesday 12 January 2011 07:42:59 Len Brown wrote:
> I'm happy to see the trace point move up into cpuidle from intel_idle.
> 
> If somebody is picking this up in a perf tree,
> Acked-by: Len Brown <len.brown@intel.com>
> 
> else I can put it in the idle tree, let me know.
I didn't know you have an idle tree.

If you can push this one through the idle tree that would be great.

I'll go through the rest of my patches and resubmit
in some days as soon as some things are clarified.

But I need this one to still be included in .38.
Please tell me if this could be a problem.

Thanks,

  Thomas

Re: [PATCH 3/9] X86/perf: fix power:cpu_idle double end events and throw cpu_idle events from the cpuidle layer

[Len Brown]

Applied to idle-test

git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux-idle-2.6.git idle-test

Thomas,
Please test this branch, and base your incremental patches on it.

thanks,
Len Brown, Intel Open Source Technology Center

[PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states

[Thomas Renninger]

and fill name, description and newly introduced abbrevation
consistently (always use snprintf) in all cpuidle drivers.

This is mainly for perf timechart. It draws vector graphics
pictures of sleep/idle state usage catching perf cpu_idle events.
The string used for the idle state must not exceed 3 chars or
you can't see much in these pictures.
The name could get used in the title, the introduced abbrevations
inside of the picture and the text must therefore be rather short.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: lenb@kernel.org
CC: linux-acpi@vger.kernel.org
CC: linux-pm@lists.linux-foundation.org
CC: Arjan van de Ven <arjan@linux.intel.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: linux-kernel@vger.kernel.org
CC: linux-omap@vger.kernel.org
---
 arch/arm/mach-at91/cpuidle.c          |   12 ++++++++----
 arch/arm/mach-davinci/cpuidle.c       |   13 +++++++++----
 arch/arm/mach-kirkwood/cpuidle.c      |   12 ++++++++----
 arch/arm/mach-omap2/cpuidle34xx.c     |    3 ++-
 arch/sh/kernel/cpu/shmobile/cpuidle.c |   19 +++++++++++--------
 drivers/acpi/processor_idle.c         |    3 +++
 drivers/cpuidle/cpuidle.c             |    1 +
 drivers/cpuidle/sysfs.c               |    3 +++
 drivers/idle/intel_idle.c             |   11 +++++++++++
 include/linux/cpuidle.h               |    2 ++
 10 files changed, 58 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
index 1cfeac1..6cdeb42 100644
--- a/arch/arm/mach-at91/cpuidle.c
+++ b/arch/arm/mach-at91/cpuidle.c
@@ -73,16 +73,20 @@ static int at91_init_cpuidle(void)
 	device->states[0].exit_latency = 1;
 	device->states[0].target_residency = 10000;
 	device->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
-	strcpy(device->states[0].name, "WFI");
-	strcpy(device->states[0].desc, "Wait for interrupt");
+	snprintf(device->states[0].name, CPUIDLE_NAME_LEN, "WFI");
+	snprintf(device->states[0].desc, CPUIDLE_DESC_LEN,
+		 "Wait for interrupt");
+	snprintf(device->states[0].abbr, CPUIDLE_ABBR_LEN, "W");
 
 	/* Wait for interrupt and RAM self refresh state */
 	device->states[1].enter = at91_enter_idle;
 	device->states[1].exit_latency = 10;
 	device->states[1].target_residency = 10000;
 	device->states[1].flags = CPUIDLE_FLAG_TIME_VALID;
-	strcpy(device->states[1].name, "RAM_SR");
-	strcpy(device->states[1].desc, "WFI and RAM Self Refresh");
+	snprintf(device->states[1].name, CPUIDLE_NAME_LEN, "RAM SR");
+	snprintf(device->states[1].desc, CPUIDLE_DESC_LEN,
+		 "WFI and RAM Self Refresh");
+	snprintf(device->states[1].abbr, CPUIDLE_ABBR_LEN, "WSR");
 
 	if (cpuidle_register_device(device)) {
 		printk(KERN_ERR "at91_init_cpuidle: Failed registering\n");
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
index bd59f31..42ad2d6 100644
--- a/arch/arm/mach-davinci/cpuidle.c
+++ b/arch/arm/mach-davinci/cpuidle.c
@@ -127,16 +127,21 @@ static int __init davinci_cpuidle_probe(struct platform_device *pdev)
 	device->states[0].exit_latency = 1;
 	device->states[0].target_residency = 10000;
 	device->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
-	strcpy(device->states[0].name, "WFI");
-	strcpy(device->states[0].desc, "Wait for interrupt");
+	snprintf(device->states[0].name, CPUIDLE_NAME_LEN, "WFI");
+	snprintf(device->states[0].desc, CPUIDLE_DESC_LEN,
+		 "Wait for interrupt");
+	snprintf(device->states[0].abbr, CPUIDLE_ABBR_LEN, "W");
 
 	/* Wait for interrupt and DDR self refresh state */
 	device->states[1].enter = davinci_enter_idle;
 	device->states[1].exit_latency = 10;
 	device->states[1].target_residency = 10000;
 	device->states[1].flags = CPUIDLE_FLAG_TIME_VALID;
-	strcpy(device->states[1].name, "DDR SR");
-	strcpy(device->states[1].desc, "WFI and DDR Self Refresh");
+	snprintf(device->states[1].name, CPUIDLE_NAME_LEN, "RAM SR");
+	snprintf(device->states[1].desc, CPUIDLE_DESC_LEN,
+		 "WFI and RAM Self Refresh");
+	snprintf(device->states[1].abbr, CPUIDLE_ABBR_LEN, "WSR");
+
 	if (pdata->ddr2_pdown)
 		davinci_states[1].flags |= DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN;
 	cpuidle_set_statedata(&device->states[1], &davinci_states[1]);
diff --git a/arch/arm/mach-kirkwood/cpuidle.c b/arch/arm/mach-kirkwood/cpuidle.c
index f68d33f..48eaabb 100644
--- a/arch/arm/mach-kirkwood/cpuidle.c
+++ b/arch/arm/mach-kirkwood/cpuidle.c
@@ -75,16 +75,20 @@ static int kirkwood_init_cpuidle(void)
 	device->states[0].exit_latency = 1;
 	device->states[0].target_residency = 10000;
 	device->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
-	strcpy(device->states[0].name, "WFI");
-	strcpy(device->states[0].desc, "Wait for interrupt");
+	snprintf(device->states[0].name, CPUIDLE_NAME_LEN, "WFI");
+	snprintf(device->states[0].desc, CPUIDLE_DESC_LEN,
+		 "Wait for interrupt");
+	snprintf(device->states[0].abbr, CPUIDLE_ABBR_LEN, "W");
 
 	/* Wait for interrupt and DDR self refresh state */
 	device->states[1].enter = kirkwood_enter_idle;
 	device->states[1].exit_latency = 10;
 	device->states[1].target_residency = 10000;
 	device->states[1].flags = CPUIDLE_FLAG_TIME_VALID;
-	strcpy(device->states[1].name, "DDR SR");
-	strcpy(device->states[1].desc, "WFI and DDR Self Refresh");
+	snprintf(device->states[1].name, CPUIDLE_NAME_LEN, "RAM SR");
+	snprintf(device->states[1].desc, CPUIDLE_DESC_LEN,
+		 "WFI and RAM Self Refresh");
+	snprintf(device->states[1].abbr, CPUIDLE_ABBR_LEN, "WSR");
 
 	if (cpuidle_register_device(device)) {
 		printk(KERN_ERR "kirkwood_init_cpuidle: Failed registering\n");
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index 0d50b45..a59ac39 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -496,7 +496,8 @@ int __init omap3_idle_init(void)
 			omap3_enter_idle_bm : omap3_enter_idle;
 		if (cx->type == OMAP3_STATE_C1)
 			dev->safe_state = state;
-		sprintf(state->name, "C%d", count+1);
+		snprintf(state->name, CPUIDLE_NAME_LEN, "C%d", count+1);
+		snprintf(state->abbr, CPUIDLE_ABBR_LEN, "C%d", count+1);
 		count++;
 	}
 
diff --git a/arch/sh/kernel/cpu/shmobile/cpuidle.c b/arch/sh/kernel/cpu/shmobile/cpuidle.c
index 83972aa..9ad151d 100644
--- a/arch/sh/kernel/cpu/shmobile/cpuidle.c
+++ b/arch/sh/kernel/cpu/shmobile/cpuidle.c
@@ -75,8 +75,9 @@ void sh_mobile_setup_cpuidle(void)
 	i = CPUIDLE_DRIVER_STATE_START;
 
 	state = &dev->states[i++];
-	snprintf(state->name, CPUIDLE_NAME_LEN, "C0");
-	strncpy(state->desc, "SuperH Sleep Mode", CPUIDLE_DESC_LEN);
+	snprintf(state->name, CPUIDLE_NAME_LEN, "SuperH");
+	snprintf(state->desc, CPUIDLE_DESC_LEN, "SuperH Sleep Mode");
+	snprintf(state->abbr, CPUIDLE_ABBR_LEN, "SH");
 	state->exit_latency = 1;
 	state->target_residency = 1 * 2;
 	state->power_usage = 3;
@@ -89,9 +90,10 @@ void sh_mobile_setup_cpuidle(void)
 
 	if (sh_mobile_sleep_supported & SUSP_SH_SF) {
 		state = &dev->states[i++];
-		snprintf(state->name, CPUIDLE_NAME_LEN, "C1");
-		strncpy(state->desc, "SuperH Sleep Mode [SF]",
-			CPUIDLE_DESC_LEN);
+		snprintf(state->name, CPUIDLE_NAME_LEN, "SuperH [SF]");
+		snprintf(state->desc, CPUIDLE_DESC_LEN,
+			 "SuperH Sleep Mode [SF]");
+		snprintf(state->abbr, CPUIDLE_ABBR_LEN, "SHF");
 		state->exit_latency = 100;
 		state->target_residency = 1 * 2;
 		state->power_usage = 1;
@@ -102,9 +104,10 @@ void sh_mobile_setup_cpuidle(void)
 
 	if (sh_mobile_sleep_supported & SUSP_SH_STANDBY) {
 		state = &dev->states[i++];
-		snprintf(state->name, CPUIDLE_NAME_LEN, "C2");
-		strncpy(state->desc, "SuperH Mobile Standby Mode [SF]",
-			CPUIDLE_DESC_LEN);
+		snprintf(state->name, CPUIDLE_NAME_LEN, "SuperH Standby [SF]");
+		snprintf(state->desc, CPUIDLE_DESC_LEN,
+			 "SuperH Mobile Standby Mode [SF]");
+		snprintf(state->abbr, CPUIDLE_ABBR_LEN, "SHS");
 		state->exit_latency = 2300;
 		state->target_residency = 1 * 2;
 		state->power_usage = 1;
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 104ae77..b28693e 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1016,6 +1016,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
 		switch (cx->type) {
 			case ACPI_STATE_C1:
 			snprintf(state->name, CPUIDLE_NAME_LEN, "C1");
+			snprintf(state->abbr, CPUIDLE_ABBR_LEN, "C1");
 			state->flags |= CPUIDLE_FLAG_SHALLOW;
 			if (cx->entry_method == ACPI_CSTATE_FFH)
 				state->flags |= CPUIDLE_FLAG_TIME_VALID;
@@ -1026,6 +1027,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
 
 			case ACPI_STATE_C2:
 			snprintf(state->name, CPUIDLE_NAME_LEN, "C2");
+			snprintf(state->abbr, CPUIDLE_ABBR_LEN, "C2");
 			state->flags |= CPUIDLE_FLAG_BALANCED;
 			state->flags |= CPUIDLE_FLAG_TIME_VALID;
 			state->enter = acpi_idle_enter_simple;
@@ -1034,6 +1036,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
 
 			case ACPI_STATE_C3:
 			snprintf(state->name, CPUIDLE_NAME_LEN, "C3");
+			snprintf(state->abbr, CPUIDLE_ABBR_LEN, "C3");
 			state->flags |= CPUIDLE_FLAG_DEEP;
 			state->flags |= CPUIDLE_FLAG_TIME_VALID;
 			state->flags |= CPUIDLE_FLAG_CHECK_BM;
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 4649495..b2d2b69 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -268,6 +268,7 @@ static void poll_idle_init(struct cpuidle_device *dev)
 
 	snprintf(state->name, CPUIDLE_NAME_LEN, "POLL");
 	snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE");
+	snprintf(state->abbr, CPUIDLE_ABBR_LEN, "P");
 	state->exit_latency = 0;
 	state->target_residency = 0;
 	state->power_usage = -1;
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index 0310ffa..ca7a62c 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -249,9 +249,11 @@ define_show_state_ull_function(usage)
 define_show_state_ull_function(time)
 define_show_state_str_function(name)
 define_show_state_str_function(desc)
+define_show_state_str_function(abbr)
 
 define_one_state_ro(name, show_state_name);
 define_one_state_ro(desc, show_state_desc);
+define_one_state_ro(abbr, show_state_abbr);
 define_one_state_ro(latency, show_state_exit_latency);
 define_one_state_ro(power, show_state_power_usage);
 define_one_state_ro(usage, show_state_usage);
@@ -260,6 +262,7 @@ define_one_state_ro(time, show_state_time);
 static struct attribute *cpuidle_state_default_attrs[] = {
 	&attr_name.attr,
 	&attr_desc.attr,
+	&attr_abbr.attr,
 	&attr_latency.attr,
 	&attr_power.attr,
 	&attr_usage.attr,
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 60fa6ec..3bb1f2b 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -91,6 +91,7 @@ static struct cpuidle_state nehalem_cstates[MWAIT_MAX_NUM_CSTATES] = {
 	{ /* MWAIT C1 */
 		.name = "NHM-C1",
 		.desc = "MWAIT 0x00",
+		.abbr = "C1",
 		.driver_data = (void *) 0x00,
 		.flags = CPUIDLE_FLAG_TIME_VALID,
 		.exit_latency = 3,
@@ -99,6 +100,7 @@ static struct cpuidle_state nehalem_cstates[MWAIT_MAX_NUM_CSTATES] = {
 	{ /* MWAIT C2 */
 		.name = "NHM-C3",
 		.desc = "MWAIT 0x10",
+		.abbr = "C3",
 		.driver_data = (void *) 0x10,
 		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 20,
@@ -107,6 +109,7 @@ static struct cpuidle_state nehalem_cstates[MWAIT_MAX_NUM_CSTATES] = {
 	{ /* MWAIT C3 */
 		.name = "NHM-C6",
 		.desc = "MWAIT 0x20",
+		.abbr = "C6",
 		.driver_data = (void *) 0x20,
 		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 200,
@@ -119,6 +122,7 @@ static struct cpuidle_state snb_cstates[MWAIT_MAX_NUM_CSTATES] = {
 	{ /* MWAIT C1 */
 		.name = "SNB-C1",
 		.desc = "MWAIT 0x00",
+		.abbr = "C1",
 		.driver_data = (void *) 0x00,
 		.flags = CPUIDLE_FLAG_TIME_VALID,
 		.exit_latency = 1,
@@ -127,6 +131,7 @@ static struct cpuidle_state snb_cstates[MWAIT_MAX_NUM_CSTATES] = {
 	{ /* MWAIT C2 */
 		.name = "SNB-C3",
 		.desc = "MWAIT 0x10",
+		.abbr = "C3",
 		.driver_data = (void *) 0x10,
 		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 80,
@@ -135,6 +140,7 @@ static struct cpuidle_state snb_cstates[MWAIT_MAX_NUM_CSTATES] = {
 	{ /* MWAIT C3 */
 		.name = "SNB-C6",
 		.desc = "MWAIT 0x20",
+		.abbr = "C6",
 		.driver_data = (void *) 0x20,
 		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 104,
@@ -143,6 +149,7 @@ static struct cpuidle_state snb_cstates[MWAIT_MAX_NUM_CSTATES] = {
 	{ /* MWAIT C4 */
 		.name = "SNB-C7",
 		.desc = "MWAIT 0x30",
+		.abbr = "C7",
 		.driver_data = (void *) 0x30,
 		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 109,
@@ -155,6 +162,7 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
 	{ /* MWAIT C1 */
 		.name = "ATM-C1",
 		.desc = "MWAIT 0x00",
+		.abbr = "C1",
 		.driver_data = (void *) 0x00,
 		.flags = CPUIDLE_FLAG_TIME_VALID,
 		.exit_latency = 1,
@@ -163,6 +171,7 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
 	{ /* MWAIT C2 */
 		.name = "ATM-C2",
 		.desc = "MWAIT 0x10",
+		.abbr = "C2",
 		.driver_data = (void *) 0x10,
 		.flags = CPUIDLE_FLAG_TIME_VALID,
 		.exit_latency = 20,
@@ -172,6 +181,7 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
 	{ /* MWAIT C4 */
 		.name = "ATM-C4",
 		.desc = "MWAIT 0x30",
+		.abbr = "C4",
 		.driver_data = (void *) 0x30,
 		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 100,
@@ -181,6 +191,7 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
 	{ /* MWAIT C6 */
 		.name = "ATM-C6",
 		.desc = "MWAIT 0x52",
+		.abbr = "C6",
 		.driver_data = (void *) 0x52,
 		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 140,
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 1be416b..4763ef3 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -20,6 +20,7 @@
 #define CPUIDLE_STATE_MAX	8
 #define CPUIDLE_NAME_LEN	16
 #define CPUIDLE_DESC_LEN	32
+#define CPUIDLE_ABBR_LEN	3
 
 struct cpuidle_device;
 
@@ -31,6 +32,7 @@ struct cpuidle_device;
 struct cpuidle_state {
 	char		name[CPUIDLE_NAME_LEN];
 	char		desc[CPUIDLE_DESC_LEN];
+	char		abbr[CPUIDLE_ABBR_LEN];
 	void		*driver_data;
 
 	unsigned int	flags;
-- 
1.7.3.1

Re: [PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states

[Kevin Hilman]

Thomas Renninger <trenn@suse.de> writes:

> and fill name, description and newly introduced abbrevation
> consistently (always use snprintf) in all cpuidle drivers.
>
> This is mainly for perf timechart. It draws vector graphics
> pictures of sleep/idle state usage catching perf cpu_idle events.
> The string used for the idle state must not exceed 3 chars or
> you can't see much in these pictures.
> The name could get used in the title, the introduced abbrevations
> inside of the picture and the text must therefore be rather short.
>
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> CC: lenb@kernel.org
> CC: linux-acpi@vger.kernel.org
> CC: linux-pm@lists.linux-foundation.org
> CC: Arjan van de Ven <arjan@linux.intel.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Frederic Weisbecker <fweisbec@gmail.com>
> CC: linux-kernel@vger.kernel.org
> CC: linux-omap@vger.kernel.org
> ---
>  arch/arm/mach-at91/cpuidle.c          |   12 ++++++++----
>  arch/arm/mach-davinci/cpuidle.c       |   13 +++++++++----
>  arch/arm/mach-kirkwood/cpuidle.c      |   12 ++++++++----
>  arch/arm/mach-omap2/cpuidle34xx.c     |    3 ++-
>  arch/sh/kernel/cpu/shmobile/cpuidle.c |   19 +++++++++++--------
>  drivers/acpi/processor_idle.c         |    3 +++
>  drivers/cpuidle/cpuidle.c             |    1 +
>  drivers/cpuidle/sysfs.c               |    3 +++
>  drivers/idle/intel_idle.c             |   11 +++++++++++
>  include/linux/cpuidle.h               |    2 ++
>  10 files changed, 58 insertions(+), 21 deletions(-)

For the davinci & OMAP changes,

Acked-by: Kevin Hilman <khilman@ti.com>


> diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
> index 1cfeac1..6cdeb42 100644
> --- a/arch/arm/mach-at91/cpuidle.c
> +++ b/arch/arm/mach-at91/cpuidle.c
> @@ -73,16 +73,20 @@ static int at91_init_cpuidle(void)
>  	device->states[0].exit_latency = 1;
>  	device->states[0].target_residency = 10000;
>  	device->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
> -	strcpy(device->states[0].name, "WFI");
> -	strcpy(device->states[0].desc, "Wait for interrupt");
> +	snprintf(device->states[0].name, CPUIDLE_NAME_LEN, "WFI");
> +	snprintf(device->states[0].desc, CPUIDLE_DESC_LEN,
> +		 "Wait for interrupt");
> +	snprintf(device->states[0].abbr, CPUIDLE_ABBR_LEN, "W");
>  
>  	/* Wait for interrupt and RAM self refresh state */
>  	device->states[1].enter = at91_enter_idle;
>  	device->states[1].exit_latency = 10;
>  	device->states[1].target_residency = 10000;
>  	device->states[1].flags = CPUIDLE_FLAG_TIME_VALID;
> -	strcpy(device->states[1].name, "RAM_SR");
> -	strcpy(device->states[1].desc, "WFI and RAM Self Refresh");
> +	snprintf(device->states[1].name, CPUIDLE_NAME_LEN, "RAM SR");
> +	snprintf(device->states[1].desc, CPUIDLE_DESC_LEN,
> +		 "WFI and RAM Self Refresh");
> +	snprintf(device->states[1].abbr, CPUIDLE_ABBR_LEN, "WSR");
>  
>  	if (cpuidle_register_device(device)) {
>  		printk(KERN_ERR "at91_init_cpuidle: Failed registering\n");
> diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
> index bd59f31..42ad2d6 100644
> --- a/arch/arm/mach-davinci/cpuidle.c
> +++ b/arch/arm/mach-davinci/cpuidle.c
> @@ -127,16 +127,21 @@ static int __init davinci_cpuidle_probe(struct platform_device *pdev)
>  	device->states[0].exit_latency = 1;
>  	device->states[0].target_residency = 10000;
>  	device->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
> -	strcpy(device->states[0].name, "WFI");
> -	strcpy(device->states[0].desc, "Wait for interrupt");
> +	snprintf(device->states[0].name, CPUIDLE_NAME_LEN, "WFI");
> +	snprintf(device->states[0].desc, CPUIDLE_DESC_LEN,
> +		 "Wait for interrupt");
> +	snprintf(device->states[0].abbr, CPUIDLE_ABBR_LEN, "W");
>  
>  	/* Wait for interrupt and DDR self refresh state */
>  	device->states[1].enter = davinci_enter_idle;
>  	device->states[1].exit_latency = 10;
>  	device->states[1].target_residency = 10000;
>  	device->states[1].flags = CPUIDLE_FLAG_TIME_VALID;
> -	strcpy(device->states[1].name, "DDR SR");
> -	strcpy(device->states[1].desc, "WFI and DDR Self Refresh");
> +	snprintf(device->states[1].name, CPUIDLE_NAME_LEN, "RAM SR");
> +	snprintf(device->states[1].desc, CPUIDLE_DESC_LEN,
> +		 "WFI and RAM Self Refresh");
> +	snprintf(device->states[1].abbr, CPUIDLE_ABBR_LEN, "WSR");
> +
>  	if (pdata->ddr2_pdown)
>  		davinci_states[1].flags |= DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN;
>  	cpuidle_set_statedata(&device->states[1], &davinci_states[1]);
> diff --git a/arch/arm/mach-kirkwood/cpuidle.c b/arch/arm/mach-kirkwood/cpuidle.c
> index f68d33f..48eaabb 100644
> --- a/arch/arm/mach-kirkwood/cpuidle.c
> +++ b/arch/arm/mach-kirkwood/cpuidle.c
> @@ -75,16 +75,20 @@ static int kirkwood_init_cpuidle(void)
>  	device->states[0].exit_latency = 1;
>  	device->states[0].target_residency = 10000;
>  	device->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
> -	strcpy(device->states[0].name, "WFI");
> -	strcpy(device->states[0].desc, "Wait for interrupt");
> +	snprintf(device->states[0].name, CPUIDLE_NAME_LEN, "WFI");
> +	snprintf(device->states[0].desc, CPUIDLE_DESC_LEN,
> +		 "Wait for interrupt");
> +	snprintf(device->states[0].abbr, CPUIDLE_ABBR_LEN, "W");
>  
>  	/* Wait for interrupt and DDR self refresh state */
>  	device->states[1].enter = kirkwood_enter_idle;
>  	device->states[1].exit_latency = 10;
>  	device->states[1].target_residency = 10000;
>  	device->states[1].flags = CPUIDLE_FLAG_TIME_VALID;
> -	strcpy(device->states[1].name, "DDR SR");
> -	strcpy(device->states[1].desc, "WFI and DDR Self Refresh");
> +	snprintf(device->states[1].name, CPUIDLE_NAME_LEN, "RAM SR");
> +	snprintf(device->states[1].desc, CPUIDLE_DESC_LEN,
> +		 "WFI and RAM Self Refresh");
> +	snprintf(device->states[1].abbr, CPUIDLE_ABBR_LEN, "WSR");
>  
>  	if (cpuidle_register_device(device)) {
>  		printk(KERN_ERR "kirkwood_init_cpuidle: Failed registering\n");
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
> index 0d50b45..a59ac39 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -496,7 +496,8 @@ int __init omap3_idle_init(void)
>  			omap3_enter_idle_bm : omap3_enter_idle;
>  		if (cx->type == OMAP3_STATE_C1)
>  			dev->safe_state = state;
> -		sprintf(state->name, "C%d", count+1);
> +		snprintf(state->name, CPUIDLE_NAME_LEN, "C%d", count+1);
> +		snprintf(state->abbr, CPUIDLE_ABBR_LEN, "C%d", count+1);
>  		count++;
>  	}
>  
> diff --git a/arch/sh/kernel/cpu/shmobile/cpuidle.c b/arch/sh/kernel/cpu/shmobile/cpuidle.c
> index 83972aa..9ad151d 100644
> --- a/arch/sh/kernel/cpu/shmobile/cpuidle.c
> +++ b/arch/sh/kernel/cpu/shmobile/cpuidle.c
> @@ -75,8 +75,9 @@ void sh_mobile_setup_cpuidle(void)
>  	i = CPUIDLE_DRIVER_STATE_START;
>  
>  	state = &dev->states[i++];
> -	snprintf(state->name, CPUIDLE_NAME_LEN, "C0");
> -	strncpy(state->desc, "SuperH Sleep Mode", CPUIDLE_DESC_LEN);
> +	snprintf(state->name, CPUIDLE_NAME_LEN, "SuperH");
> +	snprintf(state->desc, CPUIDLE_DESC_LEN, "SuperH Sleep Mode");
> +	snprintf(state->abbr, CPUIDLE_ABBR_LEN, "SH");
>  	state->exit_latency = 1;
>  	state->target_residency = 1 * 2;
>  	state->power_usage = 3;
> @@ -89,9 +90,10 @@ void sh_mobile_setup_cpuidle(void)
>  
>  	if (sh_mobile_sleep_supported & SUSP_SH_SF) {
>  		state = &dev->states[i++];
> -		snprintf(state->name, CPUIDLE_NAME_LEN, "C1");
> -		strncpy(state->desc, "SuperH Sleep Mode [SF]",
> -			CPUIDLE_DESC_LEN);
> +		snprintf(state->name, CPUIDLE_NAME_LEN, "SuperH [SF]");
> +		snprintf(state->desc, CPUIDLE_DESC_LEN,
> +			 "SuperH Sleep Mode [SF]");
> +		snprintf(state->abbr, CPUIDLE_ABBR_LEN, "SHF");
>  		state->exit_latency = 100;
>  		state->target_residency = 1 * 2;
>  		state->power_usage = 1;
> @@ -102,9 +104,10 @@ void sh_mobile_setup_cpuidle(void)
>  
>  	if (sh_mobile_sleep_supported & SUSP_SH_STANDBY) {
>  		state = &dev->states[i++];
> -		snprintf(state->name, CPUIDLE_NAME_LEN, "C2");
> -		strncpy(state->desc, "SuperH Mobile Standby Mode [SF]",
> -			CPUIDLE_DESC_LEN);
> +		snprintf(state->name, CPUIDLE_NAME_LEN, "SuperH Standby [SF]");
> +		snprintf(state->desc, CPUIDLE_DESC_LEN,
> +			 "SuperH Mobile Standby Mode [SF]");
> +		snprintf(state->abbr, CPUIDLE_ABBR_LEN, "SHS");
>  		state->exit_latency = 2300;
>  		state->target_residency = 1 * 2;
>  		state->power_usage = 1;
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 104ae77..b28693e 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -1016,6 +1016,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
>  		switch (cx->type) {
>  			case ACPI_STATE_C1:
>  			snprintf(state->name, CPUIDLE_NAME_LEN, "C1");
> +			snprintf(state->abbr, CPUIDLE_ABBR_LEN, "C1");
>  			state->flags |= CPUIDLE_FLAG_SHALLOW;
>  			if (cx->entry_method == ACPI_CSTATE_FFH)
>  				state->flags |= CPUIDLE_FLAG_TIME_VALID;
> @@ -1026,6 +1027,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
>  
>  			case ACPI_STATE_C2:
>  			snprintf(state->name, CPUIDLE_NAME_LEN, "C2");
> +			snprintf(state->abbr, CPUIDLE_ABBR_LEN, "C2");
>  			state->flags |= CPUIDLE_FLAG_BALANCED;
>  			state->flags |= CPUIDLE_FLAG_TIME_VALID;
>  			state->enter = acpi_idle_enter_simple;
> @@ -1034,6 +1036,7 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
>  
>  			case ACPI_STATE_C3:
>  			snprintf(state->name, CPUIDLE_NAME_LEN, "C3");
> +			snprintf(state->abbr, CPUIDLE_ABBR_LEN, "C3");
>  			state->flags |= CPUIDLE_FLAG_DEEP;
>  			state->flags |= CPUIDLE_FLAG_TIME_VALID;
>  			state->flags |= CPUIDLE_FLAG_CHECK_BM;
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 4649495..b2d2b69 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -268,6 +268,7 @@ static void poll_idle_init(struct cpuidle_device *dev)
>  
>  	snprintf(state->name, CPUIDLE_NAME_LEN, "POLL");
>  	snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE");
> +	snprintf(state->abbr, CPUIDLE_ABBR_LEN, "P");
>  	state->exit_latency = 0;
>  	state->target_residency = 0;
>  	state->power_usage = -1;
> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> index 0310ffa..ca7a62c 100644
> --- a/drivers/cpuidle/sysfs.c
> +++ b/drivers/cpuidle/sysfs.c
> @@ -249,9 +249,11 @@ define_show_state_ull_function(usage)
>  define_show_state_ull_function(time)
>  define_show_state_str_function(name)
>  define_show_state_str_function(desc)
> +define_show_state_str_function(abbr)
>  
>  define_one_state_ro(name, show_state_name);
>  define_one_state_ro(desc, show_state_desc);
> +define_one_state_ro(abbr, show_state_abbr);
>  define_one_state_ro(latency, show_state_exit_latency);
>  define_one_state_ro(power, show_state_power_usage);
>  define_one_state_ro(usage, show_state_usage);
> @@ -260,6 +262,7 @@ define_one_state_ro(time, show_state_time);
>  static struct attribute *cpuidle_state_default_attrs[] = {
>  	&attr_name.attr,
>  	&attr_desc.attr,
> +	&attr_abbr.attr,
>  	&attr_latency.attr,
>  	&attr_power.attr,
>  	&attr_usage.attr,
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index 60fa6ec..3bb1f2b 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -91,6 +91,7 @@ static struct cpuidle_state nehalem_cstates[MWAIT_MAX_NUM_CSTATES] = {
>  	{ /* MWAIT C1 */
>  		.name = "NHM-C1",
>  		.desc = "MWAIT 0x00",
> +		.abbr = "C1",
>  		.driver_data = (void *) 0x00,
>  		.flags = CPUIDLE_FLAG_TIME_VALID,
>  		.exit_latency = 3,
> @@ -99,6 +100,7 @@ static struct cpuidle_state nehalem_cstates[MWAIT_MAX_NUM_CSTATES] = {
>  	{ /* MWAIT C2 */
>  		.name = "NHM-C3",
>  		.desc = "MWAIT 0x10",
> +		.abbr = "C3",
>  		.driver_data = (void *) 0x10,
>  		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
>  		.exit_latency = 20,
> @@ -107,6 +109,7 @@ static struct cpuidle_state nehalem_cstates[MWAIT_MAX_NUM_CSTATES] = {
>  	{ /* MWAIT C3 */
>  		.name = "NHM-C6",
>  		.desc = "MWAIT 0x20",
> +		.abbr = "C6",
>  		.driver_data = (void *) 0x20,
>  		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
>  		.exit_latency = 200,
> @@ -119,6 +122,7 @@ static struct cpuidle_state snb_cstates[MWAIT_MAX_NUM_CSTATES] = {
>  	{ /* MWAIT C1 */
>  		.name = "SNB-C1",
>  		.desc = "MWAIT 0x00",
> +		.abbr = "C1",
>  		.driver_data = (void *) 0x00,
>  		.flags = CPUIDLE_FLAG_TIME_VALID,
>  		.exit_latency = 1,
> @@ -127,6 +131,7 @@ static struct cpuidle_state snb_cstates[MWAIT_MAX_NUM_CSTATES] = {
>  	{ /* MWAIT C2 */
>  		.name = "SNB-C3",
>  		.desc = "MWAIT 0x10",
> +		.abbr = "C3",
>  		.driver_data = (void *) 0x10,
>  		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
>  		.exit_latency = 80,
> @@ -135,6 +140,7 @@ static struct cpuidle_state snb_cstates[MWAIT_MAX_NUM_CSTATES] = {
>  	{ /* MWAIT C3 */
>  		.name = "SNB-C6",
>  		.desc = "MWAIT 0x20",
> +		.abbr = "C6",
>  		.driver_data = (void *) 0x20,
>  		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
>  		.exit_latency = 104,
> @@ -143,6 +149,7 @@ static struct cpuidle_state snb_cstates[MWAIT_MAX_NUM_CSTATES] = {
>  	{ /* MWAIT C4 */
>  		.name = "SNB-C7",
>  		.desc = "MWAIT 0x30",
> +		.abbr = "C7",
>  		.driver_data = (void *) 0x30,
>  		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
>  		.exit_latency = 109,
> @@ -155,6 +162,7 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
>  	{ /* MWAIT C1 */
>  		.name = "ATM-C1",
>  		.desc = "MWAIT 0x00",
> +		.abbr = "C1",
>  		.driver_data = (void *) 0x00,
>  		.flags = CPUIDLE_FLAG_TIME_VALID,
>  		.exit_latency = 1,
> @@ -163,6 +171,7 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
>  	{ /* MWAIT C2 */
>  		.name = "ATM-C2",
>  		.desc = "MWAIT 0x10",
> +		.abbr = "C2",
>  		.driver_data = (void *) 0x10,
>  		.flags = CPUIDLE_FLAG_TIME_VALID,
>  		.exit_latency = 20,
> @@ -172,6 +181,7 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
>  	{ /* MWAIT C4 */
>  		.name = "ATM-C4",
>  		.desc = "MWAIT 0x30",
> +		.abbr = "C4",
>  		.driver_data = (void *) 0x30,
>  		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
>  		.exit_latency = 100,
> @@ -181,6 +191,7 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
>  	{ /* MWAIT C6 */
>  		.name = "ATM-C6",
>  		.desc = "MWAIT 0x52",
> +		.abbr = "C6",
>  		.driver_data = (void *) 0x52,
>  		.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
>  		.exit_latency = 140,
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 1be416b..4763ef3 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -20,6 +20,7 @@
>  #define CPUIDLE_STATE_MAX	8
>  #define CPUIDLE_NAME_LEN	16
>  #define CPUIDLE_DESC_LEN	32
> +#define CPUIDLE_ABBR_LEN	3
>  
>  struct cpuidle_device;
>  
> @@ -31,6 +32,7 @@ struct cpuidle_device;
>  struct cpuidle_state {
>  	char		name[CPUIDLE_NAME_LEN];
>  	char		desc[CPUIDLE_DESC_LEN];
> +	char		abbr[CPUIDLE_ABBR_LEN];
>  	void		*driver_data;
>  
>  	unsigned int	flags;

Re: [PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states

[Len Brown]

I'm not fond of inventing a new 3-character abbreviation field
for every state because display tools can't handle the existing
16-character name field.

If the display tools can only handle 3 characters,
then why not have them simply use the 1st 3 characters
of the existing name field?  If that is not unique,
then re-arrange the strings so that it is unique...

Of course the ACPI part of this patch will not apply,
as it depends on patch 1 in this series, which was erroneous.
For ACPI, the existing name field is already fine,
as C%d fits into 3 characters.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states

[Thomas Renninger]

On Wednesday 12 January 2011 07:56:45 Len Brown wrote:
> I'm not fond of inventing a new 3-character abbreviation field
> for every state because display tools can't handle the existing
> 16-character name field.
I am also not "fond" of it, but it's a sane and easy
solution and appropriate for this issue.
 
> If the display tools can only handle 3 characters,
> then why not have them simply use the 1st 3 characters
> of the existing name field?
You mean use:
C3 NHM
instead of
NHM-C3
for intel_idle?

Then userspace has to parse the two informations glued
together, get rid of the whitespace, etc.
But by sysfs convention a separate file must be used
if two data are passed to userspace which is the case here.

> If that is not unique,
> then re-arrange the strings so that it is unique...
See above, it would unnecessarily make life hard for
userspace.
Afaik sysfs entries do not consume resources as long as
they do not get accessed.

> Of course the ACPI part of this patch will not apply,
> as it depends on patch 1 in this series, which was erroneous.
> For ACPI, the existing name field is already fine,
> as C%d fits into 3 characters.
For acpi_idle only it would work, but this is (nearly) the only
one.
For example for sh arch:
name: SuperH
"SH SuperH" contains two data and should get split up into:
name: SuperH
abbr: SH

By convention the first characters of "description" could be
used, but you would again end up in userspace parsing and
double info in one sysfs file.

I hope that's enough for convincing, I'd really like to go
the .abbr way. Do you give me an acked-by for that one?

Thanks,

    Thomas

Re: [PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states

[Len Brown]

> > If the display tools can only handle 3 characters,
> > then why not have them simply use the 1st 3 characters
> > of the existing name field?

> You mean use:
> C3 NHM
> instead of
> NHM-C3
> for intel_idle?

Yes.
is there a reason that would be a problem?

If only 3 characters are useful,
then I'd rather see you simly do this:

#define CPUIDLE_NAME_LEN 3

> Then userspace has to parse the two informations glued
> together, get rid of the whitespace, etc.

what "two informations"?
why is kernel bloat the instrument of choice
to avoid the expense of string truncation
in user-space tools?

> But by sysfs convention a separate file must be used
> if two data are passed to userspace which is the case here.

what two data?

It is fine for a string to include space characters.

> > If that is not unique,
> > then re-arrange the strings so that it is unique...

> See above, it would unnecessarily make life hard for
> userspace.
> Afaik sysfs entries do not consume resources as long as
> they do not get accessed.

they clutter the source code,
they create  an additional name for something
that arguably already has 3 names --
the state #, the name, and the description.
I'd rather delete a few of the fields than add a 4th...

> >Of course the ACPI part of this patch will not apply,
> > as it depends on patch 1 in this series, which was erroneous.
> > For ACPI, the existing name field is already fine,
> > as C%d fits into 3 characters.

> For acpi_idle only it would work, but this is (nearly) the only
> one.
> For example for sh arch:
> name: SuperH
> "SH SuperH" contains two data and should get split up into:
> name: SuperH
> abbr: SH

acpi/arch/sh/kernel/cpu/shmobile/cpuidle.c
already uses state->name = "C0", "C1", "C2".

Just use them and be done.

> By convention the first characters of "description" could be
> used, but you would again end up in userspace parsing and
> double info in one sysfs file.
> 
> I hope that's enough for convincing, I'd really like to go
> the .abbr way. Do you give me an acked-by for that one?

No.

We should not invent an additional abbreviation field.
The tools should use the existing name field.
If the existing name field is not sufficient, then
the states should simply be enumerated and numbers
be shown in the GUI.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states

[Thomas Renninger]

On Wednesday 12 January 2011 23:25:12 Len Brown wrote:
> > > If the display tools can only handle 3 characters,
> > > then why not have them simply use the 1st 3 characters
> > > of the existing name field?
> 
> > You mean use:
> > C3 NHM
> > instead of
> > NHM-C3
> > for intel_idle?
> 
> Yes.
> is there a reason that would be a problem?
A much more elegant solution came to my mind:
I'll rewrite the perf timechart patch to do:
Title:   C-stateName1 [1] , C-stateName2 [2]
and later I will refer to the states by 1, 2, ...
Eventually a:
Description:
[1] C-stateDescription1
[2] C-stateDescription2
...
can be put somewhere as well.

Please ignore this patch.

Stupid that I didn't think of that in the first place.

Thanks,

   Thomas

Re: [PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states

[Valdis.Kletnieks]

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

On Wed, 12 Jan 2011 17:25:12 EST, Len Brown said:

> > But by sysfs convention a separate file must be used
> > if two data are passed to userspace which is the case here.
> 
> what two data?
> 
> It is fine for a string to include space characters.

I think Thomas is concerned that although when you actually read a /sys
file, you know it's one string, that fact can get easily lost and cause issues
down the road. Consider this code:

foo=`cat /sys/some/file`
bar=`cat /sys/other/file`
baz=`cat /sys/third/file`
echo $foo $bar $baz | awk '{print $2 $3}'

Suddenly your output isn't what you expected...

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

[PATCH 5/9] acpi: processor->cpuidle: Only set cpuidle check_bm flag if pr->flags.bm_check is set

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: lenb@kernel.org
CC: linux-acpi@vger.kernel.org
CC: linux-pm@lists.linux-foundation.org
CC: Arjan van de Ven <arjan@linux.intel.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: linux-kernel@vger.kernel.org
---
 drivers/acpi/processor_idle.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index b28693e..82f74c9 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1039,7 +1039,8 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
 			snprintf(state->abbr, CPUIDLE_ABBR_LEN, "C3");
 			state->flags |= CPUIDLE_FLAG_DEEP;
 			state->flags |= CPUIDLE_FLAG_TIME_VALID;
-			state->flags |= CPUIDLE_FLAG_CHECK_BM;
+			if (pr->flags.bm_check)
+				state->flags |= CPUIDLE_FLAG_CHECK_BM;
 			state->enter = pr->flags.bm_check ?
 					acpi_idle_enter_bm :
 					acpi_idle_enter_simple;
-- 
1.7.3.1

Re: [PATCH 5/9] acpi: processor->cpuidle: Only set cpuidle check_bm flag if pr->flags.bm_check is set

[Len Brown]

On Fri, 7 Jan 2011, Thomas Renninger wrote:

>  drivers/acpi/processor_idle.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index b28693e..82f74c9 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -1039,7 +1039,8 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
>  			snprintf(state->abbr, CPUIDLE_ABBR_LEN, "C3");
>  			state->flags |= CPUIDLE_FLAG_DEEP;
>  			state->flags |= CPUIDLE_FLAG_TIME_VALID;
> -			state->flags |= CPUIDLE_FLAG_CHECK_BM;
> +			if (pr->flags.bm_check)
> +				state->flags |= CPUIDLE_FLAG_CHECK_BM;
>  			state->enter = pr->flags.bm_check ?
>  					acpi_idle_enter_bm :
>  					acpi_idle_enter_simple;
> -- 

While it is logical, this patch is a NO-OP, since nobody
ever checks for state->flags |= CPUIDLE_FLAG_CHECK_BM --
not cpuidle, and not the platform driver.

ie. it would be better to delete use of this flag from this file.

This points out a problem w/ cpuidle, of course.
Who owns and defines state->flags -- cpuidle and its governors,
or the cpuidle drivers?

cpuidle and its governors look only at...
CPUIDLE_FLAG_TIME_VALID and CPUIDLE_FLAG_IGNORE

cpuidle sets, but never looks at, CPUIDLE_FLAG_POLL

only intel_idle uses CPUIDLE_FLAG_TLB_FLUSHED,
so it should probably define it.

only OMAP uses CPUIDLE_FLAG_CHECK_BM,
so it should define it (or create a flag
that actually says what they mean here)

CPUIDLE_FLAG_SHALLOW, CPUIDLE_FLAG_BALANCED, CPUIDLE_FLAG_DEEP
are set in various architectures, but never checked,
so they should be deleted.

thanks,
Len Brown, Intel Open Source Technology Center

[PATCH] ACPI: processor_idle: delete use of NOP CPUIDLE_FLAGs

[Len Brown]

From: Len Brown <len.brown@intel.com>

CPUIDLE_FLAG_SHALLOW
CPUIDLE_FLAG_BALANCED
CPUIDLE_FLAG_DEEP
CPUIDLE_FLAG_CHECK_BM

were set by acpi_processor_setup_cpuidle(),
but never used by cpuidle or by acpi_idle.
So stop setting them.

Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/acpi/processor_idle.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index eefd4aa..70599d2 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1023,7 +1023,6 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
 		state->flags = 0;
 		switch (cx->type) {
 			case ACPI_STATE_C1:
-			state->flags |= CPUIDLE_FLAG_SHALLOW;
 			if (cx->entry_method == ACPI_CSTATE_FFH)
 				state->flags |= CPUIDLE_FLAG_TIME_VALID;
 
@@ -1032,16 +1031,13 @@ static int acpi_processor_setup_cpuidle(struct acpi_processor *pr)
 			break;
 
 			case ACPI_STATE_C2:
-			state->flags |= CPUIDLE_FLAG_BALANCED;
 			state->flags |= CPUIDLE_FLAG_TIME_VALID;
 			state->enter = acpi_idle_enter_simple;
 			dev->safe_state = state;
 			break;
 
 			case ACPI_STATE_C3:
-			state->flags |= CPUIDLE_FLAG_DEEP;
 			state->flags |= CPUIDLE_FLAG_TIME_VALID;
-			state->flags |= CPUIDLE_FLAG_CHECK_BM;
 			state->enter = pr->flags.bm_check ?
 					acpi_idle_enter_bm :
 					acpi_idle_enter_simple;
-- 
1.7.4.rc1.7.g2cf08

[PATCH] cpuidle: delete NOP CPUIDLE_FLAG_POLL

[Len Brown]

From: Len Brown <len.brown@intel.com>

it serves no purpose

Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/cpuidle/cpuidle.c |    2 +-
 include/linux/cpuidle.h   |    1 -
 2 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 37e4460..dd5f1ea 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -186,7 +186,7 @@ static void poll_idle_init(struct cpuidle_device *dev)
 	state->exit_latency = 0;
 	state->target_residency = 0;
 	state->power_usage = -1;
-	state->flags = CPUIDLE_FLAG_POLL;
+	state->flags = 0;
 	state->enter = poll_idle;
 }
 #else
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 1be416b..dee3285 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -48,7 +48,6 @@ struct cpuidle_state {
 /* Idle State Flags */
 #define CPUIDLE_FLAG_TIME_VALID	(0x01) /* is residency time measurable? */
 #define CPUIDLE_FLAG_CHECK_BM	(0x02) /* BM activity will exit state */
-#define CPUIDLE_FLAG_POLL	(0x10) /* no latency, no savings */
 #define CPUIDLE_FLAG_SHALLOW	(0x20) /* low latency, minimal savings */
 #define CPUIDLE_FLAG_BALANCED	(0x40) /* medium latency, moderate savings */
 #define CPUIDLE_FLAG_DEEP	(0x80) /* high latency, large savings */
-- 
1.7.4.rc1.7.g2cf08

[PATCH] SH, cpuidle: delete use of NOP CPUIDLE_FLAGS_SHALLOW

[Len Brown]

From: Len Brown <len.brown@intel.com>

set but not checked.

Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/sh/kernel/cpu/shmobile/cpuidle.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/sh/kernel/cpu/shmobile/cpuidle.c b/arch/sh/kernel/cpu/shmobile/cpuidle.c
index 83972aa..c19e2a9 100644
--- a/arch/sh/kernel/cpu/shmobile/cpuidle.c
+++ b/arch/sh/kernel/cpu/shmobile/cpuidle.c
@@ -81,7 +81,6 @@ void sh_mobile_setup_cpuidle(void)
 	state->target_residency = 1 * 2;
 	state->power_usage = 3;
 	state->flags = 0;
-	state->flags |= CPUIDLE_FLAG_SHALLOW;
 	state->flags |= CPUIDLE_FLAG_TIME_VALID;
 	state->enter = cpuidle_sleep_enter;
 
-- 
1.7.4.rc1.7.g2cf08

[PATCH] cpuidle: delete unused CPUIDLE_FLAG_SHALLOW, BALANCED, DEEP definitions

[Len Brown]

From: Len Brown <len.brown@intel.com>

Signed-off-by: Len Brown <len.brown@intel.com>
---
 include/linux/cpuidle.h |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index dee3285..c252953 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -48,9 +48,6 @@ struct cpuidle_state {
 /* Idle State Flags */
 #define CPUIDLE_FLAG_TIME_VALID	(0x01) /* is residency time measurable? */
 #define CPUIDLE_FLAG_CHECK_BM	(0x02) /* BM activity will exit state */
-#define CPUIDLE_FLAG_SHALLOW	(0x20) /* low latency, minimal savings */
-#define CPUIDLE_FLAG_BALANCED	(0x40) /* medium latency, moderate savings */
-#define CPUIDLE_FLAG_DEEP	(0x80) /* high latency, large savings */
 #define CPUIDLE_FLAG_IGNORE	(0x100) /* ignore during this idle period */
 #define CPUIDLE_FLAG_TLB_FLUSHED (0x200) /* tlb will be flushed */
 
-- 
1.7.4.rc1.7.g2cf08

[PATCH] cpuidle: CPUIDLE_FLAG_TLB_FLUSHED is specific to intel_idle

[Len Brown]

From: Len Brown <len.brown@intel.com>

Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/idle/intel_idle.c |    8 ++++++++
 include/linux/cpuidle.h   |    1 -
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 21d3871..8256309 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -82,6 +82,14 @@ static int intel_idle(struct cpuidle_device *dev, struct cpuidle_state *state);
 static struct cpuidle_state *cpuidle_state_table;
 
 /*
+ * Set this flag for states where the HW flushes the TLB for us
+ * and so we don't need cross-calls to keep it consistent.
+ * If this flag is set, SW flushes the TLB, so even if the
+ * HW doesn't do the flushing, this flag is safe to use.
+ */
+#define CPUIDLE_FLAG_TLB_FLUSHED	0x10000
+
+/*
  * States are indexed by the cstate number,
  * which is also the index into the MWAIT hint array.
  * Thus C0 is a dummy.
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index c252953..6be722c 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -49,7 +49,6 @@ struct cpuidle_state {
 #define CPUIDLE_FLAG_TIME_VALID	(0x01) /* is residency time measurable? */
 #define CPUIDLE_FLAG_CHECK_BM	(0x02) /* BM activity will exit state */
 #define CPUIDLE_FLAG_IGNORE	(0x100) /* ignore during this idle period */
-#define CPUIDLE_FLAG_TLB_FLUSHED (0x200) /* tlb will be flushed */
 
 #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
 
-- 
1.7.4.rc1.7.g2cf08

[PATCH] cpuidle: CPUIDLE_FLAG_CHECK_BM is omap3_idle specific

[Len Brown]

From: Len Brown <len.brown@intel.com>

Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/arm/mach-omap2/cpuidle34xx.c |    2 ++
 include/linux/cpuidle.h           |    1 -
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index 0d50b45..f9be4e3 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -47,6 +47,8 @@
 
 #define OMAP3_STATE_MAX OMAP3_STATE_C7
 
+#define CPUIDLE_FLAG_CHECK_BM	0x10000	/* use omap3_enter_idle_bm() */
+
 struct omap3_processor_cx {
 	u8 valid;
 	u8 type;
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 6be722c..36719ea 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -47,7 +47,6 @@ struct cpuidle_state {
 
 /* Idle State Flags */
 #define CPUIDLE_FLAG_TIME_VALID	(0x01) /* is residency time measurable? */
-#define CPUIDLE_FLAG_CHECK_BM	(0x02) /* BM activity will exit state */
 #define CPUIDLE_FLAG_IGNORE	(0x100) /* ignore during this idle period */
 
 #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
-- 
1.7.4.rc1.7.g2cf08

[PATCH 6/9] perf (userspace): Fix variable clash with glibc time() func

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: Arjan van de Ven <arjan@linux.intel.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: linux-kernel@vger.kernel.org
CC: linux-perf-users@vger.kernel.org
---
 tools/perf/util/svghelper.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/svghelper.c b/tools/perf/util/svghelper.c
index b3637db..38b9e40 100644
--- a/tools/perf/util/svghelper.c
+++ b/tools/perf/util/svghelper.c
@@ -43,11 +43,12 @@ static double cpu2y(int cpu)
 	return cpu2slot(cpu) * SLOT_MULT;
 }
 
-static double time2pixels(u64 time)
+static double time2pixels(u64 __time)
 {
 	double X;
 
-	X = 1.0 * svg_page_width * (time - first_time) / (last_time - first_time);
+	X = 1.0 * svg_page_width *
+		(__time - first_time) / (last_time - first_time);
 	return X;
 }
 
-- 
1.7.3.1

[PATCH 7/9] perf (userspace): Introduce --verbose param for perf timechart

[Thomas Renninger]

by making use of the already available global verbose variable
from util/debug.h (similar to perf diff or perf top).

Some already introduced if (verbose) condtitions in util/svnhelper.c
will also be activated with that one.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: Arjan van de Ven <arjan@linux.intel.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: linux-kernel@vger.kernel.org
CC: linux-perf-users@vger.kernel.org
---
 tools/perf/builtin-timechart.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
index 746cf03..54e9c37 100644
--- a/tools/perf/builtin-timechart.c
+++ b/tools/perf/builtin-timechart.c
@@ -31,6 +31,7 @@
 #include "util/event.h"
 #include "util/session.h"
 #include "util/svghelper.h"
+#include "util/debug.h"
 
 #define SUPPORT_OLD_POWER_EVENTS 1
 #define PWR_EVENT_EXIT -1
@@ -382,6 +383,10 @@ static void c_state_end(int cpu, u64 timestamp)
 	pwr->next = power_events;
 
 	power_events = pwr;
+	if (verbose)
+		printf("CPU: %d - start_time: %llu - end_time: %llu\n",
+		       power_events->cpu, power_events->start_time,
+		       power_events->end_time);
 }
 
 static void p_state_change(int cpu, u64 timestamp, u64 new_freq)
@@ -1069,6 +1074,8 @@ parse_process(const struct option *opt __used, const char *arg, int __used unset
 }
 
 static const struct option options[] = {
+	OPT_INCR('v', "verbose", &verbose,
+		    "print extended debug info to stdout"),
 	OPT_STRING('i', "input", &input_name, "file",
 		    "input file name"),
 	OPT_STRING('o', "output", &output_name, "file",
-- 
1.7.3.1

[PATCH 8/9] perf timechart: Map power:cpu_idle events to the corresponding cpuidle state

[Thomas Renninger]

Before, power:cpu_idle events were very specific X86 Intel mwait events.
This got fixed with previous patches and cpu_idle events are now thrown by
all cpuidle drivers and can be mapped to the corresponding cpuidle state
in /sys.

This patch reads out the corresponding cpuidle name of a cpu_idle event
and uses it in the title line of the chart (c-states Cx in x86, omap2
- DDR self refresh states for various arm archs).

It also reads out the corresponding abbr(eviation) and uses the string
to draw the cpu idle occurences. This needs a short (3 letter) string
to keep the overview in the chart.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: lenb@kernel.org
CC: linux-acpi@vger.kernel.org
CC: linux-pm@lists.linux-foundation.org
CC: Arjan van de Ven <arjan@linux.intel.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: linux-kernel@vger.kernel.org
CC: linux-perf-users@vger.kernel.org
CC: linux-omap@vger.kernel.org
CC: Frederic Weisbecker <fweisbec@gmail.com>
---
 tools/perf/util/include/linux/cpuidle.h |   20 ++++
 tools/perf/util/svghelper.c             |  149 +++++++++++++++++++++++++++---
 2 files changed, 154 insertions(+), 15 deletions(-)
 create mode 100644 tools/perf/util/include/linux/cpuidle.h

diff --git a/tools/perf/util/include/linux/cpuidle.h b/tools/perf/util/include/linux/cpuidle.h
new file mode 100644
index 0000000..7012f33
--- /dev/null
+++ b/tools/perf/util/include/linux/cpuidle.h
@@ -0,0 +1,20 @@
+#ifndef __PERF_CPUIDLE_H_
+#define __PERF_CPUIDLE_H_
+
+/* This comes from include/linux/cpuidle.h kernel header **********/
+#define CPUIDLE_STATE_MAX	8
+#define CPUIDLE_NAME_LEN	16
+#define CPUIDLE_DESC_LEN	32
+#define CPUIDLE_ABBR_LEN	3
+/******************************************************************/
+
+struct cpuidle_state {
+	char name[CPUIDLE_NAME_LEN + 1];
+	char abbr[CPUIDLE_ABBR_LEN + 1];
+};
+
+extern struct cpuidle_state cpuidle_states[CPUIDLE_STATE_MAX];
+
+extern unsigned int cpuidle_info_max;
+
+#endif
diff --git a/tools/perf/util/svghelper.c b/tools/perf/util/svghelper.c
index 38b9e40..5cf6c9e 100644
--- a/tools/perf/util/svghelper.c
+++ b/tools/perf/util/svghelper.c
@@ -16,8 +16,13 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+#include <linux/cpuidle.h>
 
 #include "svghelper.h"
+#include "debug.h"
 
 static u64 first_time, last_time;
 static u64 turbo_frequency, max_freq;
@@ -108,12 +113,14 @@ void open_svg(const char *filename, int cpus, int rows, u64 start, u64 end)
 	fprintf(svgfile, "      rect.WAITING  { fill:rgb(255,214, 48); fill-opacity:0.6; stroke-width:0;   stroke:rgb(  0,  0,  0); } \n");
 	fprintf(svgfile, "      rect.cpu      { fill:rgb(192,192,192); fill-opacity:0.2; stroke-width:0.5; stroke:rgb(128,128,128); } \n");
 	fprintf(svgfile, "      rect.pstate   { fill:rgb(128,128,128); fill-opacity:0.8; stroke-width:0; } \n");
-	fprintf(svgfile, "      rect.c1       { fill:rgb(255,214,214); fill-opacity:0.5; stroke-width:0; } \n");
-	fprintf(svgfile, "      rect.c2       { fill:rgb(255,172,172); fill-opacity:0.5; stroke-width:0; } \n");
-	fprintf(svgfile, "      rect.c3       { fill:rgb(255,130,130); fill-opacity:0.5; stroke-width:0; } \n");
-	fprintf(svgfile, "      rect.c4       { fill:rgb(255, 88, 88); fill-opacity:0.5; stroke-width:0; } \n");
-	fprintf(svgfile, "      rect.c5       { fill:rgb(255, 44, 44); fill-opacity:0.5; stroke-width:0; } \n");
-	fprintf(svgfile, "      rect.c6       { fill:rgb(255,  0,  0); fill-opacity:0.5; stroke-width:0; } \n");
+	fprintf(svgfile, "      rect.c0       { fill:rgb(102,255,102); fill-opacity:0.5; stroke-width:0; } \n");
+	fprintf(svgfile, "      rect.c1       { fill:rgb(102,255,  0); fill-opacity:0.5; stroke-width:0; } \n");
+	fprintf(svgfile, "      rect.c2       { fill:rgb(  0,255,102); fill-opacity:0.5; stroke-width:0; } \n");
+	fprintf(svgfile, "      rect.c3       { fill:rgb( 51,255, 51); fill-opacity:0.5; stroke-width:0; } \n");
+	fprintf(svgfile, "      rect.c4       { fill:rgb( 51,255,  0); fill-opacity:0.5; stroke-width:0; } \n");
+	fprintf(svgfile, "      rect.c5       { fill:rgb(  0,255, 51); fill-opacity:0.5; stroke-width:0; } \n");
+	fprintf(svgfile, "      rect.c6       { fill:rgb(  0,204,  0); fill-opacity:0.5; stroke-width:0; } \n");
+	fprintf(svgfile, "      rect.c7       { fill:rgb(  0,153,  0); fill-opacity:0.5; stroke-width:0; } \n");
 	fprintf(svgfile, "      line.pstate   { stroke:rgb(255,255,  0); stroke-opacity:0.8; stroke-width:2; } \n");
 
 	fprintf(svgfile, "    ]]>\n   </style>\n</defs>\n");
@@ -200,6 +207,81 @@ void svg_waiting(int Yslot, u64 start, u64 end)
 	fprintf(svgfile, "</g>\n");
 }
 
+/* Cpuidle info from sysfs ***************************/
+struct cpuidle_state cpuidle_states[CPUIDLE_STATE_MAX];
+unsigned int cpuidle_info_max;
+
+static void debug_dump_cpuidle_states(void)
+{
+	unsigned int state;
+
+	if (cpuidle_info_max == 0) {
+		printf("No cpuidle info retrieved from /sys\n");
+		return;
+	}
+	printf("cpuidle_info_max: %u\n", cpuidle_info_max);
+	for (state = 0; state < cpuidle_info_max; state++) {
+		printf("CPUIDLE[%u]:\n", state);
+		printf("Name: %s\n", cpuidle_states[state].name);
+		printf("Abbr: %s\n", cpuidle_states[state].abbr);
+	}
+}
+static int get_sysfs_string(const char *path, char *string,
+			     int max_string_size)
+{
+	int fd;
+	size_t numread, i;
+
+	fd = open(path, O_RDONLY);
+	if (fd == -1)
+		return -1;
+
+	numread = read(fd, string, max_string_size-1);
+	if (numread < 1) {
+		close(fd);
+		return -1;
+	}
+	for (i = 0; i < numread; i++)
+		if (string[i] == '\n')
+			string[i] = '\0';
+	string[numread] = '\0';
+	close(fd);
+	return 0;
+}
+
+#define PERF_CPUIDLE_SYS_PATH "/sys/devices/system/cpu/cpu0/cpuidle/state%u/"
+#define PERF_SYSFS_PATH_MAX 255
+
+/*
+ * Fills up cpuidle_states[CPUIDLE_STATE_MAX] info from
+ * /sys/devices/system/cpu/cpu0/cpuidle/stateX/ and sets cpuidle_info_max
+ * to found states
+ */
+static int retrieve_cpuidle_info(void)
+{
+	char path[PERF_SYSFS_PATH_MAX];
+	int state, ret;
+
+	for (state = 0; state < CPUIDLE_STATE_MAX; state++) {
+		snprintf(path, sizeof(path), PERF_CPUIDLE_SYS_PATH "name",
+			 state);
+		ret = get_sysfs_string(path, cpuidle_states[state].name,
+				       CPUIDLE_NAME_LEN + 1);
+		if (ret)
+			break;
+
+		snprintf(path, sizeof(path), PERF_CPUIDLE_SYS_PATH "abbr",
+			 state);
+		ret = get_sysfs_string(path, cpuidle_states[state].abbr,
+				       CPUIDLE_ABBR_LEN + 1);
+		if (ret)
+			break;
+	}
+	cpuidle_info_max = state;
+	return state;
+}
+/* Cpuidle info from sysfs ***************************/
+
 static char *cpu_model(void)
 {
 	static char cpu_m[255];
@@ -279,17 +361,33 @@ void svg_process(int cpu, u64 start, u64 end, const char *type, const char *name
 	fprintf(svgfile, "</g>\n");
 }
 
+/*
+ * Svg util and kernel supported max cpuidle states may differ.
+ * Cmp. with tools/perf/utils/include/linux/cpuidle.h
+ * and include/linux/cpuidle.h
+ * Currently cpuidle kernel interface and this svg tool, both support 8 states
+ */
+#define PERF_SVG_CPUIDLE_STATE_MAX 8
+
 void svg_cstate(int cpu, u64 start, u64 end, int type)
 {
 	double width;
 	char style[128];
+	static bool max_states_exceed_msg;
 
 	if (!svgfile)
 		return;
 
+	if (type > PERF_SVG_CPUIDLE_STATE_MAX) {
+		if (verbose || max_states_exceed_msg == false) {
+			max_states_exceed_msg = true;
+			printf("cpuidle state (%d) exceeding max supported "
+			       "states (%d).. ignoring\n",
+			       type, PERF_SVG_CPUIDLE_STATE_MAX);
+			return;
+		}
+	}
 
-	if (type > 6)
-		type = 6;
 	sprintf(style, "c%i", type);
 
 	fprintf(svgfile, "<rect class=\"%s\" x=\"%4.8f\" width=\"%4.8f\" y=\"%4.1f\" height=\"%4.1f\"/>\n",
@@ -452,16 +550,37 @@ static void svg_legenda_box(int X, const char *text, const char *style)
 
 void svg_legenda(void)
 {
+	unsigned int cstate, offset = 500;
+	char class[3];
+
+	retrieve_cpuidle_info();
+	if (verbose)
+		debug_dump_cpuidle_states();
+
 	if (!svgfile)
 		return;
 
-	svg_legenda_box(0,	"Running", "sample");
-	svg_legenda_box(100,	"Idle","rect.c1");
-	svg_legenda_box(200,	"Deeper Idle", "rect.c3");
-	svg_legenda_box(350,	"Deepest Idle", "rect.c6");
-	svg_legenda_box(550,	"Sleeping", "process2");
-	svg_legenda_box(650,	"Waiting for cpu", "waiting");
-	svg_legenda_box(800,	"Blocked on IO", "blocked");
+		svg_legenda_box(0,	"Running", "sample");
+		svg_legenda_box(100,	"Sleeping", "process2");
+		svg_legenda_box(200,	"Waiting for cpu", "waiting");
+		svg_legenda_box(350,	"Blocked on IO", "blocked");
+	/* trenn: Arch specific events. Only C1 exists on x86 if
+	   no cpuidle driver registered. Deeper and Deepest can get
+	   removed. Also C1 events may only get fired through cpuidle
+	   driver at some time. */
+	if (cpuidle_info_max == 0) {
+		svg_legenda_box(500,	"Idle", "c1");
+	} else {
+		for (cstate = 0; cstate < cpuidle_info_max; cstate++) {
+			sprintf(class, "c%u", cstate);
+			svg_legenda_box(offset, cpuidle_states[cstate].name,
+					class);
+			/* The box */
+			offset += 20;
+			/* The text */
+			offset += (strlen(cpuidle_states[cstate].name) * 10);
+		}
+	}
 }
 
 void svg_time_grid(void)
-- 
1.7.3.1

Re: [PATCH 8/9] perf timechart: Map power:cpu_idle events to the corresponding cpuidle state

[Thomas Renninger]

Argh, forgot guilt refresh on this one.
The changelog could be a bit more detailed by adding:

On Friday 07 January 2011 11:29:49 Thomas Renninger wrote:
> Before, power:cpu_idle events were very specific X86 Intel mwait events.
> This got fixed with previous patches and cpu_idle events are now thrown by
> all cpuidle drivers and can be mapped to the corresponding cpuidle state
> in /sys.
> 
> This patch reads out the corresponding cpuidle name of a cpu_idle event
> and uses it in the title line of the chart (c-states Cx in x86, omap2
> - DDR self refresh states for various arm archs).
> 
> It also reads out the corresponding abbr(eviation) and uses the string
> to draw the cpu idle occurences. This needs a short (3 letter) string
> to keep the overview in the chart.

All features/fixes this patch includes:
  - Read up cpuidle events from sysfs if available
    and thus make them architecture independent
  - Use (free) green color to display idle events in the chart,
    red is also used by "Blocked IO" event(s)
  - Fix wrong class="rect.cX" to class="cX" idle box svg drawing
    definitions. This fixes up black idle drawings for eog (eye of gnome)
    and firefox (inkscape somehow could handle the broken case).

    Thomas

[PATCH 9/9] perf: timechart: Fix memleak

[Thomas Renninger]

There are others, but these are not worth it, e.g. built
up power event list which gets destroyed on program exit anyway or
some bytes when trace events get parsed.

This one showed by far the biggest memory waste, was easy to
fix and could help when parsing huge trace event records.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: Arjan van de Ven <arjan@linux.intel.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: linux-perf-users@vger.kernel.org
CC: linux-kernel@vger.kernel.org

Found with valgrind, fixes:
==43509== 1,402 bytes in 251 blocks are definitely lost in loss record 61 of 74
==43509==    at 0x4C261D7: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==43509==    by 0x61573B1: strdup (in /lib64/libc-2.11.1.so)
==43509==    by 0x41DD3D: draw_wakeups (builtin-timechart.c:706)
==43509==    by 0x41E7C9: write_svg_file (builtin-timechart.c:957)
==43509==    by 0x41E87E: __cmd_timechart (builtin-timechart.c:989)
==43509==    by 0x41EB3C: cmd_timechart (builtin-timechart.c:1097)
==43509==    by 0x40D776: run_builtin (perf.c:286)
==43509==    by 0x40D993: handle_internal_command (perf.c:357)
==43509==    by 0x40DAD2: run_argv (perf.c:401)
==43509==    by 0x40DCB3: main (perf.c:487)
==43509==
==43509== 2,826 bytes in 429 blocks are definitely lost in loss record 63 of 74
==43509==    at 0x4C261D7: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==43509==    by 0x61573B1: strdup (in /lib64/libc-2.11.1.so)
==43509==    by 0x41DD70: draw_wakeups (builtin-timechart.c:710)
==43509==    by 0x41E7C9: write_svg_file (builtin-timechart.c:957)
==43509==    by 0x41E87E: __cmd_timechart (builtin-timechart.c:989)
==43509==    by 0x41EB3C: cmd_timechart (builtin-timechart.c:1097)
==43509==    by 0x40D776: run_builtin (perf.c:286)
==43509==    by 0x40D993: handle_internal_command (perf.c:357)
==43509==    by 0x40DAD2: run_argv (perf.c:401)
==43509==    by 0x40DCB3: main (perf.c:487)
---
 tools/perf/builtin-timechart.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
index 54e9c37..63dc1fd 100644
--- a/tools/perf/builtin-timechart.c
+++ b/tools/perf/builtin-timechart.c
@@ -696,10 +696,14 @@ static void draw_wakeups(void)
 					if (c->Y && c->start_time <= we->time && c->end_time >= we->time) {
 						if (p->pid == we->waker && !from) {
 							from = c->Y;
+							if (task_from)
+								free(task_from);
 							task_from = strdup(c->comm);
 						}
 						if (p->pid == we->wakee && !to) {
 							to = c->Y;
+							if (task_to)
+								free(task_to);
 							task_to = strdup(c->comm);
 						}
 					}
@@ -709,10 +713,14 @@ static void draw_wakeups(void)
 				while (c) {
 					if (p->pid == we->waker && !from) {
 						from = c->Y;
+						if (task_from)
+							free(task_from);
 						task_from = strdup(c->comm);
 					}
 					if (p->pid == we->wakee && !to) {
 						to = c->Y;
+						if (task_to)
+							free(task_to);
 						task_to = strdup(c->comm);
 					}
 					c = c->next;
-- 
1.7.3.1