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: <20201210112912.GB5300@arm.com>
Date:   Thu, 10 Dec 2020 11:29:12 +0000
From:   Ionela Voinescu <ionela.voinescu@....com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
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

On Thursday 10 Dec 2020 at 16:25:06 (+0530), Viresh Kumar wrote:
> 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.
> 

Okay, I replied separately in regards to the piece of code that confused
things.

> 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.
>  

Replied separately for the patch.

> > 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.
> 

Yes.

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

Yes.

> - 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.
> 

Yes.

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

Yes.

> - 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.
> 

This is the point that needs clarification:

topology_scale_freq_invariant()) = cpufreq_supports_freq_invariance() ||
                                   arch_freq_counters_available(cpu_online_mask);

This checks if the full system is invariant.

The possible scenarios are:

 - All online CPUs support AMUs - arch_freq_counters_available() will
   return true -> topology_scale_freq_invariant() will return true.

 - None of the CPUs support AMUs, or part of the CPUs support AMUs - the
   system is invariant only if cpufreq is invariant (dependent on
   whether the driver implements the proper callbacks that results in
   calling arch_set_freq_scale() in cpufreq core.

 - Either cpufreq does not support invariance or we don't have AMU
   support on all CPUs -> the system is not invariant so we disable
   the AMU static key that guards the calls to
   topology_scale_freq_tick() - we would not want to set a scale factor
   for only a part of the CPUs.

So whether were are or are not invariant does not depend only on the AMU
presence, but also on the cpufreq support for invariance. We have to
disable invariance altogether (including the AMU guarding static key)
if the system is not invariant (it no all CPUs have means to provide the
scale).

Let me know if it makes sense.

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

No worries. I myself sometimes wonder if I missed something. I will be
happy in a few years when we can remove this and use counters for all
CPUs.

Thanks,
Ionela.

> -- 
> viresh