| Message ID | 20230607152432.5435-5-pmladek@suse.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | watchdog/hardlockup: Cleanup configuration of hardlockup detectors | expand |
Hi, On Wed, Jun 7, 2023 at 8:25 AM Petr Mladek <pmladek@suse.com> wrote: > > diff --git a/arch/Kconfig b/arch/Kconfig > index 13c6e596cf9e..57f15babe188 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -404,10 +404,9 @@ config HAVE_NMI_WATCHDOG > depends on HAVE_NMI > bool > help > - The arch provides its own hardlockup detector implementation instead > + Sparc64 provides its own hardlockup detector implementation instead > of the generic perf one. It's a little weird to document generic things with the specifics of the user. The exception, IMO, is when something is deprecated. Personally, it would sound less weird to me to say something like: The arch provides its own hardlockup detector implementation instead of the generic perf one. This is a deprecated thing to do and kept around until sparc64 provides a full hardlockup implementation or moves to generic code. > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index d201f5d3876b..4b4aa0f941f9 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -1050,15 +1050,13 @@ config HAVE_HARDLOCKUP_DETECTOR_BUDDY > # sparc64: has a custom implementation which is not using the common > # hardlockup command line options and sysctl interface. > # > -# Note that HAVE_NMI_WATCHDOG is used to distinguish the sparc64 specific > -# implementaion. It is automatically enabled also for other arch-specific > -# variants which set HAVE_HARDLOCKUP_DETECTOR_ARCH. It makes the check > -# of avaialable and supported variants quite tricky. > +# Note that HAVE_NMI_WATCHDOG is set when the sparc64 specific implementation > +# is used. > # > config HARDLOCKUP_DETECTOR > bool "Detect Hard Lockups" > - depends on DEBUG_KERNEL && !S390 > - depends on ((HAVE_HARDLOCKUP_DETECTOR_PERF || HAVE_HARDLOCKUP_DETECTOR_BUDDY) && !HAVE_NMI_WATCHDOG) || HAVE_HARDLOCKUP_DETECTOR_ARCH > + depends on DEBUG_KERNEL && !S390 && !HAVE_NMI_WATCHDOG > + depends on HAVE_HARDLOCKUP_DETECTOR_PERF || HAVE_HARDLOCKUP_DETECTOR_BUDDY || HAVE_HARDLOCKUP_DETECTOR_ARCH If you add the "!HAVE_NMI_WATCHDOG" as a dependency to HAVE_HARDLOCKUP_DETECTOR_BUDDY, as discussed in a previous patch, you can skip adding it here. > imply HARDLOCKUP_DETECTOR_PERF > imply HARDLOCKUP_DETECTOR_BUDDY > select LOCKUP_DETECTOR > @@ -1079,7 +1077,7 @@ config HARDLOCKUP_DETECTOR_PREFER_BUDDY > bool "Prefer the buddy CPU hardlockup detector" > depends on HARDLOCKUP_DETECTOR > depends on HAVE_HARDLOCKUP_DETECTOR_PERF && HAVE_HARDLOCKUP_DETECTOR_BUDDY > - depends on !HAVE_NMI_WATCHDOG > + depends on !HAVE_HARLOCKUP_DETECTOR_ARCH Don't need this. Architectures never are allowed to define HAVE_HARDLOCKUP_DETECTOR_PERF and HAVE_HARLOCKUP_DETECTOR_ARCH > default n > help > Say Y here to prefer the buddy hardlockup detector over the perf one. > @@ -1096,7 +1094,7 @@ config HARDLOCKUP_DETECTOR_PERF > bool > depends on HARDLOCKUP_DETECTOR > depends on HAVE_HARDLOCKUP_DETECTOR_PERF && !HARDLOCKUP_DETECTOR_PREFER_BUDDY > - depends on !HAVE_NMI_WATCHDOG > + depends on !HAVE_HARDLOCKUP_DETECTOR_ARCH Similarly, don't need this. > select HARDLOCKUP_DETECTOR_COUNTS_HRTIMER > > config HARDLOCKUP_DETECTOR_BUDDY > @@ -1104,7 +1102,7 @@ config HARDLOCKUP_DETECTOR_BUDDY > depends on HARDLOCKUP_DETECTOR > depends on HAVE_HARDLOCKUP_DETECTOR_BUDDY > depends on !HAVE_HARDLOCKUP_DETECTOR_PERF || HARDLOCKUP_DETECTOR_PREFER_BUDDY > - depends on !HAVE_NMI_WATCHDOG > + depends on !HAVE_HARDLOCKUP_DETECTOR_ARCH Similarly, don't need this. In general I don't object to splitting out HAVE_NMI_WATCHDOG from HAVE_HARDLOCKUP_DETECTOR_ARCH.
On Wed 2023-06-07 16:36:35, Doug Anderson wrote: > Hi, > > On Wed, Jun 7, 2023 at 8:25 AM Petr Mladek <pmladek@suse.com> wrote: > > > > diff --git a/arch/Kconfig b/arch/Kconfig > > index 13c6e596cf9e..57f15babe188 100644 > > --- a/arch/Kconfig > > +++ b/arch/Kconfig > > @@ -404,10 +404,9 @@ config HAVE_NMI_WATCHDOG > > depends on HAVE_NMI > > bool > > help > > - The arch provides its own hardlockup detector implementation instead > > + Sparc64 provides its own hardlockup detector implementation instead > > of the generic perf one. > > It's a little weird to document generic things with the specifics of > the user. The exception, IMO, is when something is deprecated. > Personally, it would sound less weird to me to say something like: Or I could replace "The arch" by "Sparc64" in the 5th patch which renames the variable to HAVE_HARDLOCKUP_DETECTOR_SPARC64. It will not longer be a generic thing... Or I could squash the two patches. I did not want to do too many changes at the same time. But it might actually make sense to do this in one step. > > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > > index d201f5d3876b..4b4aa0f941f9 100644 > > --- a/lib/Kconfig.debug > > +++ b/lib/Kconfig.debug > > @@ -1050,15 +1050,13 @@ config HAVE_HARDLOCKUP_DETECTOR_BUDDY > > # sparc64: has a custom implementation which is not using the common > > # hardlockup command line options and sysctl interface. > > # > > -# Note that HAVE_NMI_WATCHDOG is used to distinguish the sparc64 specific > > -# implementaion. It is automatically enabled also for other arch-specific > > -# variants which set HAVE_HARDLOCKUP_DETECTOR_ARCH. It makes the check > > -# of avaialable and supported variants quite tricky. > > +# Note that HAVE_NMI_WATCHDOG is set when the sparc64 specific implementation > > +# is used. > > # > > config HARDLOCKUP_DETECTOR > > bool "Detect Hard Lockups" > > - depends on DEBUG_KERNEL && !S390 > > - depends on ((HAVE_HARDLOCKUP_DETECTOR_PERF || HAVE_HARDLOCKUP_DETECTOR_BUDDY) && !HAVE_NMI_WATCHDOG) || HAVE_HARDLOCKUP_DETECTOR_ARCH > > + depends on DEBUG_KERNEL && !S390 && !HAVE_NMI_WATCHDOG > > + depends on HAVE_HARDLOCKUP_DETECTOR_PERF || HAVE_HARDLOCKUP_DETECTOR_BUDDY || HAVE_HARDLOCKUP_DETECTOR_ARCH > > If you add the "!HAVE_NMI_WATCHDOG" as a dependency to > HAVE_HARDLOCKUP_DETECTOR_BUDDY, as discussed in a previous patch, you > can skip adding it here. It it related to the 2nd patch. Let's discuss it there. > > > imply HARDLOCKUP_DETECTOR_PERF > > imply HARDLOCKUP_DETECTOR_BUDDY > > select LOCKUP_DETECTOR > > @@ -1079,7 +1077,7 @@ config HARDLOCKUP_DETECTOR_PREFER_BUDDY > > bool "Prefer the buddy CPU hardlockup detector" > > depends on HARDLOCKUP_DETECTOR > > depends on HAVE_HARDLOCKUP_DETECTOR_PERF && HAVE_HARDLOCKUP_DETECTOR_BUDDY > > - depends on !HAVE_NMI_WATCHDOG > > + depends on !HAVE_HARLOCKUP_DETECTOR_ARCH > > Don't need this. Architectures never are allowed to define > HAVE_HARDLOCKUP_DETECTOR_PERF and HAVE_HARLOCKUP_DETECTOR_ARCH Same here... Best Regards, Petr
diff --git a/arch/Kconfig b/arch/Kconfig index 13c6e596cf9e..57f15babe188 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -404,10 +404,9 @@ config HAVE_NMI_WATCHDOG depends on HAVE_NMI bool help - The arch provides its own hardlockup detector implementation instead + Sparc64 provides its own hardlockup detector implementation instead of the generic perf one. - Sparc64 defines this variable without HAVE_HARDLOCKUP_DETECTOR_ARCH. It does _not_ use the command line parameters and sysctl interface used by generic hardlockup detectors. Instead it is enabled/disabled by the top-level watchdog interface that is common for both softlockup @@ -415,7 +414,6 @@ config HAVE_NMI_WATCHDOG config HAVE_HARDLOCKUP_DETECTOR_ARCH bool - select HAVE_NMI_WATCHDOG help The arch provides its own hardlockup detector implementation instead of the generic ones. @@ -423,9 +421,6 @@ config HAVE_HARDLOCKUP_DETECTOR_ARCH It uses the same command line parameters, and sysctl interface, as the generic hardlockup detectors. - HAVE_NMI_WATCHDOG is selected to build the code shared with - the sparc64 specific implementation. - config HAVE_PERF_REGS bool help diff --git a/include/linux/nmi.h b/include/linux/nmi.h index b9e816bde14a..800196c78f65 100644 --- a/include/linux/nmi.h +++ b/include/linux/nmi.h @@ -9,7 +9,7 @@ #include <asm/irq.h> /* Arch specific watchdogs might need to share extra watchdog-related APIs. */ -#if defined(CONFIG_HAVE_NMI_WATCHDOG) +#if defined(CONFIG_HAVE_HARDLOCKUP_DETECTOR_ARCH) || defined(CONFIG_HAVE_NMI_WATCHDOG) #include <asm/nmi.h> #endif @@ -92,8 +92,7 @@ static inline void hardlockup_detector_disable(void) {} #endif /* Sparc64 has special implemetantion that is always enabled. */ -#if defined(CONFIG_HARDLOCKUP_DETECTOR) || \ - (defined(CONFIG_HAVE_NMI_WATCHDOG) && !defined(CONFIG_HAVE_HARDLOCKUP_DETECTOR_ARCH)) +#if defined(CONFIG_HARDLOCKUP_DETECTOR) || defined(CONFIG_HAVE_NMI_WATCHDOG) void arch_touch_nmi_watchdog(void); #else static inline void arch_touch_nmi_watchdog(void) { } diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index d201f5d3876b..4b4aa0f941f9 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1050,15 +1050,13 @@ config HAVE_HARDLOCKUP_DETECTOR_BUDDY # sparc64: has a custom implementation which is not using the common # hardlockup command line options and sysctl interface. # -# Note that HAVE_NMI_WATCHDOG is used to distinguish the sparc64 specific -# implementaion. It is automatically enabled also for other arch-specific -# variants which set HAVE_HARDLOCKUP_DETECTOR_ARCH. It makes the check -# of avaialable and supported variants quite tricky. +# Note that HAVE_NMI_WATCHDOG is set when the sparc64 specific implementation +# is used. # config HARDLOCKUP_DETECTOR bool "Detect Hard Lockups" - depends on DEBUG_KERNEL && !S390 - depends on ((HAVE_HARDLOCKUP_DETECTOR_PERF || HAVE_HARDLOCKUP_DETECTOR_BUDDY) && !HAVE_NMI_WATCHDOG) || HAVE_HARDLOCKUP_DETECTOR_ARCH + depends on DEBUG_KERNEL && !S390 && !HAVE_NMI_WATCHDOG + depends on HAVE_HARDLOCKUP_DETECTOR_PERF || HAVE_HARDLOCKUP_DETECTOR_BUDDY || HAVE_HARDLOCKUP_DETECTOR_ARCH imply HARDLOCKUP_DETECTOR_PERF imply HARDLOCKUP_DETECTOR_BUDDY select LOCKUP_DETECTOR @@ -1079,7 +1077,7 @@ config HARDLOCKUP_DETECTOR_PREFER_BUDDY bool "Prefer the buddy CPU hardlockup detector" depends on HARDLOCKUP_DETECTOR depends on HAVE_HARDLOCKUP_DETECTOR_PERF && HAVE_HARDLOCKUP_DETECTOR_BUDDY - depends on !HAVE_NMI_WATCHDOG + depends on !HAVE_HARLOCKUP_DETECTOR_ARCH default n help Say Y here to prefer the buddy hardlockup detector over the perf one. @@ -1096,7 +1094,7 @@ config HARDLOCKUP_DETECTOR_PERF bool depends on HARDLOCKUP_DETECTOR depends on HAVE_HARDLOCKUP_DETECTOR_PERF && !HARDLOCKUP_DETECTOR_PREFER_BUDDY - depends on !HAVE_NMI_WATCHDOG + depends on !HAVE_HARDLOCKUP_DETECTOR_ARCH select HARDLOCKUP_DETECTOR_COUNTS_HRTIMER config HARDLOCKUP_DETECTOR_BUDDY @@ -1104,7 +1102,7 @@ config HARDLOCKUP_DETECTOR_BUDDY depends on HARDLOCKUP_DETECTOR depends on HAVE_HARDLOCKUP_DETECTOR_BUDDY depends on !HAVE_HARDLOCKUP_DETECTOR_PERF || HARDLOCKUP_DETECTOR_PREFER_BUDDY - depends on !HAVE_NMI_WATCHDOG + depends on !HAVE_HARDLOCKUP_DETECTOR_ARCH select HARDLOCKUP_DETECTOR_COUNTS_HRTIMER #
HAVE_NMI_WATCHDOG is always enabled when SPARC64 is enabled. The sparc64 implementation is special. It does not support the generic hardlockup related Kconfig values, command line parameters, and sysctl interface. Instead it is enabled/disabled by the top-level watchdog interface that is common for both softlockup and hardlockup detectors. As a result, sparc64 needs special treating in Kconfig and source files. The checks are a bit complicated because HAVE_NMI_WATCHDOG is automatically set when HAVE_HARDLOCKUP_DETECTOR_ARCH is set. But HAVE_HARDLOCKUP_DETECTOR_ARCH is set when the arch specific implementation uses the generic hardlockup detector related Kconfig variables, command line parameters, and sysctl interface. The motivation probably was to avoid changes in the code when the powerpc64-specific watchdog introduced HAVE_HARDLOCKUP_DETECTOR_ARCH. It probably allowed to re-use some existing checks for HAVE_NMI_WATCHDOG. But it actually made things pretty complicated. For example, the following check was needed in HARDLOCKUP_DETECTOR config variable: depends on ((HAVE_HARDLOCKUP_DETECTOR_PERF || HAVE_HARDLOCKUP_DETECTOR_BUDDY) && !HAVE_NMI_WATCHDOG) || HAVE_HARDLOCKUP_DETECTOR_ARCH The inverted logic makes things easier: + HAVE_NMI_WATCHDOG is used only on sparc64. It is clear when the sparc64 specific watchdog is used. + HAVE_HARDLOCKUP_DETECTOR_ARCH is basically compatible with the generic watchdogs. As a result, the common code is marked by ifdef CONFIG_HARDLOCKUP_DETECTOR. As a result: + Some conditions are easier. + Some conditions use HAVE_HARDLOCKUP_DETECTOR_ARCH instead of HAVE_NMI_WATCHDOG. Note that HARDLOCKUP_DETECTOR_PREFER_BUDDY, HARDLOCKUP_DETECTOR_PERF, and HARDLOCKUP_DETECTOR_BUDDY might depend only on HAVE_HARDLOCKUP_DETECTOR_ARCH. They depend on HARDLOCKUP_DETECTOR and it is not longer enabled when HAVE_NMI_WATCHDOG is set. Note that asm/nmi.h still has to be included for all arch-specific watchdogs. It declares more functions that are used in another arch specific code which includes only linux/nmi.h. Signed-off-by: Petr Mladek <pmladek@suse.com> --- arch/Kconfig | 7 +------ include/linux/nmi.h | 5 ++--- lib/Kconfig.debug | 16 +++++++--------- 3 files changed, 10 insertions(+), 18 deletions(-)