Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759642Ab0KRQeR (ORCPT ); Thu, 18 Nov 2010 11:34:17 -0500 Received: from mail-qw0-f46.google.com ([209.85.216.46]:37261 "EHLO mail-qw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759316Ab0KRQeQ convert rfc822-to-8bit (ORCPT ); Thu, 18 Nov 2010 11:34:16 -0500 MIME-Version: 1.0 X-Originating-IP: [81.245.141.86] In-Reply-To: <20101118105256.GA16912@elte.hu> References: <1289498595-25806-1-git-send-email-trenn@suse.de> <201011140734.07770.trenn@suse.de> <20101118080132.GH32621@elte.hu> <201011181027.09363.trenn@suse.de> <20101118093656.GD28467@elte.hu> <20101118105256.GA16912@elte.hu> Date: Thu, 18 Nov 2010 17:34:15 +0100 Message-ID: Subject: Re: [PATCH 2/3] PERF(kernel): Cleanup power events From: Jean Pihet To: Ingo Molnar , Thomas Renninger Cc: rjw@sisk.pl, linux-kernel@vger.kernel.org, arjan@linux.intel.com Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15550 Lines: 435 On Thu, Nov 18, 2010 at 11:52 AM, Ingo Molnar wrote: > > I am also getting build failures: > > drivers/cpufreq/cpufreq.c:357: error: 'POWER_PSTATE' undeclared (first use in this function) > drivers/cpufreq/cpufreq.c:357: error: (Each undeclared identifier is reported only once > drivers/cpufreq/cpufreq.c:357: error: for each function it appears in.) > arch/x86/kernel/process.c:375: error: 'POWER_CSTATE' undeclared (first use in this function) > arch/x86/kernel/process.c:375: error: (Each undeclared identifier is reported only once > arch/x86/kernel/process.c:375: error: for each function it appears in.) > arch/x86/kernel/process.c:446: error: 'POWER_CSTATE' undeclared (first use in this function) > arch/x86/kernel/process.c:463: error: 'POWER_CSTATE' undeclared (first use in this function) > arch/x86/kernel/process.c:485: error: 'POWER_CSTATE' undeclared (first use in this function) > include/trace/events/power.h:142: error: redefinition of 'trace_power_start' > > Config attached. The problem is because power.h gets included mutliple times, and so the POWER_ enum and the empty deprecated functions need to be protected from that. Here is a patch below that fixes it, compile tested with and without CONFIG_EVENT_POWER_TRACING_DEPRECATED set. Ingo, Thomas, please let me know if you want me tp refresh the patches with that fix. diff --git a/include/trace/events/power.h b/include/trace/events/power.h index 00d9819..89db5a1 100644 --- a/include/trace/events/power.h +++ b/include/trace/events/power.h @@ -136,12 +136,24 @@ enum { POWER_PSTATE = 2, }; #endif /* _PWR_EVENT_AVOID_DOUBLE_DEFINING_DEPRECATED */ -#else + +#else /* CONFIG_EVENT_POWER_TRACING_DEPRECATED */ + +#ifndef _PWR_EVENT_AVOID_DOUBLE_DEFINING_DEPRECATED +#define _PWR_EVENT_AVOID_DOUBLE_DEFINING_DEPRECATED +enum { + POWER_NONE = 0, + POWER_CSTATE = 1, + POWER_PSTATE = 2, +}; + /* These dummy declaration have to be ripped out when the deprecated events get removed */ static inline void trace_power_start(u64 type, u64 state, u64 cpuid) {}; static inline void trace_power_end(u64 cpuid) {}; static inline void trace_power_frequency(u64 type, u64 state, u64 cpuid) {}; +#endif /* _PWR_EVENT_AVOID_DOUBLE_DEFINING_DEPRECATED */ + #endif /* CONFIG_EVENT_POWER_TRACING_DEPRECATED */ /* Thanks, Jean > > Note: please reuse the two commits from below for further work, i did some small > cleanups to the commit text and to the patches. > > Thanks, > > ? ? ? ?Ingo > > ----------------> > From 87a2cfbda3f53c3bf00c424ce18d97b03b0c3aa0 Mon Sep 17 00:00:00 2001 > From: Thomas Renninger > Date: Thu, 18 Nov 2010 10:25:13 +0100 > Subject: [PATCH] perf: Clean up power events > > Add these new power trace events: > > ?power:cpu_idle > ?power:cpu_frequency > ?power:machine_suspend > > The old C-state/idle accounting events: > ?power:power_start > ?power:power_end > > Have now a replacement (but we are still keeping the old > tracepoints for compatibility): > > ?power:cpu_idle > > and > ?power:power_frequency > > is replaced with: > ?power:cpu_frequency > > power:machine_suspend is newly introduced. > > Jean Pihet has a patch integrated into the generic layer > (kernel/power/suspend.c) which will make use of it. > > the type= field got removed from both, it was never > used and the type is differed by the event type itself. > > perf timechart userspace tool gets adjusted in a separate patch. > > Signed-off-by: Thomas Renninger > Acked-by: Arjan van de Ven > Acked-by: Jean Pihet > Cc: Linus Torvalds > Cc: rjw@sisk.pl > LKML-Reference: <1290072314-31155-2-git-send-email-trenn@suse.de> > Signed-off-by: Ingo Molnar > --- > ?arch/x86/kernel/process.c ? ?| ? ?7 +++- > ?arch/x86/kernel/process_32.c | ? ?2 +- > ?arch/x86/kernel/process_64.c | ? ?2 + > ?drivers/cpufreq/cpufreq.c ? ?| ? ?1 + > ?drivers/cpuidle/cpuidle.c ? ?| ? ?1 + > ?drivers/idle/intel_idle.c ? ?| ? ?1 + > ?include/trace/events/power.h | ? 86 +++++++++++++++++++++++++++++++++++++---- > ?kernel/trace/Kconfig ? ? ? ? | ? 15 +++++++ > ?kernel/trace/power-traces.c ?| ? ?3 + > ?9 files changed, 107 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c > index 57d1868..155d975 100644 > --- a/arch/x86/kernel/process.c > +++ b/arch/x86/kernel/process.c > @@ -374,6 +374,7 @@ void default_idle(void) > ?{ > ? ? ? ?if (hlt_use_halt()) { > ? ? ? ? ? ? ? ?trace_power_start(POWER_CSTATE, 1, smp_processor_id()); > + ? ? ? ? ? ? ? trace_cpu_idle(1, smp_processor_id()); > ? ? ? ? ? ? ? ?current_thread_info()->status &= ~TS_POLLING; > ? ? ? ? ? ? ? ?/* > ? ? ? ? ? ? ? ? * TS_POLLING-cleared state must be visible before we > @@ -444,6 +445,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, smp_processor_id()); > + ? ? ? trace_cpu_idle((ax>>4)+1, smp_processor_id()); > ? ? ? ?if (!need_resched()) { > ? ? ? ? ? ? ? ?if (cpu_has(¤t_cpu_data, X86_FEATURE_CLFLUSH_MONITOR)) > ? ? ? ? ? ? ? ? ? ? ? ?clflush((void *)¤t_thread_info()->flags); > @@ -460,6 +462,7 @@ static void mwait_idle(void) > ?{ > ? ? ? ?if (!need_resched()) { > ? ? ? ? ? ? ? ?trace_power_start(POWER_CSTATE, 1, smp_processor_id()); > + ? ? ? ? ? ? ? trace_cpu_idle(1, smp_processor_id()); > ? ? ? ? ? ? ? ?if (cpu_has(¤t_cpu_data, X86_FEATURE_CLFLUSH_MONITOR)) > ? ? ? ? ? ? ? ? ? ? ? ?clflush((void *)¤t_thread_info()->flags); > > @@ -481,10 +484,12 @@ static void mwait_idle(void) > ?static void poll_idle(void) > ?{ > ? ? ? ?trace_power_start(POWER_CSTATE, 0, smp_processor_id()); > + ? ? ? trace_cpu_idle(0, smp_processor_id()); > ? ? ? ?local_irq_enable(); > ? ? ? ?while (!need_resched()) > ? ? ? ? ? ? ? ?cpu_relax(); > - ? ? ? trace_power_end(0); > + ? ? ? trace_power_end(smp_processor_id()); > + ? ? ? trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id()); > ?} > > ?/* > diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c > index 96586c3..4b9befa 100644 > --- a/arch/x86/kernel/process_32.c > +++ b/arch/x86/kernel/process_32.c > @@ -113,8 +113,8 @@ 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 b3d7a3a..4c818a7 100644 > --- a/arch/x86/kernel/process_64.c > +++ b/arch/x86/kernel/process_64.c > @@ -142,6 +142,8 @@ void cpu_idle(void) > ? ? ? ? ? ? ? ? ? ? ? ?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 > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index c63a438..1109f68 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -355,6 +355,7 @@ void cpufreq_notify_transition(struct cpufreq_freqs *freqs, unsigned int state) > ? ? ? ? ? ? ? ?dprintk("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new, > ? ? ? ? ? ? ? ? ? ? ? ?(unsigned long)freqs->cpu); > ? ? ? ? ? ? ? ?trace_power_frequency(POWER_PSTATE, freqs->new, freqs->cpu); > + ? ? ? ? ? ? ? trace_cpu_frequency(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 a507108..08d5f05 100644 > --- a/drivers/cpuidle/cpuidle.c > +++ b/drivers/cpuidle/cpuidle.c > @@ -107,6 +107,7 @@ static void cpuidle_idle_call(void) > ? ? ? ?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 3c95325..ba5134f 100644 > --- a/drivers/idle/intel_idle.c > +++ b/drivers/idle/intel_idle.c > @@ -221,6 +221,7 @@ static int intel_idle(struct cpuidle_device *dev, struct cpuidle_state *state) > > ? ? ? ?stop_critical_timings(); > ? ? ? ?trace_power_start(POWER_CSTATE, (eax >> 4) + 1, cpu); > + ? ? ? trace_cpu_idle((eax >> 4) + 1, cpu); > ? ? ? ?if (!need_resched()) { > > ? ? ? ? ? ? ? ?__monitor((void *)¤t_thread_info()->flags, 0, 0); > diff --git a/include/trace/events/power.h b/include/trace/events/power.h > index 286784d..00d9819 100644 > --- a/include/trace/events/power.h > +++ b/include/trace/events/power.h > @@ -7,16 +7,67 @@ > ?#include > ?#include > > -#ifndef _TRACE_POWER_ENUM_ > -#define _TRACE_POWER_ENUM_ > -enum { > - ? ? ? POWER_NONE ? ? ?= 0, > - ? ? ? POWER_CSTATE ? ?= 1, ? ?/* C-State */ > - ? ? ? POWER_PSTATE ? ?= 2, ? ?/* Fequency change or DVFS */ > - ? ? ? POWER_SSTATE ? ?= 3, ? ?/* Suspend */ > -}; > +DECLARE_EVENT_CLASS(cpu, > + > + ? ? ? TP_PROTO(unsigned int state, unsigned int cpu_id), > + > + ? ? ? TP_ARGS(state, cpu_id), > + > + ? ? ? TP_STRUCT__entry( > + ? ? ? ? ? ? ? __field( ? ? ? ?u32, ? ? ? ? ? ?state ? ? ? ? ? ) > + ? ? ? ? ? ? ? __field( ? ? ? ?u32, ? ? ? ? ? ?cpu_id ? ? ? ? ?) > + ? ? ? ), > + > + ? ? ? TP_fast_assign( > + ? ? ? ? ? ? ? __entry->state = state; > + ? ? ? ? ? ? ? __entry->cpu_id = cpu_id; > + ? ? ? ), > + > + ? ? ? TP_printk("state=%lu cpu_id=%lu", (unsigned long)__entry->state, > + ? ? ? ? ? ? ? ? (unsigned long)__entry->cpu_id) > +); > + > +DEFINE_EVENT(cpu, cpu_idle, > + > + ? ? ? TP_PROTO(unsigned int state, unsigned int cpu_id), > + > + ? ? ? TP_ARGS(state, cpu_id) > +); > + > +/* This file can get included multiple times, TRACE_HEADER_MULTI_READ at top */ > +#ifndef _PWR_EVENT_AVOID_DOUBLE_DEFINING > +#define _PWR_EVENT_AVOID_DOUBLE_DEFINING > + > +#define PWR_EVENT_EXIT -1 > ?#endif > > +DEFINE_EVENT(cpu, cpu_frequency, > + > + ? ? ? TP_PROTO(unsigned int frequency, unsigned int cpu_id), > + > + ? ? ? TP_ARGS(frequency, cpu_id) > +); > + > +TRACE_EVENT(machine_suspend, > + > + ? ? ? TP_PROTO(unsigned int state), > + > + ? ? ? TP_ARGS(state), > + > + ? ? ? TP_STRUCT__entry( > + ? ? ? ? ? ? ? __field( ? ? ? ?u32, ? ? ? ? ? ?state ? ? ? ? ? ) > + ? ? ? ), > + > + ? ? ? TP_fast_assign( > + ? ? ? ? ? ? ? __entry->state = state; > + ? ? ? ), > + > + ? ? ? TP_printk("state=%lu", (unsigned long)__entry->state) > +); > + > +/* This code will be removed after deprecation time exceeded (2.6.41) */ > +#ifdef CONFIG_EVENT_POWER_TRACING_DEPRECATED > + > ?/* > ?* The power events are used for cpuidle & suspend (power_start, power_end) > ?* ?and for cpufreq (power_frequency) > @@ -75,6 +126,24 @@ TRACE_EVENT(power_end, > > ?); > > +/* Deprecated dummy functions must be protected against multi-declartion */ > +#ifndef _PWR_EVENT_AVOID_DOUBLE_DEFINING_DEPRECATED > +#define _PWR_EVENT_AVOID_DOUBLE_DEFINING_DEPRECATED > + > +enum { > + ? ? ? POWER_NONE = 0, > + ? ? ? POWER_CSTATE = 1, > + ? ? ? POWER_PSTATE = 2, > +}; > +#endif /* _PWR_EVENT_AVOID_DOUBLE_DEFINING_DEPRECATED */ > +#else > +/* These dummy declaration have to be ripped out when the deprecated > + ? events get removed */ > +static inline void trace_power_start(u64 type, u64 state, u64 cpuid) {}; > +static inline void trace_power_end(u64 cpuid) {}; > +static inline void trace_power_frequency(u64 type, u64 state, u64 cpuid) {}; > +#endif /* CONFIG_EVENT_POWER_TRACING_DEPRECATED */ > + > ?/* > ?* The clock events are used for clock enable/disable and for > ?* ?clock rate change > @@ -153,7 +222,6 @@ DEFINE_EVENT(power_domain, power_domain_target, > > ? ? ? ?TP_ARGS(name, state, cpu_id) > ?); > - > ?#endif /* _TRACE_POWER_H */ > > ?/* This part must be outside protection */ > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig > index e04b8bc..59b44a1 100644 > --- a/kernel/trace/Kconfig > +++ b/kernel/trace/Kconfig > @@ -69,6 +69,21 @@ config EVENT_TRACING > ? ? ? ?select CONTEXT_SWITCH_TRACER > ? ? ? ?bool > > +config EVENT_POWER_TRACING_DEPRECATED > + ? ? ? depends on EVENT_TRACING > + ? ? ? bool "Deprecated power event trace API, to be removed" > + ? ? ? default y > + ? ? ? help > + ? ? ? ? Provides old power event types: > + ? ? ? ? C-state/idle accounting events: > + ? ? ? ? power:power_start > + ? ? ? ? power:power_end > + ? ? ? ? and old cpufreq accounting event: > + ? ? ? ? power:power_frequency > + ? ? ? ? This is for userspace compatibility > + ? ? ? ? and will vanish after 5 kernel iterations, > + ? ? ? ? namely 2.6.41. > + > ?config CONTEXT_SWITCH_TRACER > ? ? ? ?bool > > diff --git a/kernel/trace/power-traces.c b/kernel/trace/power-traces.c > index 0e0497d..f55fcf6 100644 > --- a/kernel/trace/power-traces.c > +++ b/kernel/trace/power-traces.c > @@ -13,5 +13,8 @@ > ?#define CREATE_TRACE_POINTS > ?#include > > +#ifdef EVENT_POWER_TRACING_DEPRECATED > ?EXPORT_TRACEPOINT_SYMBOL_GPL(power_start); > +#endif > +EXPORT_TRACEPOINT_SYMBOL_GPL(cpu_idle); > > > From b989c51b6f1989a834eecd9a64a7bd52ed230ea0 Mon Sep 17 00:00:00 2001 > From: Thomas Renninger > Date: Thu, 18 Nov 2010 10:25:12 +0100 > Subject: [PATCH] perf: Do not export power_frequency, but power_start event > > power_frequency moved to drivers/cpufreq/cpufreq.c which has > to be compiled in, no need to export it. > > intel_idle can a be module though... > > Signed-off-by: Thomas Renninger > Acked-by: Jean Pihet > CC: Arjan van de Ven > Cc: rjw@sisk.pl > LKML-Reference: <1290072314-31155-2-git-send-email-trenn@suse.de> > Signed-off-by: Ingo Molnar > --- > ?drivers/idle/intel_idle.c ? | ? ?2 -- > ?kernel/trace/power-traces.c | ? ?2 +- > ?2 files changed, 1 insertions(+), 3 deletions(-) > > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c > index 41665d2..3c95325 100644 > --- a/drivers/idle/intel_idle.c > +++ b/drivers/idle/intel_idle.c > @@ -220,9 +220,7 @@ static int intel_idle(struct cpuidle_device *dev, struct cpuidle_state *state) > ? ? ? ?kt_before = ktime_get_real(); > > ? ? ? ?stop_critical_timings(); > -#ifndef MODULE > ? ? ? ?trace_power_start(POWER_CSTATE, (eax >> 4) + 1, cpu); > -#endif > ? ? ? ?if (!need_resched()) { > > ? ? ? ? ? ? ? ?__monitor((void *)¤t_thread_info()->flags, 0, 0); > diff --git a/kernel/trace/power-traces.c b/kernel/trace/power-traces.c > index a22582a..0e0497d 100644 > --- a/kernel/trace/power-traces.c > +++ b/kernel/trace/power-traces.c > @@ -13,5 +13,5 @@ > ?#define CREATE_TRACE_POINTS > ?#include > > -EXPORT_TRACEPOINT_SYMBOL_GPL(power_frequency); > +EXPORT_TRACEPOINT_SYMBOL_GPL(power_start); > > -- 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/