Re: PATCH] ftrace: Add a C/P state tracer to help power optimization

[Jason Baron <jbaron@redhat.com>]

On Mon, Oct 27, 2008 at 02:06:30PM -0700, Arjan van de Ven wrote:
> > Arjan van de Ven <arjan@infradead.org> writes:
> > 
> > > [...]
> > > --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> > > +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> > > [...]
> > > @@ -427,6 +429,8 @@ static int acpi_cpufreq_target(struct
> > > cpufreq_policy *policy, }
> > >  	}
> > >  
> > > +	trace_power_mark(&it, POWER_PSTATE, next_perf_state);
> > > +
> > >  	switch (data->cpu_feature) {
> > >  	case SYSTEM_INTEL_MSR_CAPABLE:
> > >  		cmd.type = SYSTEM_INTEL_MSR_CAPABLE;
> > > [...]
> > 
> > Is there some reason that this doesn't use tracepoints instead
> > of such a single-backend hook?
> 
> because it's a ton simpler this way? do simple things simpe and all
> that....
> 

hi,

I wrote a patch to make c/p state tracer dependent on tracepoints and
then realized that the discussion had already been had. However, the
patch to use tracepoints is fairly simple, allows for other consumers,
and avoids a function call in the off case. please consider.

thanks,

-Jason

Signed-off-by: Jason Baron <jbaron@redhat.com>

---

 arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c |    3 +++
 arch/x86/kernel/process.c                  |    4 ++++
 include/linux/ftrace.h                     |   15 ---------------
 include/trace/power.h                      |   18 ++++++++++++++++++
 kernel/trace/trace_power.c                 |   28 ++++++++++++++++++++--------
 5 files changed, 45 insertions(+), 23 deletions(-)
 create mode 100644 include/trace/power.h


diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
index 4b1c319..4540ddc 100644
--- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
+++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
@@ -34,6 +34,7 @@
 #include <linux/compiler.h>
 #include <linux/dmi.h>
 #include <linux/ftrace.h>
+#include <trace/power.h>
 
 #include <linux/acpi.h>
 #include <acpi/processor.h>
@@ -70,6 +71,8 @@ struct acpi_cpufreq_data {
 
 static DEFINE_PER_CPU(struct acpi_cpufreq_data *, drv_data);
 
+DEFINE_TRACE(power_mark);
+
 /* acpi_perf_data is a pointer to percpu data. */
 static struct acpi_processor_performance *acpi_perf_data;
 
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index e68bb9e..09cfd5d 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -9,6 +9,7 @@
 #include <linux/pm.h>
 #include <linux/clockchips.h>
 #include <linux/ftrace.h>
+#include <trace/power.h>
 #include <asm/system.h>
 #include <asm/apic.h>
 
@@ -19,6 +20,9 @@ EXPORT_SYMBOL(idle_nomwait);
 
 struct kmem_cache *task_xstate_cachep;
 
+DEFINE_TRACE(power_start);
+DEFINE_TRACE(power_end);
+
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 {
 	*dst = *src;
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 677432b..044e0fd 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -342,21 +342,6 @@ struct power_trace {
 #endif
 };
 
-#ifdef CONFIG_POWER_TRACER
-extern void trace_power_start(struct power_trace *it, unsigned int type,
-					unsigned int state);
-extern void trace_power_mark(struct power_trace *it, unsigned int type,
-					unsigned int state);
-extern void trace_power_end(struct power_trace *it);
-#else
-static inline void trace_power_start(struct power_trace *it, unsigned int type,
-					unsigned int state) { }
-static inline void trace_power_mark(struct power_trace *it, unsigned int type,
-					unsigned int state) { }
-static inline void trace_power_end(struct power_trace *it) { }
-#endif
-
-
 /*
  * Structure that defines an entry function trace.
  */
diff --git a/include/trace/power.h b/include/trace/power.h
new file mode 100644
index 0000000..c3225ff
--- /dev/null
+++ b/include/trace/power.h
@@ -0,0 +1,18 @@
+#ifndef _TRACE_POWER_H
+#define _TRACE_POWER_H
+
+#include <linux/tracepoint.h>
+
+DECLARE_TRACE(power_start,
+	TPPROTO(struct power_trace *it, unsigned int type, unsigned int state),
+		TPARGS(it, type, state));
+
+DECLARE_TRACE(power_mark,
+	TPPROTO(struct power_trace *it, unsigned int type, unsigned int state),
+		TPARGS(it, type, state));
+
+DECLARE_TRACE(power_end,
+	TPPROTO(struct power_trace *it),
+		TPARGS(it));
+
+#endif
diff --git a/kernel/trace/trace_power.c b/kernel/trace/trace_power.c
index 7bda248..18e97c9 100644
--- a/kernel/trace/trace_power.c
+++ b/kernel/trace/trace_power.c
@@ -14,30 +14,45 @@
 #include <linux/ftrace.h>
 #include <linux/kallsyms.h>
 #include <linux/module.h>
+#include <linux/module.h>
+#include <trace/power.h>
 
 #include "trace.h"
 
 static struct trace_array *power_trace;
 static int __read_mostly trace_power_enabled;
 
+static void trace_power_start_callback(struct power_trace *it,
+					unsigned int type, unsigned int state);
+static void trace_power_mark_callback(struct power_trace *it, unsigned int type,
+					unsigned int state);
+static void trace_power_end_callback(struct power_trace *it);
 
 static void start_power_trace(struct trace_array *tr)
 {
 	trace_power_enabled = 1;
+	register_trace_power_start(trace_power_start_callback);
+	register_trace_power_end(trace_power_end_callback);
+	register_trace_power_mark(trace_power_mark_callback);
 }
 
 static void stop_power_trace(struct trace_array *tr)
 {
 	trace_power_enabled = 0;
+	unregister_trace_power_start(trace_power_start_callback);
+	unregister_trace_power_end(trace_power_end_callback);
+	unregister_trace_power_mark(trace_power_mark_callback);
 }
 
-
 static int power_trace_init(struct trace_array *tr)
 {
 	int cpu;
 	power_trace = tr;
 
 	trace_power_enabled = 1;
+	register_trace_power_start(trace_power_start_callback);
+	register_trace_power_end(trace_power_end_callback);
+	register_trace_power_mark(trace_power_mark_callback);
 
 	for_each_cpu(cpu, cpu_possible_mask)
 		tracing_reset(tr, cpu);
@@ -95,8 +110,8 @@ static int init_power_trace(void)
 }
 device_initcall(init_power_trace);
 
-void trace_power_start(struct power_trace *it, unsigned int type,
-			 unsigned int level)
+static void trace_power_start_callback(struct power_trace *it,
+			unsigned int type, unsigned int level)
 {
 	if (!trace_power_enabled)
 		return;
@@ -106,10 +121,9 @@ void trace_power_start(struct power_trace *it, unsigned int type,
 	it->type = type;
 	it->stamp = ktime_get();
 }
-EXPORT_SYMBOL_GPL(trace_power_start);
 
 
-void trace_power_end(struct power_trace *it)
+static void trace_power_end_callback(struct power_trace *it)
 {
 	struct ring_buffer_event *event;
 	struct trace_power *entry;
@@ -139,9 +153,8 @@ void trace_power_end(struct power_trace *it)
  out:
 	preempt_enable();
 }
-EXPORT_SYMBOL_GPL(trace_power_end);
 
-void trace_power_mark(struct power_trace *it, unsigned int type,
+static void trace_power_mark_callback(struct power_trace *it, unsigned int type,
 			 unsigned int level)
 {
 	struct ring_buffer_event *event;
@@ -176,4 +189,3 @@ void trace_power_mark(struct power_trace *it, unsigned int type,
  out:
 	preempt_enable();
 }
-EXPORT_SYMBOL_GPL(trace_power_mark);