Re: [PATCH v4] watchdog: Allow nmi watchdog to use "ref-cycles" event

[Song Liu]

> On May 17, 2023, at 11:44 PM, Yonghong Song <yhs@xxxxxxxx> wrote:
> 
> 
> 
> On 5/17/23 5:25 PM, Song Liu wrote:
>> NMI watchdog permanently consumes one hardware counters per CPU on the
>> system. For systems that use many hardware counters, this causes more
>> aggressive time multiplexing of perf events.
>> OTOH, some CPUs (mostly Intel) support "ref-cycles" event, which is rarely
>> used. Add kernel cmdline arg nmi_watchdog=ref-cycles to configure the
>> watchdog to use "ref-cycles" event instead of "cycles".
> 
> Maybe list some example how this new option will used?

In most case, all we need is to add "nmi_watchdog=refcycles" to kernel args.

> 
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> Signed-off-by: Song Liu <song@xxxxxxxxxx>
>> ---
>> Changes in v4:
>> Fix compile error for !CONFIG_HARDLOCKUP_DETECTOR_PERF. (kernel test bot)
>> Changes in v3:
>> Pivot the design to use kernel arg nmi_watchdog=ref-cycles (Peter)
>> ---
>>  Documentation/admin-guide/kernel-parameters.txt | 5 +++--
>>  include/linux/nmi.h                             | 2 ++
>>  kernel/watchdog.c                               | 2 ++
>>  kernel/watchdog_hld.c                           | 9 +++++++++
>>  4 files changed, 16 insertions(+), 2 deletions(-)
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 9e5bab29685f..d378e23dad7c 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -3593,10 +3593,12 @@
>>   Format: [state][,regs][,debounce][,die]
>>     nmi_watchdog= [KNL,BUGS=X86] Debugging features for SMP kernels
>> - Format: [panic,][nopanic,][num]
>> + Format: [panic,][nopanic,][ref-cycles][num]
>>   Valid num: 0 or 1
>>   0 - turn hardlockup detector in nmi_watchdog off
>>   1 - turn hardlockup detector in nmi_watchdog on
>> + ref-cycles - configure the watchdog with perf event
>> +              "ref-cycles" instead of "cycles"
>>   When panic is specified, panic when an NMI watchdog
>>   timeout occurs (or 'nopanic' to not panic on an NMI
>>   watchdog, if CONFIG_BOOTPARAM_HARDLOCKUP_PANIC is set)
>> @@ -7097,4 +7099,3 @@
>>   memory, and other data can't be written using
>>   xmon commands.
>>   off xmon is disabled.
>> -
>> diff --git a/include/linux/nmi.h b/include/linux/nmi.h
>> index 048c0b9aa623..edfd1bcce0f6 100644
>> --- a/include/linux/nmi.h
>> +++ b/include/linux/nmi.h
>> @@ -102,12 +102,14 @@ extern void hardlockup_detector_perf_disable(void);
>>  extern void hardlockup_detector_perf_enable(void);
>>  extern void hardlockup_detector_perf_cleanup(void);
>>  extern int hardlockup_detector_perf_init(void);
>> +extern void hardlockup_config_perf_event(const char *str);
>>  #else
>>  static inline void hardlockup_detector_perf_stop(void) { }
>>  static inline void hardlockup_detector_perf_restart(void) { }
>>  static inline void hardlockup_detector_perf_disable(void) { }
>>  static inline void hardlockup_detector_perf_enable(void) { }
>>  static inline void hardlockup_detector_perf_cleanup(void) { }
>> +static inline void hardlockup_config_perf_event(const char *str) { }
>>  # if !defined(CONFIG_HAVE_NMI_WATCHDOG)
>>  static inline int hardlockup_detector_perf_init(void) { return -ENODEV; }
>>  static inline void arch_touch_nmi_watchdog(void) {}
>> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
>> index 8e61f21e7e33..fed4f0be8e1a 100644
>> --- a/kernel/watchdog.c
>> +++ b/kernel/watchdog.c
>> @@ -81,6 +81,8 @@ static int __init hardlockup_panic_setup(char *str)
>>   nmi_watchdog_user_enabled = 0;
>>   else if (!strncmp(str, "1", 1))
>>   nmi_watchdog_user_enabled = 1;
>> + else if (!strncmp(str, "ref-cycles", 10))
> 
> str vs. 'ref-cycles' is tested here.
> 
>> + hardlockup_config_perf_event(str);
>>   return 1;
>>  }
>>  __setup("nmi_watchdog=", hardlockup_panic_setup);
>> diff --git a/kernel/watchdog_hld.c b/kernel/watchdog_hld.c
>> index 247bf0b1582c..4deca58ba6ed 100644
>> --- a/kernel/watchdog_hld.c
>> +++ b/kernel/watchdog_hld.c
>> @@ -294,3 +294,12 @@ int __init hardlockup_detector_perf_init(void)
>>   }
>>   return ret;
>>  }
>> +
>> +/**
>> + * hardlockup_config_perf_event - Overwrite config of wd_hw_attr
>> + */
>> +void __init hardlockup_config_perf_event(const char *str)
>> +{
>> + if (!strncmp(str, "ref-cycles", 10))
> 
> It is unnecessarily tested again here.

In that case, we can probably rename the function as 
hardlockup_use_ref_cycles(). I am fine with either way. 

Thanks,
Song