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

[Beata Michalska]

On Wed, Aug 12, 2026 at 07:34:52AM +0000, Sean Wang1 wrote:
> On Tus, Aug 11, 2026 at 06:03PM, Beata Michalska wrote:
> 
> > > --- 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)))
> > > +
> > time_is_before_jiffies(per_cpu(cpu_amu_samples.last_scale_update, cpu))
> > &&
> > > +	    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.
> 
> 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.

> 
> > 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

> 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.