linux-kernel - Re: [PATCH 3/5] intel_pstate: remove intel

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]

Message-ID: <1963166.eOk3pBXQhq@aspire.rjw.lan>
Date:   Sat, 17 Jun 2017 02:37:35 +0200
From:   "Rafael J. Wysocki" <rjw@...ysocki.net>
To:     Len Brown <lenb@...nel.org>
Cc:     X86 ML <x86@...nel.org>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Linux PM list <linux-pm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Len Brown <len.brown@...el.com>
Subject: Re: [PATCH 3/5] intel_pstate: remove intel_pstate.get()

On Friday, June 16, 2017 08:35:19 PM Len Brown wrote:
> On Fri, Jun 16, 2017 at 7:53 PM, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
> > On Wednesday, June 07, 2017 07:39:14 PM Len Brown wrote:
> >> From: Len Brown <len.brown@...el.com>
> >>
> >> The x86 cpufreq core now uses aperfmperf_khz_on_cpu()
> >> to supply /sys/.../cpufreq/scaling_cur_freq
> >> on all x86 systems supporting APERF/MPERF.
> >>
> >> That includes 100% of systems supported by intel_pstate,
> >> and so intel_pstate.get() is now a NOP -- remove it.
> >>
> >> Invoke aperfmperf_khz_on_cpu() directly,
> >> if legacy-mode p-state tracing is enabled.
> >>
> >> Signed-off-by: Len Brown <len.brown@...el.com>
> >> ---
> >>  drivers/cpufreq/intel_pstate.c | 16 +---------------
> >>  1 file changed, 1 insertion(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> >> index b7de5bd..5d67780 100644
> >> --- a/drivers/cpufreq/intel_pstate.c
> >> +++ b/drivers/cpufreq/intel_pstate.c
> >> @@ -1597,12 +1597,6 @@ static inline bool intel_pstate_sample(struct cpudata *cpu, u64 time)
> >>       return false;
> >>  }
> >>
> >> -static inline int32_t get_avg_frequency(struct cpudata *cpu)
> >> -{
> >> -     return mul_ext_fp(cpu->sample.core_avg_perf,
> >> -                       cpu->pstate.max_pstate_physical * cpu->pstate.scaling);
> >> -}
> >> -
> >>  static inline int32_t get_avg_pstate(struct cpudata *cpu)
> >>  {
> >>       return mul_ext_fp(cpu->pstate.max_pstate_physical,
> >> @@ -1728,7 +1722,7 @@ static void intel_pstate_adjust_pstate(struct cpudata *cpu, int target_pstate)
> >>               sample->mperf,
> >>               sample->aperf,
> >>               sample->tsc,
> >> -             get_avg_frequency(cpu),
> >> +             aperfmperf_khz_on_cpu(cpu->cpu),
> 
> Note that I deleted the line above in an updated version of this patch
> that I'm ready to send out.
> 
> There were a couple of problems with it.
> The first is that it was  ugly that tracing (which occurs only in the
> SW governor case)
> could shorten the measurement interval as seen by the sysfs interface.
> 
> The second is that this trace point can be called with irqs off,
> and smp_call_function_single() will WARN when called with irqs off.
> 
> Srinivas Acked that I simply remove this field from the tracepoint --
> as it is redundant to calculate it in the kernel when we are already
> exporting the raw values of aperf and mperf.

This changes the tracepoint format and I know about a couple of user space
scripts that consume these tracepoints though.

What would be wrong with leaving it as is?

> >>               fp_toint(cpu->iowait_boost * 100));
> >>  }
> >>
> >> @@ -1922,13 +1916,6 @@ static int intel_pstate_init_cpu(unsigned int cpunum)
> >>       return 0;
> >>  }
> >>
> >> -static unsigned int intel_pstate_get(unsigned int cpu_num)
> >> -{
> >> -     struct cpudata *cpu = all_cpu_data[cpu_num];
> >> -
> >> -     return cpu ? get_avg_frequency(cpu) : 0;
> >> -}
> >> -
> >>  static void intel_pstate_set_update_util_hook(unsigned int cpu_num)
> >>  {
> >>       struct cpudata *cpu = all_cpu_data[cpu_num];
> >> @@ -2157,7 +2144,6 @@ static struct cpufreq_driver intel_pstate = {
> >>       .setpolicy      = intel_pstate_set_policy,
> >>       .suspend        = intel_pstate_hwp_save_state,
> >>       .resume         = intel_pstate_resume,
> >> -     .get            = intel_pstate_get,
> >>       .init           = intel_pstate_cpu_init,
> >>       .exit           = intel_pstate_cpu_exit,
> >>       .stop_cpu       = intel_pstate_stop_cpu,
> >>
> >
> > This change will cause cpufreq_quick_get() to work differently and it is
> > called by KVM among other things.  Will that still work?
> 
> Neither invocation of cpufreq_quick_get() (ARM cpu-cooling and X86 variable TSC)
> are aligned with the hardware supported by the intel_pstate driver.

OK

Thanks,
Rafael