Re: [External] Re: [PATCH v3] arm64: topology: add source check in arch_cpu_idle_enter()

On Wed, Aug 12, 2026 at 01:02:37PM +0200, Beata Michalska wrote:

On Wed, Aug 12, 2026 at 07:34:52AM +0000, Sean Wang1 wrote:

quoted

On Tus, Aug 11, 2026 at 06:03PM, Beata Michalska wrote:

quoted

quoted

--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c

@@ -175,7 +175,8 @@ void arch_cpu_idle_enter(void)

 	/* Kick in AMU update but only if one has not happened already */
 	if (housekeeping_cpu(cpu, HK_TYPE_TICK) &&
-

time_is_before_jiffies(per_cpu(cpu_amu_samples.last_scale_update, cpu)))

quoted

+

time_is_before_jiffies(per_cpu(cpu_amu_samples.last_scale_update, cpu))
&&

quoted

+	    topology_is_scale_freq_source(SCALE_FREQ_SOURCE_ARCH, cpu))
 		amu_scale_freq_tick();

I'm not entirely convinced you gained a lot by that.
It's one additional check per each enter_idle for case where AMUs are the
source vs 2 additional check when it is not.
Will try to figure out smth less 'invasive'.

First, I think that the rcu_read_lock_sched()/unlock() in
topology_is_scale_freq_source() is unnecessary. arch_cpu_idle_enter()
is called from do_idle() after local_irq_disable() at
kernel/sched/idle.c:340, which satisfies the rcu_sched grace period
requirement. This means we can call rcu_dereference_sched() directly
without explicit RCU lock.

In this particular case RCU locking is not required, though you are exposing
an API that might be used in other curcumstances, so the least we could do is
document that.

quoted

I have two options to propose:

Option A: Keep the helper, but drop the explicit RCU lock

    bool topology_is_scale_freq_source(enum scale_freq_source source,
                                       unsigned int cpu)
    {
        struct scale_freq_data *sfd;
        sfd = rcu_dereference_sched(*per_cpu_ptr(&sft_data, cpu));
        return sfd && sfd->source == source;
    }

Option B: Drop the helper entirely, check directly in arch_cpu_idle_enter()
If a generic exported helper feels too invasive, we can do the
check locally within arch_cpu_idle_enter() without touching
drivers/base/arch_topology.c at all:

    if (housekeeping_cpu(cpu, HK_TYPE_TICK) &&
        time_is_before_jiffies(per_cpu(cpu_amu_samples.last_scale_update, cpu))) {
        struct scale_freq_data *sfd;
        sfd = rcu_dereference_sched(*this_cpu_ptr(&sft_data));
        if (sfd && sfd->source == SCALE_FREQ_SOURCE_ARCH)
            amu_scale_freq_tick();
    }

This keeps the change entirely in arm64 code and avoids adding
a new exported symbol. Which approach would you prefer?

I do not mind this additional helper. Besides, not my place to either mind it
or not.
What I do mind is doing the check in the arch idle enter path. I would rather
see some notification triggered when the source gets changed so that
the previous sfd code can do some state transition that would avoid us having
to run the check in the first place.
Still pondering on that one.
Preferably I would drop that 'tick' call from there completely, but apparently
this was needed on some platforms to make AMU readings more reliable.

How about smth between the lines of:

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index b32f13358fbb1..aff33836488c8 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c

@@ -250,6 +250,22 @@ int arch_freq_get_on_cpu(int cpu)
 	return freq;
 }
 
+static int amu_fie_source_notifier(struct notifier_block *nb,
+				   unsigned long event,
+				   void *data)
+{
+	const struct cpumask *cpus = data;
+
+	if (event == SCALE_FREQ_SOURCE_ARCH)
+		cpumask_andnot(amu_fie_cpus, amu_fie_cpus, cpus);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block amu_fie_nb = {
+	.notifier_call = amu_fie_source_notifier,
+};
+
 static void amu_fie_setup(const struct cpumask *cpus)
 {
 	int cpu;

@@ -274,6 +290,8 @@ static void amu_fie_setup(const struct cpumask *cpus)
 
 	topology_set_scale_freq_source(&amu_sfd, cpus);
 
+	if (cpumask_weight(cpus) == cpumask_weight(amu_fie_cpus))
+		topology_register_scale_freq_source_notifier(&amu_fie_nb);
 	pr_debug("CPUs[%*pbl]: counters will be used for FIE.",
 		 cpumask_pr_args(cpus));
 }

@@ -339,9 +357,9 @@ static int cpuhp_topology_online(unsigned int cpu)
 	}
 
 	cpumask_set_cpu(cpu, amu_fie_cpus);
-
 	topology_set_scale_freq_source(&amu_sfd, cpumask_of(cpu));
-
+	if (cpumask_weight(amu_fie_cpus) == 1)
+		topology_register_scale_freq_source_notifier(&amu_fie_nb);
 	pr_debug("CPU[%u]: counter will be used for FIE.", cpu);
 
 	return 0;

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 8c5e47c28d9a3..096430a99ee00 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c

@@ -22,11 +22,14 @@
 #include <linux/rcupdate.h>
 #include <linux/sched.h>
 #include <linux/units.h>
