Re: [PATCH v5 06/18] watchdog/hardlockup: Add comments to touch_nmi_watchdog()
From: Petr Mladek <pmladek@suse.com>
Date: 2023-05-23 09:58:29
Also in:
linux-arm-kernel, linux-perf-users, lkml, sparclinux
On Fri 2023-05-19 10:18:30, Douglas Anderson wrote:
quoted hunk ↗ jump to hunk
In preparation for the buddy hardlockup detector, add comments to touch_nmi_watchdog() to make it obvious that it touches the configured hardlockup detector regardless of whether it's backed by an NMI. Also note that arch_touch_nmi_watchdog() may not be architecture-specific. Ideally, we'd like to rename these functions but that is a fairly disruptive change touching a lot of drivers. After discussion [1] the plan is to defer this until a good time. [1] https://lore.kernel.org/r/ZFy0TX1tfhlH8gxj@alley (local) Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v5: - No longer rename touch_nmi_watchdog(), just add comments. Changes in v4: - ("Rename touch_nmi_watchdog() to ...") new for v4. include/linux/nmi.h | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)diff --git a/include/linux/nmi.h b/include/linux/nmi.h index 454fe99c4874..fafab128f37e 100644 --- a/include/linux/nmi.h +++ b/include/linux/nmi.h@@ -125,15 +125,30 @@ void watchdog_nmi_disable(unsigned int cpu); void lockup_detector_reconfigure(void); /** - * touch_nmi_watchdog - restart NMI watchdog timeout. + * touch_nmi_watchdog - manually pet the hardlockup watchdog. * - * If the architecture supports the NMI watchdog, touch_nmi_watchdog() - * may be used to reset the timeout - for code which intentionally - * disables interrupts for a long time. This call is stateless. + * If we support detecting hardlockups, touch_nmi_watchdog() may be + * used to pet the watchdog (reset the timeout) - for code which
Nit: I personally prefer "reset the timeout" over "pet the watchdog".
"pet" is just another ambiguous name as "touch" ;-)
+ * intentionally disables interrupts for a long time. This call is stateless.
+ *
+ * Though this function has "nmi" in the name, the hardlockup watchdog might
+ * not be backed by NMIs. This function will likely be renamed to
+ * touch_hardlockup_watchdog() in the future.
*/
static inline void touch_nmi_watchdog(void)
{
+ /*
+ * Pass on to the hardlockup detector selected via CONFIG_. Note that
+ * the hardlockup detector may not be arch-specific nor using NMIs
+ * and the arch_touch_nmi_watchdog() function will likely be renamed
+ * in the future.
+ */
arch_touch_nmi_watchdog();
+
+ /*
+ * Touching the hardlock detector implcitily pets the
+ * softlockup detector too
+ */s/implcitily/implicitly/ That said, I would remove this comment completely. It describes what is clear from the code. A more useful information would be why it is done. But it is probably clear as well. CPU could not schedule when interrupts are disabled.
touch_softlockup_watchdog(); }
With the removed comment above touch_softlockup_watchdog(): Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr