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

Robert Schöne
2010-04-12T06:53:15Z

Resend, to keep the diskussion alive
Am Mittwoch, den 31.03.2010, 08:40 +0200 schrieb Robert Schöne:
> Am Dienstag, den 30.03.2010, 09:56 +0100 schrieb Thomas Renninger:
> > On Tuesday 30 March 2010 07:46:55 Robert Schöne wrote:
> > ...
> > > I really want to keep this diskussion alive until there's a soultion we
> > > can all agree.
> > > So Arjan and Thomas, are there any comments/preferences to the proposed
> > > options?
> > I'd like to extend the powertracer and pass the cpu.
> > This is the only possibility I see to be able to support IO driven
> > frequency switching drivers where the switching code must not be executed
> > on the CPU that gets switched (without executing the tracer on each
> > CPU explicitly which does not make sense).
> As I understand you, you want to extend the event data of each power
> trace event, which would be fine for me.
> However, I think this would need some resorting of the events for the
> perf tools.
> @Arjan would this be feasible?
> >
> > The next problem where current implementation is unfixable broken with
> > the tracer just passing the frequency is the fact that several CPUs
> > could get switched with one MSR write to a depending CPU (SW_ANY).
> > The same btw applies to C-states for which the tracer is used in
> > the same way (compare with 8.4.2.2 _CSD (C-State Dependency) of a
> > current ACPI spec).
> >
> > No idea what the impact on userspace tools is, if I find some time
> > I can have a look at timechart how trace data gets read/used.
> > But I fear the Cstate tracing is used in some more tools already?
> > It would be great to get feedback/suggestions from people making use
> > of it already.
> >
> > Below is still broken, but should make things at least a bit better:
> >
> > ---
> > X86 cpufreq: Fix powertracer in acpi-cpufreq and exec it on the correct cpu(s)
> >
> > Several things are broken with the tracer currently.
> > This patch fixes:
> > - With the userspace governor the wrong cpu could get tracked if the target
> > function is executed on a CPU which does not get switched
> > - In SW_ALL CPU dependency case (CPUFREQ_SHARED_TYPE_ALL) only one CPU got
> > tracked. Now all CPUs that depend on each other are tracked.
> >
> > What this patch does not fix:
> > - In SW_ANY case (CPUFREQ_SHARED_TYPE_ANY) it's enough to write to a MSR of
> > one of the depending CPUs. The power trace macro misses the ability
> > to pass the cpu. Thus only one of the depending CPUs gets tracked correctly.
> > To be able to fix this the power trace macro must get extended.
> >
> >
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > CC: Robert Schöne <robert.schoene@tu-dresden.de>
> > CC: x86@kernel.org
> > CC: cpufreq <cpufreq@vger.kernel.org>
> > CC: Arjan van de Ven <arjan@linux.intel.com>
> > CC: stable@kernel.org
> >
> > ---
> > arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c | 5 ++++-
> > 1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> > index 1b1920f..259c49e 100644
> > --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> > +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> > @@ -144,6 +144,7 @@ struct drv_cmd {
> > struct io_addr io;
> > } addr;
> > u32 val;
> > + unsigned int frequency;
> > };
> >
> > /* Called via smp_call_function_single(), on the target CPU */
> > @@ -177,11 +178,13 @@ static void do_drv_write(void *_cmd)
> > rdmsr(cmd->addr.msr.reg, lo, hi);
> > lo = (lo & ~INTEL_MSR_RANGE) | (cmd->val & INTEL_MSR_RANGE);
> > wrmsr(cmd->addr.msr.reg, lo, hi);
> > + trace_power_frequency(POWER_PSTATE, cmd->frequency);
> > break;
> > case SYSTEM_IO_CAPABLE:
> > acpi_os_write_port((acpi_io_address)cmd->addr.io.port,
> > cmd->val,
> > (u32)cmd->addr.io.bit_width);
> > + trace_power_frequency(POWER_PSTATE, cmd->frequency);
> > break;
> > default:
> > break;
> > @@ -363,7 +366,7 @@ static int acpi_cpufreq_target(struct cpufreq_policy *policy,
> > }
> > }
> >
> > - trace_power_frequency(POWER_PSTATE, data->freq_table[next_state].frequency);
> > + cmd.frequency = data->freq_table[next_state].frequency;
> >
> > switch (data->cpu_feature) {
> > case SYSTEM_INTEL_MSR_CAPABLE:
> >
> >
>
>
> --
> Robert Schoene
> Technische Universitaet Dresden
> Zentrum fuer Informationsdienste und Hochleistungsrechnen
> 01062 Dresden
>
> Tel.: (0351) 463-42483, Fax: (0351) 463-37773
> E-Mail: Robert.Schoene@tu-dresden.de
>
> --
> 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/
>