Hi Ionela,

Overall I think this approach is much better, and apart from a few nits below
this is looking pretty good.

On 2/11/20 6:45 PM, Ionela Voinescu wrote:

                                            ^^^^^
<pedantic hat on>: s/up to/down to/

+	 * unlikely).
+	 */
+	ratio = (u64)arch_timer_get_rate() << (2 * SCHED_CAPACITY_SHIFT);
+	ratio = div64_u64(ratio, max_freq_hz);
+	if (!ratio) {
+		pr_err("System timer frequency too low.\n");

Should that be a WARN_ONCE() instead? If the arch timer freq is too low,
we'll end up spamming this message, since we go through this for all CPUs.

+		return -EINVAL;
+	}
+
+	per_cpu(arch_max_freq_scale, cpu) = (unsigned long)ratio;
+

It occurred to me that this isn't strictly speaking a per-CPU information as
it only depends on the max possible frequency. Not really worth bothering
about though, I think.

+	return 0;
+}
+
+static inline int
+enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus)
+{
+	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+
+	if (!policy) {
+		pr_debug("CPU%d: No cpufreq policy found.\n", cpu);
+		return false;
+	}
+
+	if (cpumask_subset(policy->related_cpus, valid_cpus)) {
+		cpumask_or(amu_fie_cpus, policy->related_cpus,
+			   amu_fie_cpus);
+		pr_info("CPUs[%*pbl]: counters will be used for FIE.",
+			cpumask_pr_args(amu_fie_cpus));

Could we have a single print of all CPUs in one go? AIUI this will generate a
line per cpufreq policy. Maybe just something at the tail of init_amu_fie():

if (!cpumask_empty(amu_fie_cpus))
	pr_info(<blah>);

+	}
+
+	cpufreq_cpu_put(policy);
+
+	return true;
+}
+
+static int __init init_amu_fie(void)
+{
+	cpumask_var_t valid_cpus;
+	bool have_policy = false;
+	int cpu;
+
+	if (!zalloc_cpumask_var(&valid_cpus, GFP_KERNEL) ||
+	    !zalloc_cpumask_var(&amu_fie_cpus, GFP_KERNEL))
+		return -ENOMEM;
+
+	for_each_possible_cpu(cpu) {
+		if (validate_cpu_freq_invariance_counters(cpu))
+			continue;
+		cpumask_set_cpu(cpu, valid_cpus);
+		have_policy = enable_policy_freq_counters(cpu, valid_cpus) ||
+			      have_policy;

What about:
		have_policy |= enable_policy_freq_counters(cpu, valid_cpus);

+	}
+
+	if (!have_policy) {
+		/*
+		 * 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 (cpumask_equal(valid_cpus, cpu_possible_mask)) {

Mmm so I'm thinking what we want here is the cpu_present_mask rather than
the possible one. This is very corner-casy, but I think that if we fail to
boot a secondary, we'll have it possible but not present.

While at it you could make the loop only target present CPUs, but I think the
one bit that matters is this check right here (!present should fail at
validate_cpu_freq_invariance_counters()).

+			cpumask_or(amu_fie_cpus, amu_fie_cpus, valid_cpus);
+			pr_info("CPUs[%*pbl]: counters will be used for FIE.",
+				cpumask_pr_args(amu_fie_cpus));
+		}
+	}
+
+	free_cpumask_var(valid_cpus);
+
+	return 0;
+}
+late_initcall_sync(init_amu_fie);
+
+bool topology_cpu_freq_counters(struct cpumask *cpus)
+{
+	return cpumask_available(amu_fie_cpus) &&
+	       cpumask_subset(cpus, amu_fie_cpus);
+}
+
+void topology_scale_freq_tick(void)
+{
+	u64 prev_core_cnt, prev_const_cnt;
+	u64 core_cnt, const_cnt, scale;
+	int cpu = smp_processor_id();
+
+	if (!cpumask_available(amu_fie_cpus) ||
+	    !cpumask_test_cpu(cpu, amu_fie_cpus))
+		return;

It might be a good idea to have a static key to gate our entry into this
function - that way we can lessen our impact on older platforms (without AMUs)
running a recent kernel with CONFIG_ARM64_AMU_EXTN=y.

x86 does just that, if you look at their arch_scale_freq_tick()
implementation. FWIW I don't think we should bother with playing with the
key counter to count AMU-enabled CPUs, just enable it at startup if we have

1 such CPU and let the cpumask drive the rest.

In your check here, the static key check could replace the cpumask_available()
check. The static key could also be used for topology_cpu_freq_counters().

+
+	const_cnt = read_sysreg_s(SYS_AMEVCNTR0_CONST_EL0);
+	core_cnt = read_sysreg_s(SYS_AMEVCNTR0_CORE_EL0);
+	prev_const_cnt = this_cpu_read(arch_const_cycles_prev);
+	prev_core_cnt = this_cpu_read(arch_core_cycles_prev);
+
+	if (unlikely(core_cnt <= prev_core_cnt ||
+		     const_cnt <= prev_const_cnt))
+		goto store_and_exit;
+
+	/*
+	 *	    /\core    arch_max_freq_scale
+	 * scale =  ------- * --------------------
+	 *	    /\const   SCHED_CAPACITY_SCALE
+	 *
+	 * We shift by SCHED_CAPACITY_SHIFT (divide by SCHED_CAPACITY_SCALE)
+	 * in order to compensate for the SCHED_CAPACITY_SCALE² factor in
+	 * arch_max_freq_scale (used to ensure its resolution) while keeping
+	 * the scale value in the 0-SCHED_CAPACITY_SCALE capacity range.
+	 */

A simple "See validate_cpu_freq_invariance_counters() for details on the
scale factor" would suffice wrt the shifting details.

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