Re: [PATCH] watchdog: Prefer use "ref-cycles" for NMI watchdog

[Song Liu]

> On May 12, 2023, at 4:40 PM, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> 
> On Tue, 9 May 2023 15:17:00 -0700 Song Liu <song@xxxxxxxxxx> 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. Try use "ref-cycles" for the watchdog. If the CPU supports it, so
>> that one more hardware counter is available to the user. If the CPU doesn't
>> support "ref-cycles", fall back to "cycles".
>> 
>> The downside of this change is that users of "ref-cycles" need to disable
>> nmi_watchdog.
>> 
>> ...
>> 
>> @@ -286,6 +286,12 @@ int __init hardlockup_detector_perf_init(void)
>> {
>> int ret = hardlockup_detector_event_create();
>> 
>> + if (ret) {
> 
> If we get here, hardlockup_detector_event_create() has sent a scary
> pr_debug message.
> 
>> + /* Failed to create "ref-cycles", try "cycles" instead */
>> + wd_hw_attr.config = PERF_COUNT_HW_CPU_CYCLES;
>> + ret = hardlockup_detector_event_create();
> 
> So it would be good to emit a followup message here telling users that
> things are OK.  Or tell the user we're retrying with a different
> counter, etc.

How about we ask hardlockup_detector_event_create() not to send pr_debug
message in the first try (something like below)?

Also, I think Peter's concern is valid. If some user daemon monitors 
ref-cycles in the background (I am not aware of such use cases though), 
this could be a real issue. 

Thanks,
Song


diff --git i/kernel/watchdog_hld.c w/kernel/watchdog_hld.c
index f77109d98641..a1d2a43ea31f 100644
--- i/kernel/watchdog_hld.c
+++ w/kernel/watchdog_hld.c
@@ -163,7 +163,7 @@ static void watchdog_overflow_callback(struct perf_event *event,
        return;
 }

-static int hardlockup_detector_event_create(void)
+static int hardlockup_detector_event_create(bool send_warning)
 {
        unsigned int cpu = smp_processor_id();
        struct perf_event_attr *wd_attr;
@@ -176,8 +176,10 @@ static int hardlockup_detector_event_create(void)
        evt = perf_event_create_kernel_counter(wd_attr, cpu, NULL,
                                               watchdog_overflow_callback, NULL);
        if (IS_ERR(evt)) {
-               pr_debug("Perf event create on CPU %d failed with %ld\n", cpu,
-                        PTR_ERR(evt));
+               if (send_warning) {
+                       pr_debug("Perf event create on CPU %d failed with %ld\n", cpu,
+                                PTR_ERR(evt));
+               }
                return PTR_ERR(evt);
        }
        this_cpu_write(watchdog_ev, evt);
@@ -189,7 +191,7 @@ static int hardlockup_detector_event_create(void)
  */
 void hardlockup_detector_perf_enable(void)
 {
-       if (hardlockup_detector_event_create())
+       if (hardlockup_detector_event_create(true))
                return;

        /* use original value for check */
@@ -284,12 +286,12 @@ void __init hardlockup_detector_perf_restart(void)
  */
 int __init hardlockup_detector_perf_init(void)
 {
-       int ret = hardlockup_detector_event_create();
+       int ret = hardlockup_detector_event_create(false);

        if (ret) {
                /* Failed to create "ref-cycles", try "cycles" instead */
                wd_hw_attr.config = PERF_COUNT_HW_CPU_CYCLES;
-               ret = hardlockup_detector_event_create();
+               ret = hardlockup_detector_event_create(true);
        }

        if (ret) {