Re: [PATCH v2 2/6] watchdog/hardlockup: Make the config checks more straightforward

2023-06-16 Thread Doug Anderson
Hi,

On Fri, Jun 16, 2023 at 8:07 AM Petr Mladek  wrote:
>
> There are four possible variants of hardlockup detectors:
>
>   + buddy: available when SMP is set.
>
>   + perf: available when HAVE_HARDLOCKUP_DETECTOR_PERF is set.
>
>   + arch-specific: available when HAVE_HARDLOCKUP_DETECTOR_ARCH is set.
>
>   + sparc64 special variant: available when HAVE_NMI_WATCHDOG is set
> and HAVE_HARDLOCKUP_DETECTOR_ARCH is not set.
>
> The check for the sparc64 variant is more complicated because
> HAVE_NMI_WATCHDOG is used to #ifdef code used by both arch-specific
> and sparc64 specific variant. Therefore it is automatically
> selected with HAVE_HARDLOCKUP_DETECTOR_ARCH.
>
> This complexity is partly hidden in HAVE_HARDLOCKUP_DETECTOR_NON_ARCH.
> It reduces the size of some checks but it makes them harder to follow.
>
> Finally, the other temporary variable HARDLOCKUP_DETECTOR_NON_ARCH
> is used to re-compute HARDLOCKUP_DETECTOR_PERF/BUDDY when the global
> HARDLOCKUP_DETECTOR switch is enabled/disabled.
>
> Make the logic more straightforward by the following changes:
>
>   + Better explain the role of HAVE_HARDLOCKUP_DETECTOR_ARCH and
> HAVE_NMI_WATCHDOG in comments.
>
>   + Add HAVE_HARDLOCKUP_DETECTOR_BUDDY so that there is separate
> HAVE_* for all four hardlockup detector variants.
>
> Use it in the other conditions instead of SMP. It makes it
> clear that it is about the buddy detector.
>
>   + Open code HAVE_HARDLOCKUP_DETECTOR_NON_ARCH in HARDLOCKUP_DETECTOR
> and HARDLOCKUP_DETECTOR_PREFER_BUDDY. It helps to understand
> the conditions between the four hardlockup detector variants.
>
>   + Define the exact conditions when HARDLOCKUP_DETECTOR_PERF/BUDDY
> can be enabled. It explains the dependency on the other
> hardlockup detector variants.
>
> Also it allows to remove HARDLOCKUP_DETECTOR_NON_ARCH by using "imply".
> It triggers re-evaluating HARDLOCKUP_DETECTOR_PERF/BUDDY when
> the global HARDLOCKUP_DETECTOR switch is changed.
>
>   + Add dependency on HARDLOCKUP_DETECTOR so that the affected variables
> disappear when the hardlockup detectors are disabled.
>
> Another nice side effect is that HARDLOCKUP_DETECTOR_PREFER_BUDDY
> value is not preserved when the global switch is disabled.
> The user has to make the decision again when it gets re-enabled.
>
> Signed-off-by: Petr Mladek 
> ---
>  arch/Kconfig  | 23 +-
>  lib/Kconfig.debug | 62 +++
>  2 files changed, 53 insertions(+), 32 deletions(-)

While I'd still paint the bikeshed a different color and organize the
dependencies a little differently, as discussed in your v1 post, this
is still OK w/ me.

Reviewed-by: Douglas Anderson 


[PATCH v2 2/6] watchdog/hardlockup: Make the config checks more straightforward

2023-06-16 Thread Petr Mladek
There are four possible variants of hardlockup detectors:

  + buddy: available when SMP is set.

  + perf: available when HAVE_HARDLOCKUP_DETECTOR_PERF is set.

  + arch-specific: available when HAVE_HARDLOCKUP_DETECTOR_ARCH is set.

  + sparc64 special variant: available when HAVE_NMI_WATCHDOG is set
and HAVE_HARDLOCKUP_DETECTOR_ARCH is not set.

The check for the sparc64 variant is more complicated because
HAVE_NMI_WATCHDOG is used to #ifdef code used by both arch-specific
and sparc64 specific variant. Therefore it is automatically
selected with HAVE_HARDLOCKUP_DETECTOR_ARCH.

