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

On Thu, Aug 20, 2026 at 3:11 AM Beata Michalska <[email protected]> wrote:
>
> 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:
> > > 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.
>
> 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);

Should we distinguish between events?

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

As an aside, could we verify the functionality of the AMU's
SYS_AMEVCNTR0_CONST_EL0 in the current CPU before enabling amu_file?
To my knowledge, many chips currently have inaccurate errata in this counter.
Alternatively, could we introduce a command line that allows users to
dynamically disable this feature?

Thanks!
>
> ---
> BR
> Beata
> >
> > >
> > > > 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.
>