Messages in this thread | | | Date | Thu, 10 Dec 2020 10:38:15 +0000 | From | Ionela Voinescu <> | Subject | Re: [PATCH] arm64: topology: Cleanup init_amu_fie() a bit |
| |
Hi,
On Thursday 10 Dec 2020 at 12:48:20 (+0530), Viresh Kumar wrote: > Every time I have stumbled upon this routine, I get confused with the > way 'have_policy' is used and I have to dig in to understand why is it > so. >
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.
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;
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.
I'm happy to hear the opinion of other arm64 folk about this.
Many thanks for looking over the code, Ionela.
| |