* [PATCH 1/2] acpi-cpufreq: Fix CPU_ANY CPUFREQ_{PRE,POST}CHANGE notification
@ 2010-04-20 11:17 Thomas Renninger
2010-04-20 11:17 ` [PATCH 2/2] x86 cpufreq: Make trace_power_frequency cpufreq driver independent Thomas Renninger
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Renninger @ 2010-04-20 11:17 UTC (permalink / raw
To: davej; +Cc: Thomas Renninger, venki, arjan, linux-kernel
Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: venki@google.com
CC: davej@redhat.com
CC: arjan@infradead.org
CC: davej@redhat.com
CC: linux-kernel@vger.kernel.org
---
arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
index 4591680..c6de3a9 100644
--- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
+++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
@@ -391,7 +391,7 @@ static int acpi_cpufreq_target(struct cpufreq_policy *policy,
freqs.old = perf->states[perf->state].core_frequency * 1000;
freqs.new = data->freq_table[next_state].frequency;
- for_each_cpu(i, cmd.mask) {
+ for_each_cpu(i, policy->cpus) {
freqs.cpu = i;
cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
}
@@ -407,7 +407,7 @@ static int acpi_cpufreq_target(struct cpufreq_policy *policy,
}
}
- for_each_cpu(i, cmd.mask) {
+ for_each_cpu(i, policy->cpus) {
freqs.cpu = i;
cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
}
--
1.6.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] x86 cpufreq: Make trace_power_frequency cpufreq driver independent
2010-04-20 11:17 [PATCH 1/2] acpi-cpufreq: Fix CPU_ANY CPUFREQ_{PRE,POST}CHANGE notification Thomas Renninger
@ 2010-04-20 11:17 ` Thomas Renninger
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Renninger @ 2010-04-20 11:17 UTC (permalink / raw
To: davej; +Cc: Thomas Renninger, arjan, linux-kernel, robert.schoene
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: davej@redhat.com
CC: linux-kernel@vger.kernel.org
CC: robert.schoene@tu-dresden.de
Tested-by: robert.schoene@tu-dresden.de
---
arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c | 3 ---
arch/x86/kernel/process.c | 8 ++++----
drivers/cpufreq/cpufreq.c | 5 +++++
drivers/cpuidle/cpuidle.c | 2 +-
include/trace/events/power.h | 27 +++++++++++++++------------
tools/perf/builtin-timechart.c | 11 ++++++-----
6 files changed, 31 insertions(+), 25 deletions(-)
diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
index c6de3a9..9241402 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>
@@ -364,8 +363,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 28ad9f4..bc3bbf6 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(¤t_cpu_data, X86_FEATURE_CLFLUSH_MONITOR))
clflush((void *)¤t_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(¤t_cpu_data, X86_FEATURE_CLFLUSH_MONITOR))
clflush((void *)¤t_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 2d5d575..8ec1539 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,9 @@ void cpufreq_notify_transition(struct cpufreq_freqs *freqs, unsigned int state)
case CPUFREQ_POSTCHANGE:
adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
+ dprintk("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
+ (unsigned long)freqs->cpu);
+ 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 0d4d8ff..7809bef 100644
--- a/tools/perf/builtin-timechart.c
+++ b/tools/perf/builtin-timechart.c
@@ -306,8 +306,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
@@ -504,13 +505,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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] x86 cpufreq: Make trace_power_frequency cpufreq driver independent
2010-04-27 14:57 Implement trace_power_frequency for all cpufreq drivers Thomas Renninger
@ 2010-04-27 14:57 ` Thomas Renninger
2010-04-30 9:08 ` Ingo Molnar
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Renninger @ 2010-04-27 14:57 UTC (permalink / raw
To: x86
Cc: linux-trace-users, linux-perf-users, robert.schoene, cpufreq,
linux-kernel, davej, arjan, 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
---
arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c | 3 ---
arch/x86/kernel/process.c | 8 ++++----
drivers/cpufreq/cpufreq.c | 5 +++++
drivers/cpuidle/cpuidle.c | 2 +-
include/trace/events/power.h | 27 +++++++++++++++------------
tools/perf/builtin-timechart.c | 11 ++++++-----
6 files changed, 31 insertions(+), 25 deletions(-)
diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
index c6de3a9..9241402 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>
@@ -364,8 +363,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 28ad9f4..bc3bbf6 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(¤t_cpu_data, X86_FEATURE_CLFLUSH_MONITOR))
clflush((void *)¤t_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(¤t_cpu_data, X86_FEATURE_CLFLUSH_MONITOR))
clflush((void *)¤t_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 2d5d575..8ec1539 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,9 @@ void cpufreq_notify_transition(struct cpufreq_freqs *freqs, unsigned int state)
case CPUFREQ_POSTCHANGE:
adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
+ dprintk("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
+ (unsigned long)freqs->cpu);
+ 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 0d4d8ff..7809bef 100644
--- a/tools/perf/builtin-timechart.c
+++ b/tools/perf/builtin-timechart.c
@@ -306,8 +306,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
@@ -504,13 +505,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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] x86 cpufreq: Make trace_power_frequency cpufreq driver independent
2010-04-27 14:57 ` [PATCH 2/2] x86 cpufreq: Make trace_power_frequency cpufreq driver independent Thomas Renninger
@ 2010-04-30 9:08 ` Ingo Molnar
2010-04-30 10:00 ` Thomas Renninger
0 siblings, 1 reply; 5+ messages in thread
From: Ingo Molnar @ 2010-04-30 9:08 UTC (permalink / raw
To: Thomas Renninger
Cc: x86, linux-trace-users, linux-perf-users, robert.schoene, cpufreq,
linux-kernel, davej, arjan, Arnaldo Carvalho de Melo,
Fr??d??ric Weisbecker, Peter Zijlstra
* Thomas Renninger <trenn@suse.de> wrote:
> + trace_power_start(POWER_CSTATE, 1, smp_processor_id());
> + trace_power_start(POWER_CSTATE, (ax>>4)+1, smp_processor_id());
> + trace_power_start(POWER_CSTATE, 1, smp_processor_id());
> + trace_power_start(POWER_CSTATE, 0, smp_processor_id());
> + trace_power_frequency(POWER_PSTATE, freqs->new, freqs->cpu);
> + trace_power_end(smp_processor_id());
Extending power tracing to all cpufreq modules is obviously a good thing.
But why is trace_power_start() adding a CPU ID argument? CPU ids are already
available and can be sampled via PERF_SAMPLE_CPU if needed. AFAICS only
power_frequency needs a new 'target_cpu_id' field.
> diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
> index 0d4d8ff..7809bef 100644
> --- a/tools/perf/builtin-timechart.c
> +++ b/tools/perf/builtin-timechart.c
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?
(Also, there's some whitespace noise in the patch.)
Thanks,
Ingo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] x86 cpufreq: Make trace_power_frequency cpufreq driver independent
2010-04-30 9:08 ` Ingo Molnar
@ 2010-04-30 10:00 ` Thomas Renninger
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Renninger @ 2010-04-30 10:00 UTC (permalink / raw
To: Ingo Molnar
Cc: x86, linux-trace-users, linux-perf-users, robert.schoene, cpufreq,
linux-kernel, davej, arjan, Arnaldo Carvalho de Melo,
Fr??d??ric Weisbecker, Peter Zijlstra
On Friday 30 April 2010 11:08:14 Ingo Molnar wrote:
>
> * Thomas Renninger <trenn@suse.de> wrote:
>
> > + trace_power_start(POWER_CSTATE, 1, smp_processor_id());
> > + trace_power_start(POWER_CSTATE, (ax>>4)+1, smp_processor_id());
> > + trace_power_start(POWER_CSTATE, 1, smp_processor_id());
> > + trace_power_start(POWER_CSTATE, 0, smp_processor_id());
> > + trace_power_frequency(POWER_PSTATE, freqs->new, freqs->cpu);
> > + trace_power_end(smp_processor_id());
>
> Extending power tracing to all cpufreq modules is obviously a good thing.
>
> But why is trace_power_start() adding a CPU ID argument? CPU ids are already
> available and can be sampled via PERF_SAMPLE_CPU if needed. AFAICS only
> power_frequency needs a new 'target_cpu_id' field.
Currently the C-states get triggered on the machine you are running on.
But you can have C-state dependencies (ACPI spec at least defines this
the same way as done for P-states, not sure whether this exists in
reality already, Linux does not evaluate them yet).
The same way we run into trouble with P-states (SW_ALL, SW_ANY, HW).
Not sure how this will/could show up in reality in HW or implementation, but
as these dependencies are already defined in spec, it sounds like a good idea
to pass the CPU through the POWER_CSTATE events as well.
Compare with chapter 8.4.2.2 _CSD (C-State Dependency)
of an ACPI spec 3.0 or newer.
There are also other architectures starting to use processor sleep states.
So this is not actually used (smp_processor_id() is the same as already
tracked by trace event internally), but to make the interface more robust
for the future. Changes in the trace_power layout later may hurt.
> > diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
> > index 0d4d8ff..7809bef 100644
> > --- a/tools/perf/builtin-timechart.c
> > +++ b/tools/perf/builtin-timechart.c
>
> 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?
>
> (Also, there's some whitespace noise in the patch.)
Oops. Tell me if you like to push it and I send a checkpatch cleaned up version.
Sorry about that.
Thomas
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-04-30 18:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-04-20 11:17 [PATCH 1/2] acpi-cpufreq: Fix CPU_ANY CPUFREQ_{PRE,POST}CHANGE notification Thomas Renninger
2010-04-20 11:17 ` [PATCH 2/2] x86 cpufreq: Make trace_power_frequency cpufreq driver independent Thomas Renninger
-- strict thread matches above, loose matches on Subject: below --
2010-04-27 14:57 Implement trace_power_frequency for all cpufreq drivers Thomas Renninger
2010-04-27 14:57 ` [PATCH 2/2] x86 cpufreq: Make trace_power_frequency cpufreq driver independent Thomas Renninger
2010-04-30 9:08 ` Ingo Molnar
2010-04-30 10:00 ` Thomas Renninger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).