[PATCH v2 0/2] Update the watchdog period according to real CPU frequency

[PATCH v2 0/2] Update the watchdog period according to real CPU frequency

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

watchdog perf needs architecture to provide method for converting the watchdog
thresh to counter period. For arm64 we're using the max CPU frequency for
doing the conversion which is from cpufreq driver. But some cpufreq driver
are registered lately, for example cppc_cpufreq will be registered at late
initcall which is after the initialization of watchdog perf (initialized in
armv8_pmuv3 of device initcall). In such case the period of watchdog will not
be accurate enough. Fix this by registering a cpufreq notifier and update the
watchdog period once the cpufreq driver is initialized.

Change since v1:
- Handle the theoretical race condition and other comments from Doug, thanks
Link: https://lore.kernel.org/all/20250307021811.46981-1-yangyicong@huawei.com/

Yicong Yang (2):
  watchdog/perf: Provide function for adjusting the event period
  arm64/watchdog_hld: Add a cpufreq notifier for update watchdog thresh

 arch/arm64/kernel/watchdog_hld.c | 58 ++++++++++++++++++++++++++++++++
 include/linux/nmi.h              |  2 ++
 kernel/watchdog_perf.c           | 23 +++++++++++++
 3 files changed, 83 insertions(+)

-- 
2.24.0

[PATCH v2 1/2] watchdog/perf: Provide function for adjusting the event period

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

Architecture's using perf events for hard lockup detection needs to
convert the watchdog_thresh to the event's period, some architecture
for example arm64 perform this conversion using the CPU's maximum
frequency which will be acquired by cpufreq. However by the time
the lockup detector's initialized the cpufreq driver may not be
initialized, thus launch a watchdog with inaccurate period. Provide
a function hardlockup_detector_perf_adjust_period() to allowing
adjust the event period. Then architecture can update with more
accurate period if cpufreq is initialized.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 include/linux/nmi.h    |  2 ++
 kernel/watchdog_perf.c | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/include/linux/nmi.h b/include/linux/nmi.h
index e78fa535f61d..0a6d8a8d2d5b 100644
--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -103,10 +103,12 @@ void watchdog_hardlockup_check(unsigned int cpu, struct pt_regs *regs);
 extern void hardlockup_detector_perf_stop(void);
 extern void hardlockup_detector_perf_restart(void);
 extern void hardlockup_config_perf_event(const char *str);
+extern void hardlockup_detector_perf_adjust_period(int cpu, u64 period);
 #else
 static inline void hardlockup_detector_perf_stop(void) { }
 static inline void hardlockup_detector_perf_restart(void) { }
 static inline void hardlockup_config_perf_event(const char *str) { }
+static inline void hardlockup_detector_perf_adjust_period(int cpu, u64 period) { }
 #endif
 
 void watchdog_hardlockup_stop(void);
diff --git a/kernel/watchdog_perf.c b/kernel/watchdog_perf.c
index 75af12ff774e..41c299e63fab 100644
--- a/kernel/watchdog_perf.c
+++ b/kernel/watchdog_perf.c
@@ -186,6 +186,29 @@ void watchdog_hardlockup_disable(unsigned int cpu)
 	}
 }
 
+/**
+ * hardlockup_detector_perf_adjust_period - Adjust the event period due
+ *                                          to cpu frequency change
+ * @cpu: The CPU whose event period will be adjusted
+ * @period: The target period to be set
+ */
+void hardlockup_detector_perf_adjust_period(int cpu, u64 period)
+{
+	struct perf_event *event = per_cpu(watchdog_ev, cpu);
+
+	if (!(watchdog_enabled & WATCHDOG_HARDLOCKUP_ENABLED))
+		return;
+
+	if (!event)
+		return;
+
+	if (event->attr.sample_period == period)
+		return;
+
+	if (perf_event_period(event, period))
+		pr_err("failed to change period to %llu\n", period);
+}
+
 /**
  * hardlockup_detector_perf_stop - Globally stop watchdog events
  *
-- 
2.24.0

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

[Yicong Yang]

From: Yicong Yang <yangyicong@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@hisilicon.com>
---
 arch/arm64/kernel/watchdog_hld.c | 58 ++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/arch/arm64/kernel/watchdog_hld.c b/arch/arm64/kernel/watchdog_hld.c
index dcd25322127c..e55548cb26df 100644
--- a/arch/arm64/kernel/watchdog_hld.c
+++ b/arch/arm64/kernel/watchdog_hld.c
@@ -34,3 +34,61 @@ bool __init arch_perf_nmi_is_available(void)
 	 */
 	return arm_pmu_irq_is_nmi();
 }