This complexity is partly hidden in HAVE_HARDLOCKUP_DETECTOR_NON_ARCH.
It reduces the size of some checks but it makes them harder to follow.

Finally, the other temporary variable HARDLOCKUP_DETECTOR_NON_ARCH
is used to re-compute HARDLOCKUP_DETECTOR_PERF/BUDDY when the global
HARDLOCKUP_DETECTOR switch is enabled/disabled.

Make the logic more straightforward by the following changes:

  + Better explain the role of HAVE_HARDLOCKUP_DETECTOR_ARCH and
HAVE_NMI_WATCHDOG in comments.

  + Add HAVE_HARDLOCKUP_DETECTOR_BUDDY so that there is separate
HAVE_* for all four hardlockup detector variants.

Use it in the other conditions instead of SMP. It makes it
clear that it is about the buddy detector.

  + Open code HAVE_HARDLOCKUP_DETECTOR_NON_ARCH in HARDLOCKUP_DETECTOR
and HARDLOCKUP_DETECTOR_PREFER_BUDDY. It helps to understand
the conditions between the four hardlockup detector variants.

  + Define the exact conditions when HARDLOCKUP_DETECTOR_PERF/BUDDY
can be enabled. It explains the dependency on the other
hardlockup detector variants.

Also it allows to remove HARDLOCKUP_DETECTOR_NON_ARCH by using "imply".
It triggers re-evaluating HARDLOCKUP_DETECTOR_PERF/BUDDY when
the global HARDLOCKUP_DETECTOR switch is changed.

  + Add dependency on HARDLOCKUP_DETECTOR so that the affected variables
disappear when the hardlockup detectors are disabled.

Another nice side effect is that HARDLOCKUP_DETECTOR_PREFER_BUDDY
value is not preserved when the global switch is disabled.
The user has to make the decision again when it gets re-enabled.

Signed-off-by: Petr Mladek 
---
 arch/Kconfig  | 23 +-
 lib/Kconfig.debug | 62 +++
 2 files changed, 53 insertions(+), 32 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 422f0ffa269e..77e5af5fda3f 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -404,17 +404,28 @@ config HAVE_NMI_WATCHDOG
depends on HAVE_NMI
bool
help
- The arch provides a low level NMI watchdog. It provides
- asm/nmi.h, and defines its own watchdog_hardlockup_probe() and
- arch_touch_nmi_watchdog().
+ The arch provides its own hardlockup detector implementation instead
+ of the generic ones.
+
+ Sparc64 defines this variable without HAVE_HARDLOCKUP_DETECTOR_ARCH.
+ It is the last arch-specific implementation which was developed before
+ adding the common infrastructure for handling hardlockup detectors.
+ It is always built. It does _not_ use the common command line
+ parameters and sysctl interface, except for
+ /proc/sys/kernel/nmi_watchdog.
 
 config HAVE_HARDLOCKUP_DETECTOR_ARCH
bool
select HAVE_NMI_WATCHDOG
help
- The arch chooses to provide its own hardlockup detector, which is
- a superset of the HAVE_NMI_WATCHDOG. It also conforms to config
- interfaces and parameters provided by hardlockup detector subsystem.
+ The arch provides its own hardlockup detector implementation instead
+ of the generic ones.
+
+ 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
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 3e91fa33c7a0..a0b0c4decb89 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1035,16 +1035,33 @@ config BOOTPARAM_SOFTLOCKUP_PANIC
 
  Say N if unsure.
 
+config HAVE_HARDLOCKUP_DETECTOR_BUDDY
+   bool
+   depends on SMP
+   default y
+
 #
-# arch/ can define HAVE_HARDLOCKUP_DETECTOR_ARCH to provide their own hard
-# lockup detector rather than the perf based detector.
+# Global switch whether to build a hardlockup detector at all. It is available
+# only when the architecture supports at least one implementation. There are
+# two exceptions. The hardlockup detector is never enabled on:
+#
+#  s390: it reported many false positives there
+#
+#  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