[PATCH 2/2] arm64/watchdog_hld: Add a cpufreq notifier for update watchdog thresh

[Doug Anderson]

Hi,

On Tue, Mar 11, 2025 at 1:33 AM Yicong Yang <yangyicong at huawei.com> wrote:
>
> On 2025/3/8 0:53, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Mar 6, 2025 at 6:18 PM Yicong Yang <yangyicong at huawei.com> wrote:
> >>
> >> From: Yicong Yang <yangyicong at hisilicon.com>
> >>
> >> arm64 depends on the cpufreq driver to gain the maximum cpu frequency
> >> to convert the watchdog_thresh to perf event period. cpufreq drivers
> >> like cppc_cpufreq will be initialized lately after the initializing of
> >> the hard lockup detector so just use a safe cpufreq which will be
> >> inaccurency. Use a cpufreq notifier to adjust the event's period to
> >> a more accurate one.
> >>
> >> Signed-off-by: Yicong Yang <yangyicong at hisilicon.com>
> >> ---
> >>  arch/arm64/kernel/watchdog_hld.c | 34 ++++++++++++++++++++++++++++++++
> >>  1 file changed, 34 insertions(+)
> >>
> >> diff --git a/arch/arm64/kernel/watchdog_hld.c b/arch/arm64/kernel/watchdog_hld.c
> >> index dcd25322127c..719ae39e8bc0 100644
> >> --- a/arch/arm64/kernel/watchdog_hld.c
> >> +++ b/arch/arm64/kernel/watchdog_hld.c
> >> @@ -34,3 +34,37 @@ bool __init arch_perf_nmi_is_available(void)
> >>          */
> >>         return arm_pmu_irq_is_nmi();
> >>  }
> >> +
> >> +static int watchdog_freq_notifier_callback(struct notifier_block *nb,
> >> +                                          unsigned long val, void *data)
> >> +{
> >> +       struct cpufreq_policy *policy = data;
> >> +       unsigned long max_cpu_freq;
> >
> > Feels like max_cpu_freq should be a u64. It probably should have been
> > u64 in hw_nmi_get_sample_period() as well. Otherwise you could end up
> > with a problem if the max CPU frequency is above ~4.3GHz and "unsigned
> > long" is 32-bits (I think it technically could be?)
> >
>
> I guess this won't happen for arm64 and we're in kernel space. here I
> keep the same type with hw_nmi_get_sample_period() which is also using
> "unsigned long".

I guess I won't insist, but given that you use u64 explicitly
elsewhere it feels like you should be explicit that you need 64-bits
here too.


> >> +       for_each_cpu(cpu, policy->cpus) {
> >> +               max_cpu_freq = cpufreq_get_hw_max_freq(cpu) * 1000UL;
> >> +               if (!max_cpu_freq)
> >> +                       continue;
> >> +
> >> +               new_period = watchdog_thresh * max_cpu_freq;
> >> +               hardlockup_detector_perf_adjust_period(cpu, new_period);
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static struct notifier_block watchdog_freq_notifier = {
> >> +       .notifier_call = watchdog_freq_notifier_callback,
> >> +};
> >> +
> >> +static int __init init_watchdog_freq_notifier(void)
> >> +{
> >> +       return cpufreq_register_notifier(&watchdog_freq_notifier,
> >> +                                        CPUFREQ_POLICY_NOTIFIER);
> >> +}
> >> +core_initcall(init_watchdog_freq_notifier);
> >
> > I haven't dug into everything, but don't you have a potential blind
> > spot, though? What if the order is this:
> >
> > 1. hw_nmi_get_sample_period() is called
> > 2. cpufreq driver probes
> > 3. init_watchdog_freq_notifier() initcall runs
> >
> > Since the cpufreq driver proved before you registered for the
> > notifier, won't you miss the creation of the policy? Maybe instead of
> > registering for an initcall you should somehow trigger off the first
> > call to hw_nmi_get_sample_period() and register at that point in time?
> >
>
> for arm64 the watchdog_perf is registered after armv8 PMU driver's init,
> so the sequence is like:
>
> # core_initcall
> init_watchdog_freq_notifier()
> # device_initcall
> armv8_pmu_driver_init()
>   lockup_detector_retry_init()
>     schedule_work(&detector_work);
> # in detector work
> hw_nmi_get_sample_period() // notifier's already been registered
>
> so we'll always get the correct frequency, either by the cpufreq
> already registered or by the notifier which will update the
> frequency. Please correct me if I missed anything :)

Hmm, OK. So this relies on the fact that because the
init_watchdog_freq_notifier() is at "core" level that it'll pretty
much init before any of the other things. This type of thing is always
a little fragile but I guess it's OK.

What about if the order is different though? Again, I haven't traced
through it all, but I'm thinking:

# core_initcall
init_watchdog_freq_notifier()
# device_initcall
<cpufreq driver policy is created>

Now hardlockup_detector_perf_adjust_period() will be called before
armv8_pmu_driver_init(). Is that a problem?

I guess even worse would be if async probe was on and the cpufreq
driver and PMU driver were running at the same time?


-Doug