+#include <linux/notifier.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/hw_pressure.h>
 
 static DEFINE_PER_CPU(struct scale_freq_data __rcu *, sft_data);
+static struct blocking_notifier_head scale_freq_source_change =
+	BLOCKING_NOTIFIER_INIT(scale_freq_source_change);
 static struct cpumask scale_freq_counters_mask;
 static bool scale_freq_invariant;
 DEFINE_PER_CPU(unsigned long, capacity_freq_ref) = 0;

@@ -67,6 +70,18 @@ static void update_scale_freq_invariant(bool status)
 	}
 }
 
+int topology_register_scale_freq_source_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&scale_freq_source_change, nb);
+}
+EXPORT_SYMBOL_GPL(topology_register_scale_freq_source_notifier);
+
+int topology_unregister_scale_freq_source_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&scale_freq_source_change, nb);
+}
+EXPORT_SYMBOL_GPL(topology_unregister_scale_freq_source_notifier);
+
 void topology_set_scale_freq_source(struct scale_freq_data *data,
 				    const struct cpumask *cpus)
 {

@@ -95,6 +110,7 @@ void topology_set_scale_freq_source(struct scale_freq_data *data,
 	rcu_read_unlock();
 
 	update_scale_freq_invariant(true);
+
 }
 EXPORT_SYMBOL_GPL(topology_set_scale_freq_source);
 

@@ -102,8 +118,11 @@ void topology_clear_scale_freq_source(enum scale_freq_source source,
 				      const struct cpumask *cpus)
 {
 	struct scale_freq_data *sfd;
+	cpumask_var_t cleared_mask __free(free_cpumask_var) = CPUMASK_VAR_NULL;
 	int cpu;
 
+	zalloc_cpumask_var(&cleared_mask, GFP_KERNEL);
+
 	rcu_read_lock();
 
 	for_each_cpu(cpu, cpus) {

@@ -112,6 +131,8 @@ void topology_clear_scale_freq_source(enum scale_freq_source source,
 		if (sfd && sfd->source == source) {
 			rcu_assign_pointer(per_cpu(sft_data, cpu), NULL);
 			cpumask_clear_cpu(cpu, &scale_freq_counters_mask);
+			if (cpumask_available(cleared_mask))
+				cpumask_set_cpu(cpu, cleared_mask);
 		}
 	}
 

@@ -124,6 +145,10 @@ void topology_clear_scale_freq_source(enum scale_freq_source source,
 	synchronize_rcu();
 
 	update_scale_freq_invariant(false);
+
+	if (cpumask_available(cleared_mask))
+		blocking_notifier_call_chain(&scale_freq_source_change, source,
+					     cleared_mask);
 }
 EXPORT_SYMBOL_GPL(topology_clear_scale_freq_source);
 

diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index ebd7f8935f969..4c31fd6dff0ef 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h

@@ -48,6 +48,8 @@ struct scale_freq_data {
 void topology_scale_freq_tick(void);
 void topology_set_scale_freq_source(struct scale_freq_data *data, const struct cpumask *cpus);
 void topology_clear_scale_freq_source(enum scale_freq_source source, const struct cpumask *cpus);
+int topology_register_scale_freq_source_notifier(struct notifier_block *nb);
+int topology_unregister_scale_freq_source_notifier(struct notifier_block *nb);
 
 DECLARE_PER_CPU(unsigned long, hw_pressure);
 

---

This is just a rough idea, and needs ironing out the wrinkles, which are there,
but that allows leaving the idle enter as is. I also believe this is the right
approach for the interface itself, although I also see some drawbacks and
potential issues (in its current state), especially that the functionality is
being exposed to modules.
Nevertheless, those are my two cents, untested, just sketched.

---
BR
Beata

quoted

quoted

Aside: I should have probably asked that earlier, but I am not sure I do fully
understand the case we are trying to fix here.
The topology_set_scale_freq_source prefers arch source to others. So if the
AMUs were chosen to server as the source for the freq scale - I do not see
why the sfd would be changed. That would require calling sequence clear-set
to get a different source in place. I do understand the issue itself, though how
did we end up there in the first place ?

The issue arises when topology_clear_scale_freq_source() is called
with SCALE_FREQ_SOURCE_ARCH to explicitly disable AMU-based frequency
scaling. This API is exported (EXPORT_SYMBOL_GPL), so it is designed
to be used by modules or subsystems that need to replace the frequency
invariance mechanism at runtime.
 

So this is the bit I was missing: external module that does the switch
willingly giving up on arch provided freq scale source.
The rest is clear. Thanks.

---
BR
Beata

quoted

After clearing, the tick path (topology_scale_freq_tick()) correctly
skips the AMU update because sft_data is set to NULL. However, the
idle path (arch_cpu_idle_enter()) bypasses this check by calling
amu_scale_freq_tick() directly, so arch_freq_scale still gets
modified by AMU counters.
 
This creates an inconsistency: the tick path respects
topology_clear_scale_freq_source() but the idle path does not.
 
The goal of this patch is to make the idle path consistent with
the tick path, ensuring that topology_clear_scale_freq_source()
fully disables AMU updates across all paths.