On 09-07-20, 11:17, Ionela Voinescu wrote:

On Thursday 09 Jul 2020 at 12:22:45 (+0530), Viresh Kumar wrote:

quoted

 		cpumask_set_cpu(cpu, valid_cpus);
-		have_policy |= enable_policy_freq_counters(cpu, valid_cpus);
+		update_amu_fie_cpus(cpu, valid_cpus);

I see this as two different pieces of functionality:
 - (1) validate_cpu_freq_invariance_counters(cpu) has the job of validating
   the CPU support, including max_freq_hz.
 - (2) enable_policy_freq_counters() has the job to restrict AMU enablement
   for the CPUs in a policy if all CPUs in the policy support AMUs.

So both of them, separately, support the case of !CONFIG_CPU_FREQ.

quoted

 	}
 
-	/*
-	 * If we are not restricted by cpufreq policies, we only enable
-	 * the use of the AMU feature for FIE if all CPUs support AMU.
-	 * Otherwise, enable_policy_freq_counters has already enabled
-	 * policy cpus.
-	 */
-	if (!have_policy && cpumask_equal(valid_cpus, cpu_present_mask))

This is meant to have the following logic: if for some reason we're not
restricted by policies (according to 2), but all AMU validation was
successful (according to 1), there is no reason not to enable fully AMU
enabled frequency invariance.

I agree that this happening is a cornercase and a reason for which
cpufreq_get_hw_max_freq() was made weak. If some platform has entirely
firmware driven frequency control, but it enables CONFIG_CPU_FREQ
(as is the default) and it defines its own cpufreq_get_hw_max_freq(),
it could benefit from AMU use.

So I did believe it was best for these checks to be decoupled, for this
reason, and potential other reasons in the future, involving more
decoupling from cpufreq.

I do have code in progress to clean the overall interaction between
cpufreq and AMUs, started at [1]. Bear with me on this, it is all
connected :).

Of course I missed few things here.

- I didn't realize that cpufreq_get_hw_max_freq() is defined weak :(

  I understand that we want to support everything that is possible,
  but there is no need to support cases which we may never have
  actually. We have seen code going in the kernel, which no one ever
  ends up using.

  Do we see a case in near future where someone is going to override
  this weak implementation ? If we don't have an actual target for it
  at the moment, then we should probably remove the weak attribute and
  simplify the code.

- I understood earlier that, we don't pick up AMU support unless all
  CPUs of a policy are supported by AMUs, but forgot that later while
  writing the patch. What is the thing with AMUs? Why would some
  platform add it only for some CPUs out of a policy ? Do we have such
  platforms already or in queue ?

Lets discuss more after we have settled on the first point here.

Thanks for review Ionela.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel