On Tue, Nov 07, 2023 at 12:18:20PM +0100, Vincent Guittot wrote:

On Tue, 7 Nov 2023 at 11:38, Will Deacon [off-list ref] wrote:

quoted

On Sat, Nov 04, 2023 at 11:59:07AM +0100, Vincent Guittot wrote:

quoted

Use the new capacity_ref_freq to set the ratio that is used by AMU for
computing the arch_scale_freq_capacity().
This helps to keep everything aligned using the same reference for
computing CPUs capacity.

The default value of the ratio (stored in per_cpu(arch_max_freq_scale))
ensures that arch_scale_freq_capacity() returns max capacity until it is
set to its correct value with the cpu capacity and capacity_ref_freq.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 arch/arm64/kernel/topology.c  | 26 ++++++++++++++------------
 drivers/base/arch_topology.c  | 12 +++++++++++-
 include/linux/arch_topology.h |  1 +
 3 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 817d788cd866..615c1a20129f 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c

@@ -82,7 +82,12 @@ int __init parse_acpi_topology(void)
 #undef pr_fmt
 #define pr_fmt(fmt) "AMU: " fmt

-static DEFINE_PER_CPU_READ_MOSTLY(unsigned long, arch_max_freq_scale);
+/*
+ * Ensure that amu_scale_freq_tick() will return SCHED_CAPACITY_SCALE until
+ * the CPU capacity and its associated frequency have been correctly
+ * initialized.
+ */
+static DEFINE_PER_CPU_READ_MOSTLY(unsigned long, arch_max_freq_scale) =  1UL << (2 * SCHED_CAPACITY_SHIFT);

This doesn't seem to match the comment? SCHED_CAPACITY_SCALE doesn't have
the '2 *' multiplier.

The comment in freq_inv_set_max_ratio() says:

* We use a factor of 2 * SCHED_CAPACITY_SHIFT -> SCHED_CAPACITY_SCALE²
* in order to ensure a good resolution for arch_max_freq_scale for
* very low reference frequencies (down to the KHz range which should
* be unlikely).

Then there is a  "  * arch_max_freq_scale) >> SCHED_CAPACITY_SHIFT"
when computing the scale which brings the result back to
SCHED_CAPACITY_SHIFT

Ah, I see; I'd not spotted that amu_scale_freq_tick() is doing some
arithmetic on the value (it doesn't return anything because it's 'void').
It's slightly confusing because the comment talks about SCHED_CAPACITY_SCALE
whereas all the code works on the shift, but I get it now.

quoted

quoted

 static DEFINE_PER_CPU(u64, arch_const_cycles_prev);
 static DEFINE_PER_CPU(u64, arch_core_cycles_prev);
 static cpumask_var_t amu_fie_cpus;

@@ -112,14 +117,14 @@ static inline bool freq_counters_valid(int cpu)
      return true;
 }

-static int freq_inv_set_max_ratio(int cpu, u64 max_rate, u64 ref_rate)
+void freq_inv_set_max_ratio(int cpu, u64 max_rate)
 {
-     u64 ratio;
+     u64 ratio, ref_rate = arch_timer_get_rate();

      if (unlikely(!max_rate || !ref_rate)) {
-             pr_debug("CPU%d: invalid maximum or reference frequency.\n",
+             WARN_ONCE(1, "CPU%d: invalid maximum or reference frequency.\n",
                       cpu);
-             return -EINVAL;
+             return;
      }

      /*

@@ -139,12 +144,12 @@ static int freq_inv_set_max_ratio(int cpu, u64 max_rate, u64 ref_rate)
      ratio = div64_u64(ratio, max_rate);
      if (!ratio) {
              WARN_ONCE(1, "Reference frequency too low.\n");
-             return -EINVAL;
+             return;
      }

-     per_cpu(arch_max_freq_scale, cpu) = (unsigned long)ratio;
+     WRITE_ONCE(per_cpu(arch_max_freq_scale, cpu), (unsigned long)ratio);

Why is WRITE_ONCE() now needed?

the tick can already use it. We want to make sure to use either the
old or the new one but not an intermediate value

Isn't that already the case without this patch? In other words, this should
be a separate change.

Will

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