linux-kernel - Re: [PATCH] arm64: topology: Cleanup 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201210105506.gi76peabl2bv5j62@vireshk-i7>
Date:   Thu, 10 Dec 2020 16:25:06 +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] arm64: topology: Cleanup init_amu_fie() a bit

My intent was to keep the logical working of the code as is (in all
the patches I have sent today), let me see if I broke that assumption
somewhere unintentionally.

On 10-12-20, 10:38, Ionela Voinescu wrote:
> I'm first of all replying to discuss the need of this policy analysis
> that enable_policy_freq_counters() does which results in the setting of
> have_policy.
> 
> Basically, that's functions purpose is only to make sure that invariance
> at the level of the policy is consistent: either all CPUs in a policy
> support counters and counters will be used for the scale factor, or
> either none or only some support counters and therefore the default
> cpufreq implementation will be used (arch_set_freq_scale()) for all CPUs
> in a policy.
> 
> If we find that cpufreq policies are not present at all, we only enable
> counter use if all CPUs support them.

Right, and the same must be true after this patch.

> This being said, there is a very important part of your patches in this
> set:
> 
> +	/* Disallow partial use of counters for frequency invariance */
> +	if (!cpumask_equal(amu_fie_cpus, cpu_present_mask))
> +		goto free_valid_mask;

The current code already has this check and so this isn't something
new.
 
> If this is agreed upon, there is a lot more that can be removed from the
> initialisation: enable_policy_freq_counters() can entirely go away plus
> all the checks around it.
> 
> I completely agree that all of this will be more clear if we decided to
> "Disallow partial use of counters for frequency invariance", but I think
> we might have to add it back in again when systems with partial support
> for counters show up.
> 
> I don't agree to not support these systems from the get go as it's
> likely that the first big.LITTLE systems will only have partial support
> for AMUs, but it's only an assumption at this point.

Here is how things move AFAICT:

- We set valid_cpus 1-by-1 with all CPUs that have counters available.

- Once all CPUs of a policy are part of valid_cpus, we update
  amu_fie_cpus with that policies related_cpus.

- Once we are done with all CPUs, we check if cpufreq policy was there
  for any of the CPUs, if not, we update amu_fie_cpus if all present
  CPUs are part of valid_cpus.

- At this point we call static_branch_enable() if amu_fie_cpus is not
  empty (i.e. even if it is partially set).

- But right after that we call static_branch_disable() if we aren't
  invariant (call to topology_scale_freq_invariant()), and this will
  happen if amu_fie_cpus doesn't have all the CPUs there. Isn't it? So
  partial amu support is already disallowed, without cpufreq.

Where am I wrong ? (And I know there is a high possibility of that
happening here :) )..

-- 
viresh