+
+static int watchdog_perf_update_period(void *data)
+{
+	int cpu = raw_smp_processor_id();
+	u64 max_cpu_freq, new_period;
+
+	max_cpu_freq = cpufreq_get_hw_max_freq(cpu) * 1000UL;
+	if (!max_cpu_freq)
+		return 0;
+
+	new_period = watchdog_thresh * max_cpu_freq;
+	hardlockup_detector_perf_adjust_period(cpu, new_period);
+
+	return 0;
+}
+
+static int watchdog_freq_notifier_callback(struct notifier_block *nb,
+					   unsigned long val, void *data)
+{
+	struct cpufreq_policy *policy = data;
+	int cpu;
+
+	if (val != CPUFREQ_CREATE_POLICY)
+		return NOTIFY_DONE;
+
+	/*
+	 * Let each online CPU related to the policy update the period by their
+	 * own. This will serialize with the framework on start/stop the lockup
+	 * detector (softlockup_{start,stop}_all) and avoid potential race
+	 * condition. Otherwise we may have below theoretical race condition:
+	 * (core 0/1 share the same policy)
+	 * [core 0]                      [core 1]
+	 *                               hardlockup_detector_event_create()
+	 *                                 hw_nmi_get_sample_period()
+	 * (cpufreq registered, notifier callback invoked)
+	 * watchdog_freq_notifier_callback()
+	 *   watchdog_perf_update_period()
+	 *   (since core 1's event's not yet created,
+	 *    the period is not set)
+	 *                                 perf_event_create_kernel_counter()
+	 *                                 (event's period is SAFE_MAX_CPU_FREQ)
+	 */
+	for_each_cpu(cpu, policy->cpus)
+		smp_call_on_cpu(cpu, watchdog_perf_update_period, NULL, false);
+
+	return NOTIFY_DONE;
+}
+
+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);
-- 
2.24.0

Re: [PATCH v2 0/2] Update the watchdog period according to real CPU frequency

[Andrew Morton]

On Mon, 12 May 2025 21:09:17 +0800 Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> watchdog perf needs architecture to provide method for converting the watchdog
> thresh to counter period. For arm64 we're using the max CPU frequency for
> doing the conversion which is from cpufreq driver. But some cpufreq driver
> are registered lately, for example cppc_cpufreq will be registered at late
> initcall which is after the initialization of watchdog perf (initialized in
> armv8_pmuv3 of device initcall). In such case the period of watchdog will not
> be accurate enough. Fix this by registering a cpufreq notifier and update the
> watchdog period once the cpufreq driver is initialized.
> 

Thanks.  Thoughts.

1: What is the impact of this change?  Is the current code causing
   problems?  If so, what are they?  How is the end-user experience
   improved by this change?  Important info!

2: As far as I can tell, this patchset impacts arm64 only.  Do you
   think that other architectures should implement this?

3: As far as I can tell, this patchset affects all cpufreq drivers
   which use late_initcall() (on arm64, of course).  Is this correct?

4: It is asserted that we should use the *maximum* possible CPU
   frequency for this calculation.  Why?  I assume this is because we
   care about the minimum watchdog period?

Can I assume that the ARM maintainers will be handling this?

Re: [PATCH v2 1/2] watchdog/perf: Provide function for adjusting the event period

[Andrew Morton]

