trace power_frequency events on the correct cpu (for Intel x86 CPUs)
in [Kernel]
|
Prev: [PATCH -v2] x86, k8 nb: Enable k8_northbridges unconditionally on AMD
Next: [RFC] resource, PCI: work around pci=use_crs conflicts
From: Arjan van de Ven on 12 Mar 2010 10:00 On 3/12/2010 5:17, Robert Sch�ne wrote: > This patch fixes the following behaviour: > Currently, the power_frequency event is reported for the cpu (core) which initiated the frequency change. > It should be reported for the cpu that actually changes its frequency. > > Example: when using > taskset -c 0 echo<new_frequency> > /sys/devices/system/cpu/cpu1/cpufreq/scaling_setspeed > cpu 0 is traced, instead of cpu 1 > > Signed of by Robert Schoene<robert.schoene(a)tu-dresden.de> > > > diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c > index 1b1920f..0a47f10 100644 > --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c > +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c > @@ -174,6 +174,7 @@ static void do_drv_write(void *_cmd) > > switch (cmd->type) { > case SYSTEM_INTEL_MSR_CAPABLE: > + trace_power_frequency(POWER_PSTATE, cmd->val); > rdmsr(cmd->addr.msr.reg, lo, hi); > lo = (lo& ~INTEL_MSR_RANGE) | (cmd->val& INTEL_MSR_RANGE); > wrmsr(cmd->addr.msr.reg, lo, hi); > @@ -363,7 +364,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: > > are you sure this is right? it's moving something from outside a switch statement to inside only one prong of a switch statement... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Robert Schöne on 12 Mar 2010 10:50 Am Freitag, den 12.03.2010, 06:52 -0800 schrieb Arjan van de Ven: > On 3/12/2010 5:17, Robert Schöne wrote: > > This patch fixes the following behaviour: > > Currently, the power_frequency event is reported for the cpu (core) which initiated the frequency change. > > It should be reported for the cpu that actually changes its frequency. > > > > Example: when using > > taskset -c 0 echo<new_frequency> > /sys/devices/system/cpu/cpu1/cpufreq/scaling_setspeed > > cpu 0 is traced, instead of cpu 1 > > > > Signed of by Robert Schoene<robert.schoene(a)tu-dresden.de> > > > > > > diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c > > index 1b1920f..0a47f10 100644 > > --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c > > +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c > > @@ -174,6 +174,7 @@ static void do_drv_write(void *_cmd) > > > > switch (cmd->type) { > > case SYSTEM_INTEL_MSR_CAPABLE: > > + trace_power_frequency(POWER_PSTATE, cmd->val); > > rdmsr(cmd->addr.msr.reg, lo, hi); > > lo = (lo& ~INTEL_MSR_RANGE) | (cmd->val& INTEL_MSR_RANGE); > > wrmsr(cmd->addr.msr.reg, lo, hi); > > @@ -363,7 +364,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: > > > > > > are you sure this is right? > it's moving something from outside a switch statement to inside only one prong of a switch statement... I'm pretty sure, since I'm moving it from function acpi_cpufreq_target(...) to do_drv_write(...) > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- Robert Schoene Technische Universitaet Dresden Zentrum fuer Informationsdienste und Hochleistungsrechnen 01062 Dresden Tel.: (0351) 463-42483, Fax: (0351) 463-37773 E-Mail: Robert.Schoene(a)tu-dresden.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Robert Schöne on 16 Mar 2010 03:20 Am Montag, den 15.03.2010, 11:51 +0100 schrieb Thomas Renninger: > On Friday 12 March 2010 16:41:46 Robert Schöne wrote: > > Am Freitag, den 12.03.2010, 06:52 -0800 schrieb Arjan van de Ven: > > > On 3/12/2010 5:17, Robert Schöne wrote: > > > > This patch fixes the following behaviour: > > > > Currently, the power_frequency event is reported for the cpu (core) which initiated the frequency change. > > > > It should be reported for the cpu that actually changes its frequency. > > > > > > > > Example: when using > > > > taskset -c 0 echo<new_frequency> > /sys/devices/system/cpu/cpu1/cpufreq/scaling_setspeed > > > > cpu 0 is traced, instead of cpu 1 > > > > > > > > Signed of by Robert Schoene<robert.schoene(a)tu-dresden.de> > > > > > > > > > > > > diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c > > > > index 1b1920f..0a47f10 100644 > > > > --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c > > > > +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c > > > > @@ -174,6 +174,7 @@ static void do_drv_write(void *_cmd) > > > > > > > > switch (cmd->type) { > > > > case SYSTEM_INTEL_MSR_CAPABLE: > > > > + trace_power_frequency(POWER_PSTATE, cmd->val); > > > > rdmsr(cmd->addr.msr.reg, lo, hi); > > > > lo = (lo& ~INTEL_MSR_RANGE) | (cmd->val& INTEL_MSR_RANGE); > > > > wrmsr(cmd->addr.msr.reg, lo, hi); > > > > @@ -363,7 +364,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: > > > > > > > > > > > > > > are you sure this is right? > > > it's moving something from outside a switch statement to inside only one prong of a switch statement... You are right, it should be in all cases, which execute a frequency change. > > > > I'm pretty sure, since I'm moving it from function acpi_cpufreq_target(...) to do_drv_write(...) > What exactly is the argument you are pretty sure this is correct? > > I expect Arjan is right. > You now only trace MSR based and not IO based frequency switching. > > I don't know the tracing stuff, but it seems the cpu that executes > trace_power_frequency shows up in the statistics as the one on which the > frequency change happened which currently is wrong and you try to fix this? Yes > > What exactly is the reason you do not add > trace_power_frequency(..); > also in the > SYSTEM_IO_CAPABLE: > branch in do_drv_write()? I don't know system io capable systems and what they are doing, so I ignored it to prevent reporting wrong "frequencies". > > Thomas > > > > Thomas I stand corrected and appended the new patch (with an additional trace command for io capable systems) Robert diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c index 1b1920f..4803883 100644 --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c @@ -174,11 +174,13 @@ static void do_drv_write(void *_cmd) switch (cmd->type) { case SYSTEM_INTEL_MSR_CAPABLE: + trace_power_frequency(POWER_PSTATE, cmd->val); rdmsr(cmd->addr.msr.reg, lo, hi); lo = (lo & ~INTEL_MSR_RANGE) | (cmd->val & INTEL_MSR_RANGE); wrmsr(cmd->addr.msr.reg, lo, hi); break; case SYSTEM_IO_CAPABLE: + trace_power_frequency(POWER_PSTATE, cmd->val); acpi_os_write_port((acpi_io_address)cmd->addr.io.port, cmd->val, (u32)cmd->addr.io.bit_width); @@ -363,7 +365,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: -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ingo Molnar on 16 Mar 2010 06:00 * Robert Sch?ne <robert.schoene(a)tu-dresden.de> wrote: > Am Montag, den 15.03.2010, 11:51 +0100 schrieb Thomas Renninger: > > On Friday 12 March 2010 16:41:46 Robert Sch??ne wrote: > > > Am Freitag, den 12.03.2010, 06:52 -0800 schrieb Arjan van de Ven: > > > > On 3/12/2010 5:17, Robert Sch??ne wrote: > > > > > This patch fixes the following behaviour: > > > > > Currently, the power_frequency event is reported for the cpu (core) which initiated the frequency change. > > > > > It should be reported for the cpu that actually changes its frequency. > > > > > > > > > > Example: when using > > > > > taskset -c 0 echo<new_frequency> > /sys/devices/system/cpu/cpu1/cpufreq/scaling_setspeed > > > > > cpu 0 is traced, instead of cpu 1 > > > > > > > > > > Signed of by Robert Schoene<robert.schoene(a)tu-dresden.de> > > > > > > > > > > > > > > > diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c > > > > > index 1b1920f..0a47f10 100644 > > > > > --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c > > > > > +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c > > > > > @@ -174,6 +174,7 @@ static void do_drv_write(void *_cmd) > > > > > > > > > > switch (cmd->type) { > > > > > case SYSTEM_INTEL_MSR_CAPABLE: > > > > > + trace_power_frequency(POWER_PSTATE, cmd->val); > > > > > rdmsr(cmd->addr.msr.reg, lo, hi); > > > > > lo = (lo& ~INTEL_MSR_RANGE) | (cmd->val& INTEL_MSR_RANGE); > > > > > wrmsr(cmd->addr.msr.reg, lo, hi); > > > > > @@ -363,7 +364,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: > > > > > > > > > > > > > > > > > > are you sure this is right? > > > > it's moving something from outside a switch statement to inside only one prong of a switch statement... > You are right, it should be in all cases, which execute a frequency change. > > > > > > I'm pretty sure, since I'm moving it from function acpi_cpufreq_target(...) to do_drv_write(...) > > What exactly is the argument you are pretty sure this is correct? > > > > I expect Arjan is right. > > You now only trace MSR based and not IO based frequency switching. > > > > I don't know the tracing stuff, but it seems the cpu that executes > > trace_power_frequency shows up in the statistics as the one on which the > > frequency change happened which currently is wrong and you try to fix this? > Yes > > > > What exactly is the reason you do not add > > trace_power_frequency(..); > > also in the > > SYSTEM_IO_CAPABLE: > > branch in do_drv_write()? > I don't know system io capable systems and what they are doing, so I ignored it to prevent reporting wrong "frequencies". > > > > Thomas > > > > > > > > Thomas > > I stand corrected and appended the new patch (with an additional trace > command for io capable systems) Robert Please send a changelogged version with everyone Cc:-ed once the dust settles and the acks are in. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arjan van de Ven on 16 Mar 2010 10:20
> But something else...: > What exactly is the power tracer good for and what is it > capable of which cpufreq_stats is not capable to do? look at timechart for example..... it's extremely useful to have this for us that do power tuning... cpufreq_stats is nice but not nearly good enough since you only get averages, not time behavior. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |