linux-kernel - Re: [PATCH V2 2/2] arm64: topology: Reorder init_amu

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] [day] [month] [year] [list]

Message-ID: <20201215044920.fo2n2k5j2snk4pbn@vireshk-i7>
Date:   Tue, 15 Dec 2020 10:19:20 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Ionela Voinescu <ionela.voinescu@....com>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 2/2] arm64: topology: Reorder init_amu_fie() a bit

On 14-12-20, 14:00, Ionela Voinescu wrote:
> I think there could be a potential problem here (it would be unlikely
> but why not fix it :) ). It was in the code before your changes.
> 
> When we enable amu_fie_key here, topology_scale_freq_tick() could be
> called for AMU CPUs, which will compute and set a scale factor. Later
> on, if we happen to find the system not invariant, we disable counter
> based invariance, but a scale factor might have been set already for a
> CPU, which would and should have returned 1024 otherwise (the
> initialisation value of freq_scale).
> 
> 
> Therefore, while here, you could instead do the following:
> 
> cpufreq_inv = cpufreq_supports_freq_invariance();
> 
> if (!cpufreq_inv &&
>     !cpumask_subset(cpu_online_mask, amu_fie_cpus))
>     goto free_valid_mask;
> 
> static_branch_enable(&amu_fie_key);
> 
> pr_info(..);
> 
> if (!cpufreq_inv)
>     rebuild_sched_domains_energy();
> 
> What do you think?

I already had a patch for this, but for a different reason, i.e. to
avoid the enable/disable dance.

       /* We aren't fully invariant yet */
       if (!prev && !cpumask_equal(amu_fie_cpus, cpu_present_mask))
               return;

And this is quite similar to what you have here.

-- 
viresh