Re: [PATCH 1/4] watchdog/hardlockup: Always update saved interrupts during check

[Petr Mladek <pmladek@suse.com>]

On Wed 2026-03-04 16:58:35, Doug Anderson wrote:
> Hi,
> 
> On Wed, Mar 4, 2026 at 6:44 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Thu 2026-02-12 14:12:10, Mayank Rungta via B4 Relay wrote:
> > > From: Mayank Rungta <mrungta@google.com>
> > >
> > > Currently, arch_touch_nmi_watchdog() causes an early return that
> > > skips updating hrtimer_interrupts_saved. This leads to stale
> > > comparisons and delayed lockup detection.
> > >
> > > Update the saved interrupt count before checking the touched flag
> > > to ensure detection timeliness.
> >
> > IMHO, it is not that easy, see below.
> >
> > So I am curious. Have you found this when debugging a particular
> > problem or just by reading the code, please?
> 
> As I understand it, Mayank found this because the watchdog was
> reacting significantly more slowly than he expected. In his caes, he
> tracked it down to the fact that 8250 console driver has several calls
> to touch_nmi_watchdog(), including on every call to console_write().
> This caused the watchdog to take _much_ longer to fire.
> 
> On devices that fairly chatty w/ their output to the serial console,
> the console_write() is called almost constantly. That means that the
> watchdog is being touched constantly. If I remember Mayank tracked it
> down as:
> 
> * watchdog_hardlockup_check() called and saves counter (1000)
> * timer runs and updates the timer (1000 -> 1001)
> * touch_nmi_watchdog() is called
> * CPU locks up
> * 10 seconds pass
> * watchdog_hardlockup_check() called and saves counter (1001)
> * 10 seconds pass
> * watchdog_hardlockup_check() called and notices touch

Great visualization!

Nit: It seems to be actually the other way around:

 * 10 seconds pass
 * watchdog_hardlockup_check() called and notices touch and skips updating counters
 * 10 seconds pass
 * watchdog_hardlockup_check() called and saves counter (1001)

> * 10 seconds pass
> * watchdog_hardlockup_check() called and finally detects lockup
> 
> ...so we detect the lockup after 30 seconds, which is pretty bad. With
> his new scheme, we'd detect the lockup in 20 seconds.

Fair enough.

