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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 15 Mar 2010 11:51:05 +0100
From:	Thomas Renninger <trenn@...e.de>
To:	Robert Schöne <robert.schoene@...dresden.de>
Cc:	Arjan van de Ven <arjan@...ux.intel.com>,
	Dave Jones <davej@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"linux-kernel" <linux-kernel@...r.kernel.org>,
	cpufreq <cpufreq@...r.kernel.org>, x86@...nel.org
Subject: Re: [PATCH] trace power_frequency events on the correct cpu  (for Intel x86 CPUs)

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@...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(...)
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/