Re: [PATCH v4 05/17] watchdog/hardlockup: Rename touch_nmi_watchdog() to touch_hardlockup_watchdog()

Nicholas Piggin
2023-05-05T02:52:23Z

On Fri May 5, 2023 at 8:13 AM AEST, Douglas Anderson wrote:
> In preparation for the buddy hardlockup detector, rename
> touch_nmi_watchdog() to touch_hardlockup_watchdog() to make it clear
> that it will touch whatever hardlockup detector is configured. We'll
> add a #define for the old name (touch_nmi_watchdog) so that we don't
> have to touch every piece of code referring to the old name.

Is this really helpful? Now it's got two names Could just leave it.
If you insist then it'd be better just to rename everything in one
go at the end of a merge window IMO. Conflicts would be trivial.

> Ideally this change would also rename the arch_touch_nmi_watchdog(),
> but that is harder since arch_touch_nmi_watchdog() is exported with
> EXPORT_SYMBOL() and thus is ABI. Add a comment next to the call to
> hopefully alleviate some of the confusion here.

We don't keep ABI fixed upstream.

Thanks,
Nick

>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v4:
> - ("Rename touch_nmi_watchdog() to ...") new for v4.
>
> include/linux/nmi.h | 27 ++++++++++++++++++++++-----
> 1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/nmi.h b/include/linux/nmi.h
> index 454fe99c4874..35d09d70f394 100644
> --- a/include/linux/nmi.h
> +++ b/include/linux/nmi.h
> @@ -125,18 +125,35 @@ void watchdog_nmi_disable(unsigned int cpu);
> void lockup_detector_reconfigure(void);
>
> /**
> - * touch_nmi_watchdog - restart NMI watchdog timeout.
> + * touch_hardlockup_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_hardlockup_watchdog() may be
> + * used to pet the watchdog (reset the timeout) - for code which
> + * intentionally disables interrupts for a long time. This call is stateless.
> */
> -static inline void touch_nmi_watchdog(void)
> +static inline void touch_hardlockup_watchdog(void)
> {
> + /*
> + * Pass on to the hardlockup detector selected via CONFIG_. Note that
> + * the hardlockup detector may not be arch-specific nor using NMIs,
> + * but arch_touch_nmi_watchdog() is exported with EXPORT_SYMBOL() and
> + * is thus ABI.
> + */
> arch_touch_nmi_watchdog();
> +
> + /*
> + * Touching the hardlock detector implcitily pets the
> + * softlockup detector too
> + */
> touch_softlockup_watchdog();
> }
>
> +/*
> + * It's encouraged for code to refer to the new name, but allow the old
> + * name as well.
> + */
> +#define touch_nmi_watchdog touch_hardlockup_watchdog
> +
> /*
> * Create trigger_all_cpu_backtrace() out of the arch-provided
> * base function. Return whether such support was available,
> --
> 2.40.1.521.gf1e218fcd8-goog