> > > @@ -186,7 +186,21 @@ static void watchdog_hardlockup_kick(void)
> > >
> > >  void watchdog_hardlockup_check(unsigned int cpu, struct pt_regs *regs)
> > >  {
> > > +     bool is_hl;
> > >       int hardlockup_all_cpu_backtrace;
> > > +     /*
> > > +      * Check for a hardlockup by making sure the CPU's timer
> > > +      * interrupt is incrementing. The timer interrupt should have
> > > +      * fired multiple times before we overflow'd. If it hasn't
> > > +      * then this is a good indication the cpu is stuck
> > > +      *
> > > +      * Purposely check this _before_ checking watchdog_hardlockup_touched
> > > +      * so we make sure we still update the saved value of the interrupts.
> > > +      * Without that we'll take an extra round through this function before
> > > +      * we can detect a lockup.
> > > +      */
> > > +
> > > +     is_hl = is_hardlockup(cpu);
> > >
> > >       if (per_cpu(watchdog_hardlockup_touched, cpu)) {
> > >               per_cpu(watchdog_hardlockup_touched, cpu) = false;
> >
> > Hmm, this does not look correct to me.
> >
> > 2. Let's look at is_hardlockup() in detail:
> >
> >     static bool is_hardlockup(unsigned int cpu)
> >     {
> >         int hrint = atomic_read(&per_cpu(hrtimer_interrupts, cpu));
> >
> >         if (per_cpu(hrtimer_interrupts_saved, cpu) == hrint) {
> >                 per_cpu(hrtimer_interrupts_missed, cpu)++;
> >                 if (per_cpu(hrtimer_interrupts_missed, cpu) >= watchdog_hardlockup_miss_thresh)
> >                         return true;
> >
> >                 return false;
> >         }
> >
> >         per_cpu(hrtimer_interrupts_saved, cpu) = hrint;
> >         per_cpu(hrtimer_interrupts_missed, cpu) = 0;
> >
> >         return false;
> >     }
> >
> >     If we call it when the watchdog was touched then
> >     (per_cpu(hrtimer_interrupts_saved, cpu) == hrint)
> >
> >         =>  per_cpu(hrtimer_interrupts_missed, cpu)++;
> >
> >     is called even when watchdog was touched.
> >
> >     As a result, we might report stall which should have been hidden,
> >     for example:
> >
> > CPU0                               CPU1
> >
> >  <NMI>
> >    watchdog_hardlockup_check() # passes
> >      is_hardlockup() # no
> >        hr_int_saved = hr_int;
> >        hr_int_missed = 0;
> >  </NMI>
> >
> >   local_irq_save()
> >     printk()
> >       console_trylock()
> >       console_unlock()
> >         console_flush_all()
> >
> >            touch_nmi_watchdog()
> >
> >                                    // Other CPUs print many messages,
> >                                    // e.g. during boot when initializing a lot of HW
> >                                    for (i=0; i<1000; i++) do
> >                                        printk();
> >
> >       <NMI>
> >         watchdog_hardlockup_check()
> >           is_hardlockup() # yes
> >             hr_int_missed++  # 1
> >
> >           # skip because touched
> >       </NMI>
> >
> >          touch_nmi_watchdog()
> >
> >       <NMI>
> >         watchdog_hardlockup_check()
> >           is_hardlockup() # yes
> >             hr_int_missed++  # 2
> >
> >           # skip because touched
> >       </NMI>
> >
> >     ... repeat many times ...
> >
> >   local_irq_restore()
> >
> >     # this might normally trigger handling of pending IRQs
> >     # including the timers. But IMHO, it can be offloaded
> >     # to a kthread (at least on RT)
> >
> >       <NMI>
> >         watchdog_hardlockup_check()
> >           is_hardlockup() # yes
> >             hr_int_missed++  # might be already 3, 4,...
> >
> >           Report hardlockup even when all the "hr_int_missed"
> >           values should have been ignored because of
> >           touch_watchdog.
> >
> >       </NMI>
> >
> >
> > A solution might be clearing "hrtimer_interrupts_missed"
> > when the watchdog was touched.
> 
> Great catch! When I was thinking about Mayank's patches, I thought
> about them independently. ...and I believe that independently, each
> patch is fine. The problem is that together they have exactly the
> problem you indicated.

Heh, I was not aware that "hrtimer_interrupts_missed" was added by
the 3rd patch. I looked at the final code with all patches applied ;-)

> Clearing "hrtimer_interrupts_missed" seems like the right solution in
> Mayank's patch #3.

OK, this 1st patch moves "is_hardlockup()" up because it has some
"side effects". It adds a 4-line comment to explain it.
But it still causes problems in the 3rd patch.

A better solution might be to separate the check and update/reset
of the values. Something like (on top of this patchset, just
compilation tested):

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 30199eaeb5d7..4d0851f0f412 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -167,18 +167,10 @@ void watchdog_hardlockup_touch_cpu(unsigned int cpu)
 	per_cpu(watchdog_hardlockup_touched, cpu) = true;
 }
 
-static bool is_hardlockup(unsigned int cpu)
+static void watchdog_hardlockup_update_reset(unsigned int cpu)
 {
 	int hrint = atomic_read(&per_cpu(hrtimer_interrupts, cpu));
 
-	if (per_cpu(hrtimer_interrupts_saved, cpu) == hrint) {
-		per_cpu(hrtimer_interrupts_missed, cpu)++;
-		if (per_cpu(hrtimer_interrupts_missed, cpu) >= watchdog_hardlockup_miss_thresh)
-			return true;
-
-		return false;
-	}
-
 	/*
 	 * NOTE: we don't need any fancy atomic_t or READ_ONCE/WRITE_ONCE
 	 * for hrtimer_interrupts_saved. hrtimer_interrupts_saved is
@@ -186,8 +178,20 @@ static bool is_hardlockup(unsigned int cpu)
 	 */
 	per_cpu(hrtimer_interrupts_saved, cpu) = hrint;
 	per_cpu(hrtimer_interrupts_missed, cpu) = 0;
+}
 
-	return false;
+static bool is_hardlockup(unsigned int cpu)
+{
+	int hrint = atomic_read(&per_cpu(hrtimer_interrupts, cpu));
+
+	if (per_cpu(hrtimer_interrupts_saved, cpu) != hrint)
+		return false;
+
+	per_cpu(hrtimer_interrupts_missed, cpu)++;
+	if (per_cpu(hrtimer_interrupts_missed, cpu) < watchdog_hardlockup_miss_thresh)
+		return false;
+
+	return true;
 }
 
 static void watchdog_hardlockup_kick(void)
