Re: [PATCH] trace power_frequency events on the correct cpu (for Intel x86 CPUs)

[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@xxxxxxxxxxxxx>
> > >
> > >
> > > 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(...)
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?

What exactly is the reason you do not add
trace_power_frequency(..);
also in the
SYSTEM_IO_CAPABLE:
branch in do_drv_write()?

    Thomas



   Thomas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/