[PATCH] x86 cpufreq: Make trace_power_frequency cpufreq driver independent

[PATCH] x86 cpufreq: Make trace_power_frequency cpufreq driver independent

[Thomas Renninger]

and fix the broken case if a core's frequency depends on others.

trace_power_frequency was only implemented in a rather ungeneric way
in acpi-cpufreq driver's target() function only.
-> Move the call to trace_power_frequency to
   cpufreq.c:cpufreq_notify_transition() where CPUFREQ_POSTCHANGE
   notifier is triggered.
   This will support power frequency tracing by all cpufreq drivers

trace_power_frequency did not trace frequency changes correctly when
the userspace governor was used or when CPU cores' frequency depend
on each other.
-> Moving this into the CPUFREQ_POSTCHANGE notifier and pass the cpu
   which gets switched automatically fixes this.

Robert Schoene provided some important fixes on top of my initial
quick shot version which are integrated in this patch:
- Forgot some changes in power_end trace (TP_printk/variable names)
- Variable dummy in power_end must now be cpu_id
- Use static 64 bit variable instead of unsigned int for cpu_id

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: davej@redhat.com
CC: arjan@infradead.org
CC: linux-kernel@vger.kernel.org
CC: robert.schoene@tu-dresden.de
CC: cpufreq@vger.kernel.org
Tested-by: robert.schoene@tu-dresden.de
CC: linux-perf-users@vger.kernel.org
CC: linux-trace-users@vger.kernel.org
CC: x86@kernel.org
CC: akpm@linux-foundation.org
---
 arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c |    3 ---
 arch/x86/kernel/process.c                  |    8 ++++----
 drivers/cpufreq/cpufreq.c                  |    3 +++
 drivers/cpuidle/cpuidle.c                  |    2 +-
 include/trace/events/power.h               |   27 +++++++++++++++------------
 tools/perf/builtin-timechart.c             |   11 ++++++-----
 6 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
index 1d3cdda..cee5263 100644
--- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
+++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
@@ -34,7 +34,6 @@
 #include <linux/compiler.h>
 #include <linux/dmi.h>
 #include <linux/slab.h>
-#include <trace/events/power.h>
 
 #include <linux/acpi.h>
 #include <linux/io.h>
@@ -324,8 +323,6 @@ static int acpi_cpufreq_target(struct cpufreq_policy *policy,
 		}
 	}
 
-	trace_power_frequency(POWER_PSTATE, data->freq_table[next_state].frequency);
-
 	switch (data->cpu_feature) {
 	case SYSTEM_INTEL_MSR_CAPABLE:
 		cmd.type = SYSTEM_INTEL_MSR_CAPABLE;
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index e7e3521..787572d 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -371,7 +371,7 @@ static inline int hlt_use_halt(void)
 void default_idle(void)
 {
 	if (hlt_use_halt()) {
-		trace_power_start(POWER_CSTATE, 1);
+		trace_power_start(POWER_CSTATE, 1, smp_processor_id());
 		current_thread_info()->status &= ~TS_POLLING;
 		/*
 		 * TS_POLLING-cleared state must be visible before we
@@ -441,7 +441,7 @@ 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);
+	trace_power_start(POWER_CSTATE, (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);
@@ -457,7 +457,7 @@ void mwait_idle_with_hints(unsigned long ax, unsigned long cx)
 static void mwait_idle(void)
 {
 	if (!need_resched()) {
-		trace_power_start(POWER_CSTATE, 1);
+		trace_power_start(POWER_CSTATE, 1, smp_processor_id());
 		if (cpu_has(&current_cpu_data, X86_FEATURE_CLFLUSH_MONITOR))
 			clflush((void *)&current_thread_info()->flags);
 
@@ -478,7 +478,7 @@ static void mwait_idle(void)
  */
 static void poll_idle(void)
 {
-	trace_power_start(POWER_CSTATE, 0);
+	trace_power_start(POWER_CSTATE, 0, smp_processor_id());
 	local_irq_enable();
 	while (!need_resched())
 		cpu_relax();
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 063b218..4ed6657 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -29,6 +29,8 @@
 #include <linux/completion.h>
 #include <linux/mutex.h>
 
+#include <trace/events/power.h>
+
 #define dprintk(msg...) cpufreq_debug_printk(CPUFREQ_DEBUG_CORE, \
 						"cpufreq-core", msg)
 
@@ -354,6 +356,7 @@ void cpufreq_notify_transition(struct cpufreq_freqs *freqs, unsigned int state)
 
 	case CPUFREQ_POSTCHANGE:
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
+                trace_power_frequency(POWER_PSTATE, freqs->new, freqs->cpu);
 		srcu_notifier_call_chain(&cpufreq_transition_notifier_list,
 				CPUFREQ_POSTCHANGE, freqs);
 		if (likely(policy) && likely(policy->cpu == freqs->cpu))
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 12fdd39..c672f4a 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -95,7 +95,7 @@ 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(0);
+	trace_power_end(smp_processor_id());
 }
 
 /**
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index c4efe9b..35a2a6e 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -18,52 +18,55 @@ enum {
 
 DECLARE_EVENT_CLASS(power,
 
-	TP_PROTO(unsigned int type, unsigned int state),
+	TP_PROTO(unsigned int type, unsigned int state, unsigned int cpu_id),
 
-	TP_ARGS(type, state),
+	TP_ARGS(type, state, cpu_id),
 
 	TP_STRUCT__entry(
 		__field(	u64,		type		)
 		__field(	u64,		state		)
+		__field(	u64,		cpu_id		)
 	),
 
 	TP_fast_assign(
 		__entry->type = type;
 		__entry->state = state;
+		__entry->cpu_id = cpu_id;
 	),
 
-	TP_printk("type=%lu state=%lu", (unsigned long)__entry->type, (unsigned long)__entry->state)
+	TP_printk("type=%lu state=%lu cpu_id=%lu", (unsigned long)__entry->type,
+		(unsigned long)__entry->state, (unsigned long)__entry->cpu_id)
 );
 
 DEFINE_EVENT(power, power_start,
 
-	TP_PROTO(unsigned int type, unsigned int state),
+	TP_PROTO(unsigned int type, unsigned int state, unsigned int cpu_id),
 
-	TP_ARGS(type, state)
+	TP_ARGS(type, state, cpu_id)
 );
 
 DEFINE_EVENT(power, power_frequency,
 
-	TP_PROTO(unsigned int type, unsigned int state),
+	TP_PROTO(unsigned int type, unsigned int state, unsigned int cpu_id),
 
-	TP_ARGS(type, state)
+	TP_ARGS(type, state, cpu_id)
 );
 
 TRACE_EVENT(power_end,
 
-	TP_PROTO(int dummy),
+	TP_PROTO(unsigned int cpu_id),
 
-	TP_ARGS(dummy),
+	TP_ARGS(cpu_id),
 
 	TP_STRUCT__entry(
-		__field(	u64,		dummy		)
+		__field(	u64,		cpu_id		)
 	),
 
 	TP_fast_assign(
-		__entry->dummy = 0xffff;
+		__entry->cpu_id = cpu_id;
 	),
 
-	TP_printk("dummy=%lu", (unsigned long)__entry->dummy)
+	TP_printk("cpu_id=%lu", (unsigned long)__entry->cpu_id)
 
 );
 
diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
index 5a52ed9..5161619 100644
--- a/tools/perf/builtin-timechart.c
+++ b/tools/perf/builtin-timechart.c
@@ -300,8 +300,9 @@ struct trace_entry {
 
 struct power_entry {
 	struct trace_entry te;
-	s64	type;
-	s64	value;
+	u64	type;
+	u64	value;
+	u64	cpu_id;
 };
 
 #define TASK_COMM_LEN 16
@@ -498,13 +499,13 @@ static int process_sample_event(event_t *event, struct perf_session *session)
 			return 0;
 
 		if (strcmp(event_str, "power:power_start") == 0)
-			c_state_start(data.cpu, data.time, pe->value);
+			c_state_start(pe->cpu_id, data.time, pe->value);
 
 		if (strcmp(event_str, "power:power_end") == 0)
-			c_state_end(data.cpu, data.time);
+			c_state_end(pe->cpu_id, data.time);
 
 		if (strcmp(event_str, "power:power_frequency") == 0)
-			p_state_change(data.cpu, data.time, pe->value);
+			p_state_change(pe->cpu_id, data.time, pe->value);
 
 		if (strcmp(event_str, "sched:sched_wakeup") == 0)
 			sched_wakeup(data.cpu, data.time, data.pid, te);
-- 
1.6.3

Re: [PATCH] x86 cpufreq: Make trace_power_frequency cpufreq driver independent

[Thomas Renninger]

Hi Ingo,

this is in fact a resend. First version was sent on 2010-04-30.
Your comment was:
----
Timechart is maintained by Arjan so we need an ack from him as well.
I've seen some back and forth in the discussions -
what's the technical resolution of that?
----
There was enough time for people to argue on this.
The technical resolution is the patch below.

Can this still be seen in 2.6.34 or does this need x86-tip/linux-next
queuing already (which would be bad...).
This is in 11.3/master OpenSUSE branch kernel for some weeks already.

The checkpatch violation is intended to match the indentation of:
include/trace/events/power.h

Thanks,

   Thomas


On Tuesday 25 May 2010 14:39:59 Thomas Renninger wrote:
> and fix the broken case if a core's frequency depends on others.
> 
> trace_power_frequency was only implemented in a rather ungeneric way
> in acpi-cpufreq driver's target() function only.
> -> Move the call to trace_power_frequency to
>    cpufreq.c:cpufreq_notify_transition() where CPUFREQ_POSTCHANGE
>    notifier is triggered.
>    This will support power frequency tracing by all cpufreq drivers
> 
> trace_power_frequency did not trace frequency changes correctly when
> the userspace governor was used or when CPU cores' frequency depend
> on each other.
> -> Moving this into the CPUFREQ_POSTCHANGE notifier and pass the cpu
>    which gets switched automatically fixes this.
> 
> Robert Schoene provided some important fixes on top of my initial
> quick shot version which are integrated in this patch:
> - Forgot some changes in power_end trace (TP_printk/variable names)
> - Variable dummy in power_end must now be cpu_id
> - Use static 64 bit variable instead of unsigned int for cpu_id
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> CC: davej@redhat.com
> CC: arjan@infradead.org
> CC: linux-kernel@vger.kernel.org
> CC: robert.schoene@tu-dresden.de
> CC: cpufreq@vger.kernel.org
> Tested-by: robert.schoene@tu-dresden.de
> CC: linux-perf-users@vger.kernel.org
> CC: linux-trace-users@vger.kernel.org
> CC: x86@kernel.org
> CC: akpm@linux-foundation.org
> ---
>  arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c |    3 ---
>  arch/x86/kernel/process.c                  |    8 ++++----
>  drivers/cpufreq/cpufreq.c                  |    3 +++
>  drivers/cpuidle/cpuidle.c                  |    2 +-
>  include/trace/events/power.h               |   27 
+++++++++++++++------------
>  tools/perf/builtin-timechart.c             |   11 ++++++-----
>  6 files changed, 29 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c 
b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> index 1d3cdda..cee5263 100644
> --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> @@ -34,7 +34,6 @@
>  #include <linux/compiler.h>
>  #include <linux/dmi.h>
>  #include <linux/slab.h>
> -#include <trace/events/power.h>
>  
>  #include <linux/acpi.h>
>  #include <linux/io.h>
> @@ -324,8 +323,6 @@ static int acpi_cpufreq_target(struct 
cpufreq_policy *policy,
>  		}
>  	}
>  
> -	trace_power_frequency(POWER_PSTATE, data-
>freq_table[next_state].frequency);
> -
>  	switch (data->cpu_feature) {
>  	case SYSTEM_INTEL_MSR_CAPABLE:
>  		cmd.type = SYSTEM_INTEL_MSR_CAPABLE;
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index e7e3521..787572d 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -371,7 +371,7 @@ static inline int hlt_use_halt(void)
>  void default_idle(void)
>  {
>  	if (hlt_use_halt()) {
> -		trace_power_start(POWER_CSTATE, 1);
> +		trace_power_start(POWER_CSTATE, 1, smp_processor_id());
>  		current_thread_info()->status &= ~TS_POLLING;
>  		/*
>  		 * TS_POLLING-cleared state must be visible before we
> @@ -441,7 +441,7 @@ 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);
> +	trace_power_start(POWER_CSTATE, (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);
> @@ -457,7 +457,7 @@ void mwait_idle_with_hints(unsigned long ax, 
unsigned long cx)
>  static void mwait_idle(void)
>  {
>  	if (!need_resched()) {
> -		trace_power_start(POWER_CSTATE, 1);
> +		trace_power_start(POWER_CSTATE, 1, smp_processor_id());
>  		if (cpu_has(&current_cpu_data, X86_FEATURE_CLFLUSH_MONITOR))
>  			clflush((void *)&current_thread_info()->flags);
>  
> @@ -478,7 +478,7 @@ static void mwait_idle(void)
>   */
>  static void poll_idle(void)
>  {
> -	trace_power_start(POWER_CSTATE, 0);
> +	trace_power_start(POWER_CSTATE, 0, smp_processor_id());
>  	local_irq_enable();
>  	while (!need_resched())
>  		cpu_relax();
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 063b218..4ed6657 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -29,6 +29,8 @@
>  #include <linux/completion.h>
>  #include <linux/mutex.h>
>  
> +#include <trace/events/power.h>
> +
>  #define dprintk(msg...) cpufreq_debug_printk(CPUFREQ_DEBUG_CORE, \
>  						"cpufreq-core", msg)
>  
> @@ -354,6 +356,7 @@ void cpufreq_notify_transition(struct 
cpufreq_freqs *freqs, unsigned int state)
>  
>  	case CPUFREQ_POSTCHANGE:
>  		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
> +                trace_power_frequency(POWER_PSTATE, freqs->new, 
freqs->cpu);
>  		srcu_notifier_call_chain(&cpufreq_transition_notifier_list,
>  				CPUFREQ_POSTCHANGE, freqs);
>  		if (likely(policy) && likely(policy->cpu == freqs->cpu))
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 12fdd39..c672f4a 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -95,7 +95,7 @@ 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(0);
> +	trace_power_end(smp_processor_id());
>  }
>  
>  /**
> diff --git a/include/trace/events/power.h 
b/include/trace/events/power.h
> index c4efe9b..35a2a6e 100644
> --- a/include/trace/events/power.h
> +++ b/include/trace/events/power.h
> @@ -18,52 +18,55 @@ enum {
>  
>  DECLARE_EVENT_CLASS(power,
>  
> -	TP_PROTO(unsigned int type, unsigned int state),
> +	TP_PROTO(unsigned int type, unsigned int state, unsigned int 
cpu_id),
>  
> -	TP_ARGS(type, state),
> +	TP_ARGS(type, state, cpu_id),
>  
>  	TP_STRUCT__entry(
>  		__field(	u64,		type		)
>  		__field(	u64,		state		)
> +		__field(	u64,		cpu_id		)
>  	),
>  
>  	TP_fast_assign(
>  		__entry->type = type;
>  		__entry->state = state;
> +		__entry->cpu_id = cpu_id;
>  	),
>  
> -	TP_printk("type=%lu state=%lu", (unsigned long)__entry->type, 
(unsigned long)__entry->state)
> +	TP_printk("type=%lu state=%lu cpu_id=%lu", (unsigned long)__entry-
>type,
> +		(unsigned long)__entry->state, (unsigned long)__entry-
>cpu_id)
>  );
>  
>  DEFINE_EVENT(power, power_start,
>  
> -	TP_PROTO(unsigned int type, unsigned int state),
> +	TP_PROTO(unsigned int type, unsigned int state, unsigned int 
cpu_id),
>  
> -	TP_ARGS(type, state)
> +	TP_ARGS(type, state, cpu_id)
>  );
>  
>  DEFINE_EVENT(power, power_frequency,
>  
> -	TP_PROTO(unsigned int type, unsigned int state),
> +	TP_PROTO(unsigned int type, unsigned int state, unsigned int 
cpu_id),
>  
> -	TP_ARGS(type, state)
> +	TP_ARGS(type, state, cpu_id)
>  );
>  
>  TRACE_EVENT(power_end,
>  
> -	TP_PROTO(int dummy),
> +	TP_PROTO(unsigned int cpu_id),
>  
> -	TP_ARGS(dummy),
> +	TP_ARGS(cpu_id),
>  
>  	TP_STRUCT__entry(
> -		__field(	u64,		dummy		)
> +		__field(	u64,		cpu_id		)
>  	),
>  
>  	TP_fast_assign(
> -		__entry->dummy = 0xffff;
> +		__entry->cpu_id = cpu_id;
>  	),
>  
> -	TP_printk("dummy=%lu", (unsigned long)__entry->dummy)
> +	TP_printk("cpu_id=%lu", (unsigned long)__entry->cpu_id)
>  
>  );
>  
> diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-
timechart.c
> index 5a52ed9..5161619 100644
> --- a/tools/perf/builtin-timechart.c
> +++ b/tools/perf/builtin-timechart.c
> @@ -300,8 +300,9 @@ struct trace_entry {
>  
>  struct power_entry {
>  	struct trace_entry te;
> -	s64	type;
> -	s64	value;
> +	u64	type;
> +	u64	value;
> +	u64	cpu_id;
>  };
>  
>  #define TASK_COMM_LEN 16
> @@ -498,13 +499,13 @@ static int process_sample_event(event_t *event, 
struct perf_session *session)
>  			return 0;
>  
>  		if (strcmp(event_str, "power:power_start") == 0)
> -			c_state_start(data.cpu, data.time, pe->value);
> +			c_state_start(pe->cpu_id, data.time, pe->value);
>  
>  		if (strcmp(event_str, "power:power_end") == 0)
> -			c_state_end(data.cpu, data.time);
> +			c_state_end(pe->cpu_id, data.time);
>  
>  		if (strcmp(event_str, "power:power_frequency") == 0)
> -			p_state_change(data.cpu, data.time, pe->value);
> +			p_state_change(pe->cpu_id, data.time, pe->value);
>  
>  		if (strcmp(event_str, "sched:sched_wakeup") == 0)
>  			sched_wakeup(data.cpu, data.time, data.pid, te);
> -- 
> 1.6.3
> 
> 

Re: [PATCH] x86 cpufreq: Make trace_power_frequency cpufreq driver independent

[Thomas Renninger]

Hi,

On Tuesday 25 May 2010 14:53:05 Thomas Renninger wrote:
> Hi Ingo,
> 
> this is in fact a resend. First version was sent on 2010-04-30.

Andrew: could you eventually queue this one until it gets
picked up.
Robert and myself invested quite some time to fix
trace_power_frequency for most Intel server CPUs and now it
should also work for any arbitrary CPU freq driver.

Thanks,

       Thomas

PS: I won't be able to answer for one week.

Re: [PATCH] x86 cpufreq: Make trace_power_frequency cpufreq driver independent

[Arjan van de Ven]

On Tue, 25 May 2010 14:39:59 +0200
Thomas Renninger <trenn@suse.de> wrote:

> and fix the broken case if a core's frequency depends on others.
> 
> trace_power_frequency was only implemented in a rather ungeneric way
> in acpi-cpufreq driver's target() function only.
> -> Move the call to trace_power_frequency to
>    cpufreq.c:cpufreq_notify_transition() where CPUFREQ_POSTCHANGE
>    notifier is triggered.
>    This will support power frequency tracing by all cpufreq drivers
> 
> trace_power_frequency did not trace frequency changes correctly when
> the userspace governor was used or when CPU cores' frequency depend
> on each other.
> -> Moving this into the CPUFREQ_POSTCHANGE notifier and pass the cpu
>    which gets switched automatically fixes this.


while I'm no great fan of having a second field in the perf structure
for the cpu... I can see that Thomas has a use case where this solves
his issue...

Acked-by: Arjan van de Ven <arjan@linux.intel.com>

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org