@@ -200,23 +204,10 @@ static void watchdog_hardlockup_kick(void)
 
 void watchdog_hardlockup_check(unsigned int cpu, struct pt_regs *regs)
 {
-	bool is_hl;
 	int hardlockup_all_cpu_backtrace;
-	/*
-	 * Check for a hardlockup by making sure the CPU's timer
-	 * interrupt is incrementing. The timer interrupt should have
-	 * fired multiple times before we overflow'd. If it hasn't
-	 * then this is a good indication the cpu is stuck
-	 *
-	 * Purposely check this _before_ checking watchdog_hardlockup_touched
-	 * so we make sure we still update the saved value of the interrupts.
-	 * Without that we'll take an extra round through this function before
-	 * we can detect a lockup.
-	 */
-
-	is_hl = is_hardlockup(cpu);
 
 	if (per_cpu(watchdog_hardlockup_touched, cpu)) {
+		watchdog_hardlockup_update_reset(cpu);
 		per_cpu(watchdog_hardlockup_touched, cpu) = false;
 		return;
 	}
@@ -224,7 +215,13 @@ void watchdog_hardlockup_check(unsigned int cpu, struct pt_regs *regs)
 	hardlockup_all_cpu_backtrace = (hardlockup_si_mask & SYS_INFO_ALL_BT) ?
 					1 : sysctl_hardlockup_all_cpu_backtrace;
 
-	if (is_hl) {
+	/*
+	 * Check for a hardlockup by making sure the CPU's timer
+	 * interrupt is incrementing. The timer interrupt should have
+	 * fired multiple times before we overflow'd. If it hasn't
+	 * then this is a good indication the cpu is stuck
+	 */
+	if (is_hardlockup(cpu)) {
 		unsigned int this_cpu = smp_processor_id();
 		unsigned long flags;
 
@@ -290,6 +287,7 @@ void watchdog_hardlockup_check(unsigned int cpu, struct pt_regs *regs)
 
 		per_cpu(watchdog_hardlockup_warned, cpu) = true;
 	} else {
+		watchdog_hardlockup_update_reset(cpu);
 		per_cpu(watchdog_hardlockup_warned, cpu) = false;
 	}
 }

> > But honestly, I am not sure if this is worth the complexity.
> >
> >
> > Higher level look:
> > ------------------
> >
> > My understanding is that this patch has an effect only when
> > touch_nmi_watchdog() is called as frequently as
> > watchdog_hardlockup_check().
> >
> > The original code gives the system more time to recover after
> > a known stall (touch_nmi_watchdog() called).
> >
> > The new code is more eager to report a stall. It might be more prone
> > to report "false" positives.
> >
> > IMHO, the root of the problem is that touch_nmi_watchdog() is
> > called too frequently. And this patch is rather dancing around
> > then fixing it.
> 
> I don't think it's really any more likely to report false positives
> after the bug you pointed out is fixed. The old watchdog was just too
> conservative. With Mayank's proposal I think calling
> touch_nmi_watchdog() should reset the watchdog the same amount as
> letting the hrtimer run once and that seems like a very reasonable
> interpretation.

Fair enough.

> > Alternative:
> > ------------
> >
> > An alternative solution might to detect and report when too many
> > watchdog_hardlockup_check() calls are ignored because of
> > touch_nmi_watchdog().
> >
> > It might help to find a mis-use of touch_nmi_watchdog(). The question
> > is what details should be reported in this case.
> >
> > It should be optional because touch_nmi_watchdog() is supposed
> > to hide "well-known" sinners after all.
> 
> Hmmmm. I certainly support trying to reduce the number of places that
> call touch_nmi_watchdog(), but at the same time I don't think Mayank's
> patch is "dancing around" the problem. IMO considering the
> touch_nmi_watchdog() to be "pretend a timer interrupt fired" is the
> intuitive way one would think the call should work. The fact that the
> code gave an entire extra 10 seconds before the watchdog could be
> caught just feels like a bug that should be fixed.
> 
> For the 8250 driver in particular, it looks like the
> touch_nmi_watchdog() was removed from serial8250_console_write() as
> part of nbcon, but then that got reverted. That would still leave two
> other touch_nmi_watchdog() calls in that driver...

Sigh, it seems that touch_nmi_watchdog() can't be removed easily.

Best Regards,
Petr