Messages in this thread | | | Date | Thu, 10 Dec 2020 16:25:06 +0530 | From | Viresh Kumar <> | 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
| |