linux-kernel - Re: [PATCH v3 5/6] cpufreq/cppc: set the frequency used for computing the capacity

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: <CAKfTPtCogPYyfe2aoNQE4BLRM+++3ezdx-3NsVFFMBKnZNWfiw@mail.gmail.com>
Date:   Tue, 24 Oct 2023 11:56:46 +0200
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Pierre Gondois <pierre.gondois@....com>
Cc:     linux@...linux.org.uk, catalin.marinas@....com, will@...nel.org,
        paul.walmsley@...ive.com, palmer@...belt.com,
        aou@...s.berkeley.edu, sudeep.holla@....com,
        gregkh@...uxfoundation.org, mingo@...hat.com, peterz@...radead.org,
        juri.lelli@...hat.com, dietmar.eggemann@....com,
        rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
        bristot@...hat.com, vschneid@...hat.com, viresh.kumar@...aro.org,
        lenb@...nel.org, robert.moore@...el.com, lukasz.luba@....com,
        ionela.voinescu@....com, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
        linux-pm@...r.kernel.org, linux-acpi@...r.kernel.org,
        acpica-devel@...ts.linuxfoundation.org, conor.dooley@...rochip.com,
        suagrfillet@...il.com, ajones@...tanamicro.com, lftan@...nel.org,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v3 5/6] cpufreq/cppc: set the frequency used for computing
 the capacity

On Fri, 20 Oct 2023 at 18:05, Pierre Gondois <pierre.gondois@....com> wrote:
>
> Hello Vincent,
>
> On 10/18/23 19:26, Rafael J. Wysocki wrote:
> > On Wed, Oct 18, 2023 at 6:25 PM Vincent Guittot
> > <vincent.guittot@...aro.org> wrote:
> >>
> >> Save the frequency associated to the performance that has been used when
> >> initializing the capacity of CPUs.
> >> Also, cppc cpufreq driver can register an artificial energy model. In such
> >> case, it needs the frequency for this compute capacity.
> >> We moved and renamed cppc_perf_to_khz and cppc_perf_to_khz to use them
> >> outside cppc_cpufreq in topology_init_cpu_capacity_cppc().
> >>
> >> Signed-off-by: Vincent Guittot <vincent.guittot@...aro.org>
> >
> > For the changes in drivers/acpi/cppc_acpi.c :
> >
> > Acked-by: Rafael J. Wysocki <rafael@...nel.org>
> >
> >> ---
> >>   drivers/acpi/cppc_acpi.c       |  93 ++++++++++++++++++++++
> >>   drivers/base/arch_topology.c   |  15 +++-
> >>   drivers/cpufreq/cppc_cpufreq.c | 141 ++++++---------------------------
> >>   include/acpi/cppc_acpi.h       |   2 +
> >>   4 files changed, 133 insertions(+), 118 deletions(-)
> >>
>
> [snip]
>
> >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> >> index 9a073c2d2086..2372ce791bb4 100644
> >> --- a/drivers/base/arch_topology.c
> >> +++ b/drivers/base/arch_topology.c
> >> @@ -350,6 +350,7 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
> >>   void topology_init_cpu_capacity_cppc(void)
> >>   {
> >>          struct cppc_perf_caps perf_caps;
> >> +       u64 capacity, capacity_scale;
>
> I think capacity_scale should be initialized to 0 here,
> since it is used to find the max value of raw_capacity[cpu].

yes

>
> >>          int cpu;
> >>
> >>          if (likely(!acpi_cpc_valid()))
> >> @@ -365,6 +366,10 @@ void topology_init_cpu_capacity_cppc(void)
> >>                      (perf_caps.highest_perf >= perf_caps.nominal_perf) &&
> >>                      (perf_caps.highest_perf >= perf_caps.lowest_perf)) {
> >>                          raw_capacity[cpu] = perf_caps.highest_perf;
> >> +                       capacity_scale = max_t(u64, capacity_scale, raw_capacity[cpu]);
> >> +
> >> +                       per_cpu(capacity_ref_freq, cpu) = cppc_perf_to_khz(&perf_caps, raw_capacity[cpu]);
>
> I think capacity_ref_freq in in Hz, so the freq should be multiplied by 1000 .

yes, I forgot the *1000. I'm going to add * HZ_PER_KHZ

>
> With these two modifications, the patches worked well on a cppc-based platform.
>
> Sorry I forgot to detail what it was. It's a modified Juno with CPPC enabled. AMUs are not
> enabled, so the CPPC performance counters are not handled correctly and FIE cannot be enabled,
> but it is possible to change frequencies.
>
> The _CPC objects are setup as:
> little CPUs:
> - lowest_freq = 450 (MHz)
> - nominal_freq = 800 (MHz)
> - highest_perf = 383 * 1000
> - nominal_perf = 322 * 1000
> - lowest_perf = 181 * 1000
> - lowest_nonlinear_perf = 181 * 1000
>
> big CPUs:
> - lowest_freq = 600 (MHz)
> - nominal_freq = 1200 (MHz)
> - highest_perf = 1024 * 1000
> - nominal_perf = 833 * 1000
> - lowest_perf = 512 * 1000
> - lowest_nonlinear_perf = 512 * 1000
>
> As a remainder, available frequencies are:
> - little CPUs: 450, 800, 950 MHz
> - big CPUs: 600, 1000, 1200 Mhz
> So the platform is setup to have the last frequency as a boost frequency (for testing).
>
> ----
>
> Just to make a note of 2 potential side-issues for later (independent from these patches):
>
> - When testing with boosted/non-bossted frequencies, it didn't seem that cpu_overutilized()
>    was taking the maximum frequency into consideration. This might mean that when lowering the
>    maximum frequency of a policy, the maximum capacity of the CPUs of this policy is used
>    to detect over-utilization.
>    I would have thought that the over-utilization threshold would be lowered at the same time.

No it's not, It will be part of a next step patchset. This patchset
aims to consolidate and use the same reference so we can then easily
propagate changes if needed

>
> - Similarly for EAS, the energy computation doesn't take into account the maximum frequency
>    of the policy. This should mean that EAS is taking into consideration frequencies that
>    are not actually available.
>
>
> Regards,
> Pierre