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

[Frederic Weisbecker <fweisbec@gmail.com>]

On Tue, Feb 10, 2009 at 03:57:47PM -0500, Jason Baron wrote:
> 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


Hi Jason, yes that's a nice idea.

 
> 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


A recent patch on -tip tree moved the power tracing related headers to trace/power.h
So you will probably need to rebase your patch against latest -tip (not a lot of changes
are needed though).


> 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);


It would be probably better to put your tracepoints registering on a dedicated
function to call it on both init and start.
Also it should be better to check the return value and warn on fail case.

Thanks!


>  	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);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/