On Mon, 12 May 2025 21:09:18 +0800 Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> Architecture's using perf events for hard lockup detection needs to
> convert the watchdog_thresh to the event's period, some architecture
> for example arm64 perform this conversion using the CPU's maximum
> frequency which will be acquired by cpufreq. However by the time
> the lockup detector's initialized the cpufreq driver may not be
> initialized, thus launch a watchdog with inaccurate period. Provide
> a function hardlockup_detector_perf_adjust_period() to allowing
> adjust the event period. Then architecture can update with more
> accurate period if cpufreq is initialized.
> 
> ...
>  
> +/**
> + * hardlockup_detector_perf_adjust_period - Adjust the event period due
> + *                                          to cpu frequency change
> + * @cpu: The CPU whose event period will be adjusted
> + * @period: The target period to be set
> + */
> +void hardlockup_detector_perf_adjust_period(int cpu, u64 period)
> +{
> +	struct perf_event *event = per_cpu(watchdog_ev, cpu);
> +
> +	if (!(watchdog_enabled & WATCHDOG_HARDLOCKUP_ENABLED))
> +		return;

Is this the right thing to do?  Would it be better to proceed with the
alteration of the period so that the state is correct if
WATCHDOG_HARDLOCKUP_ENABLED is later enabled?  (If that's possible).


> +	if (!event)
> +		return;
> +
> +	if (event->attr.sample_period == period)
> +		return;
> +
> +	if (perf_event_period(event, period))
> +		pr_err("failed to change period to %llu\n", period);
> +}
> +
>  /**
>   * hardlockup_detector_perf_stop - Globally stop watchdog events
>   *
> -- 
> 2.24.0

Re: [PATCH v2 0/2] Update the watchdog period according to real CPU frequency

[Yicong Yang]

On 2025/5/13 7:06, Andrew Morton wrote:
> On Mon, 12 May 2025 21:09:17 +0800 Yicong Yang <yangyicong@huawei.com> wrote:
> 
>> From: Yicong Yang <yangyicong@hisilicon.com>
>>
>> watchdog perf needs architecture to provide method for converting the watchdog
>> thresh to counter period. For arm64 we're using the max CPU frequency for
>> doing the conversion which is from cpufreq driver. But some cpufreq driver
>> are registered lately, for example cppc_cpufreq will be registered at late
>> initcall which is after the initialization of watchdog perf (initialized in
>> armv8_pmuv3 of device initcall). In such case the period of watchdog will not
>> be accurate enough. Fix this by registering a cpufreq notifier and update the
>> watchdog period once the cpufreq driver is initialized.
>>
> 
> Thanks.  Thoughts.
> 
> 1: What is the impact of this change?  Is the current code causing
>    problems?  If so, what are they?  How is the end-user experience
>    improved by this change?  Important info!
> 

This will make NMI watchdog (hardlockup detector) work more accurately. HARDLOCKUP_DETECTOR_PERF
is driven by the PMU sample interrupts of cpu cycle event with period related to the watchdog
threshold.  User will set the watchdog threshold in seconds (e.g. 10s by default) and for
HARDLOCKUP_DETECTOR_PERF we need to convert the seconds to cycles to setup the PMU counter.
The coversion method is provided by the arhitecture by implementing hw_nmi_get_sample_period().
For arm64 it's using max_cpufreq to do the conversion:
cycle_event_period = threshold(s) * max_cpufreq(hz)

Since arm64 doesn't have an arthictectural way to get the max_cpufreq, we use cpufreq_driver
to get it. If cpufreq_driver is not available, currently we use a safe max_cpufreq as 5GHz.
Without this patchset, if the cpufreq_driver is initialized after the hardlockup detector we'll
use 5GHz for calculating the event period. It's my case here as described in the coverletter.
That means in the default case (10s threshold) if the real max_cpufreq is 2.5GHz, the NMI watchdog
is actually working in a 20s period. With this patchset the period can be calibrated after
the cpufreq driver is initialized, much more accurate.

> 2: As far as I can tell, this patchset impacts arm64 only.  Do you
>    think that other architectures should implement this?

It's highly depends on the architecure's implementation of their HARDLOCKUP_DETECTOR_PERF (the
implementation of hw_nmi_get_sample_period()). If other architectures can gain the max_cpufreq
without cpufreq driver they don't have this problems (seems x86 implement in another way
and can gain the cpufreq by some arhitectural way).

> 
> 3: As far as I can tell, this patchset affects all cpufreq drivers
>    which use late_initcall() (on arm64, of course).  Is this correct?
> 

cpufreq drivers are not touched. we registered a notifier block to the cpufreq framework
so the changes to the cpufreq will notify us to see whether it's needed to modify the
event period.


> 4: It is asserted that we should use the *maximum* possible CPU
>    frequency for this calculation.  Why?  I assume this is because we
>    care about the minimum watchdog period?
> 

I think it may because it's impossbile to use the current frequency for calculating the counter
period as it may change at any time. Using maximum frequency will make the real period as close
as possible to the expected watchdog thresh in a simplest way.

> Can I assume that the ARM maintainers will be handling this?
> .

I think yes since it only affects the arm64's implementation of HARDLOCKUP_DETECTOR_PERF.

Thanks.

Re: [PATCH v2 1/2] watchdog/perf: Provide function for adjusting the event period

[Yicong Yang]

On 2025/5/13 7:07, Andrew Morton wrote:
> On Mon, 12 May 2025 21:09:18 +0800 Yicong Yang <yangyicong@huawei.com> wrote:
> 
>> From: Yicong Yang <yangyicong@hisilicon.com>
>>
>> Architecture's using perf events for hard lockup detection needs to
>> convert the watchdog_thresh to the event's period, some architecture
>> for example arm64 perform this conversion using the CPU's maximum
>> frequency which will be acquired by cpufreq. However by the time
>> the lockup detector's initialized the cpufreq driver may not be
>> initialized, thus launch a watchdog with inaccurate period. Provide
>> a function hardlockup_detector_perf_adjust_period() to allowing
>> adjust the event period. Then architecture can update with more
>> accurate period if cpufreq is initialized.
>>
>> ...
>>  
>> +/**
>> + * hardlockup_detector_perf_adjust_period - Adjust the event period due
>> + *                                          to cpu frequency change
>> + * @cpu: The CPU whose event period will be adjusted
>> + * @period: The target period to be set
>> + */
>> +void hardlockup_detector_perf_adjust_period(int cpu, u64 period)
>> +{
>> +	struct perf_event *event = per_cpu(watchdog_ev, cpu);
>> +
>> +	if (!(watchdog_enabled & WATCHDOG_HARDLOCKUP_ENABLED))
>> +		return;
> 
> Is this the right thing to do?  Would it be better to proceed with the
> alteration of the period so that the state is correct if
> WATCHDOG_HARDLOCKUP_ENABLED is later enabled?  (If that's possible).
> 

This is just a check to see whether we need to proceed. WATCHDOG_HARDLOCKUP_ENABLED
will be set if:
1. hardlockup is supported
2. user doesn't disable it by cmdline/sysctl

If WATCHDOG_HARDLOCKUP_ENABLED is not set the hardlockup detector won't be initialized
as well (in watchdog_enable()), then we don't need to adjust the period as well.

> 
>> +	if (!event)
>> +		return;
>> +
>> +	if (event->attr.sample_period == period)
>> +		return;
>> +
>> +	if (perf_event_period(event, period))
>> +		pr_err("failed to change period to %llu\n", period);
>> +}
>> +
>>  /**
>>   * hardlockup_detector_perf_stop - Globally stop watchdog events
>>   *
>> -- 
>> 2.24.0
> 
> .
> 

Re: [PATCH v2 0/2] Update the watchdog period according to real CPU frequency

[Will Deacon]

On Mon, May 12, 2025 at 04:06:12PM -0700, Andrew Morton wrote:
> On Mon, 12 May 2025 21:09:17 +0800 Yicong Yang <yangyicong@huawei.com> wrote:
> 
> > From: Yicong Yang <yangyicong@hisilicon.com>
> > 
> > watchdog perf needs architecture to provide method for converting the watchdog
> > thresh to counter period. For arm64 we're using the max CPU frequency for
> > doing the conversion which is from cpufreq driver. But some cpufreq driver
> > are registered lately, for example cppc_cpufreq will be registered at late
> > initcall which is after the initialization of watchdog perf (initialized in
> > armv8_pmuv3 of device initcall). In such case the period of watchdog will not
> > be accurate enough. Fix this by registering a cpufreq notifier and update the
> > watchdog period once the cpufreq driver is initialized.
> > 
> 
> Thanks.  Thoughts.
> 
> 1: What is the impact of this change?  Is the current code causing
>    problems?  If so, what are they?  How is the end-user experience
>    improved by this change?  Important info!
> 
> 2: As far as I can tell, this patchset impacts arm64 only.  Do you
>    think that other architectures should implement this?
> 
> 3: As far as I can tell, this patchset affects all cpufreq drivers
>    which use late_initcall() (on arm64, of course).  Is this correct?
> 
> 4: It is asserted that we should use the *maximum* possible CPU
>    frequency for this calculation.  Why?  I assume this is because we
>    care about the minimum watchdog period?
> 
> Can I assume that the ARM maintainers will be handling this?

Argh, this has all ended up in my spam for some reason...

Lemme go fish the patches back out. Andrew -- we can take these via
arm64 if you like (although I have some comments on the code first).

Will

Re: [PATCH v2 0/2] Update the watchdog period according to real CPU frequency

[Andrew Morton]

On Fri, 27 Jun 2025 16:26:15 +0100 Will Deacon <will@kernel.org> wrote:

> > Can I assume that the ARM maintainers will be handling this?
> 
> Argh, this has all ended up in my spam for some reason...

So you won't be buying these ED meds?

> Lemme go fish the patches back out. Andrew -- we can take these via
> arm64 if you like (although I have some comments on the code first).

No probs, I dropped the mm.git copy.