| Message ID | 20230607152432.5435-2-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: > > 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. > > Only one hardlockup detector can be compiled in. The selection is done > using quite complex dependencies between several CONFIG variables. > The following patches will try to make it more straightforward. > > As a first step, reorder the definitions of the various CONFIG variables. > The logical order is: > > 1. HAVE_* variables define available variants. They are typically > defined in the arch/ config files. > > 2. HARDLOCKUP_DETECTOR y/n variable defines whether the hardlockup > detector is enabled at all. > > 3. HARDLOCKUP_DETECTOR_PREFER_BUDDY y/n variable defines whether > the buddy detector should be preferred over the perf one. > Note that the arch specific variants are always preferred when > available. > > 4. HARDLOCKUP_DETECTOR_PERF/BUDDY variables define whether the given > detector is enabled in the end. > > 5. HAVE_HARDLOCKUP_DETECTOR_NON_ARCH and HARDLOCKUP_DETECTOR_NON_ARCH > are temporary variables that are going to be removed in > a followup patch. > > The patch just shuffles the definitions. It should not change the existing > behavior. > > Signed-off-by: Petr Mladek <pmladek@suse.com> > --- > lib/Kconfig.debug | 112 +++++++++++++++++++++++----------------------- > 1 file changed, 56 insertions(+), 56 deletions(-) I don't really have any strong opinions, so I'm fine with this. In general I think the ordering I picked tried to match the existing "style" which generally tried to list configs and then select them below. To me the existing style makes more sense when thinking about writing C code without having to put a pile of prototypes at the top of your file: you define things higher in the file and reference them below. For instance, the old style (before any of my patches) had: config SOFTLOCKUP_DETECTOR: ... blah blah blah ... config HARDLOCKUP_DETECTOR_PERF: select SOFTLOCKUP_DETECTOR config HARDLOCKUP_DETECTOR: ... blah blah blah ... select LOCKUP_DETECTOR select HARDLOCKUP_DETECTOR_PERF if HAVE_HARDLOCKUP_DETECTOR_PERF Your new style seems to be the opposite of that. -Doug
On Wed 2023-06-07 16:34:20, Doug Anderson wrote: > Hi, > > On Wed, Jun 7, 2023 at 8:25 AM Petr Mladek <pmladek@suse.com> wrote: > > Only one hardlockup detector can be compiled in. The selection is done > > using quite complex dependencies between several CONFIG variables. > > The following patches will try to make it more straightforward. > > > > As a first step, reorder the definitions of the various CONFIG variables. > > The logical order is: > > > > 1. HAVE_* variables define available variants. They are typically > > defined in the arch/ config files. > > > > 2. HARDLOCKUP_DETECTOR y/n variable defines whether the hardlockup > > detector is enabled at all. > > > > 3. HARDLOCKUP_DETECTOR_PREFER_BUDDY y/n variable defines whether > > the buddy detector should be preferred over the perf one. > > Note that the arch specific variants are always preferred when > > available. > > > > 4. HARDLOCKUP_DETECTOR_PERF/BUDDY variables define whether the given > > detector is enabled in the end. > > > > 5. HAVE_HARDLOCKUP_DETECTOR_NON_ARCH and HARDLOCKUP_DETECTOR_NON_ARCH > > are temporary variables that are going to be removed in > > a followup patch. > > > > I don't really have any strong opinions, so I'm fine with this. In > general I think the ordering I picked tried to match the existing > "style" which generally tried to list configs and then select them > below. To me the existing style makes more sense when thinking about > writing C code I know. My motivation was the following: 1. Kconfig is not C. My view is that it is more like a menu. There is a top level item. If the top level is enabled then there is a submenu with a more detailed selection of various variants and options. 2. The current logic is quite complicated from my POV. And it was even before your patchset. For example, HAVE_HARDLOCKUP_DETECTOR_BUDDY is defined as: config HAVE_HARDLOCKUP_DETECTOR_BUDDY bool depends on SMP default y One would expect that it would be enabled on SMP system. But the final value depends on many other variables which are defined using relatively complex conditions, especially HARDLOCKUP_DETECTOR, HAVE_HARDLOCKUP_DETECTOR_NON_ARCH, and HARDLOCKUP_DETECTOR_NON_ARCH. Understanding the logic is even more complicated because Kconfig is not indexed by cscope. Important: The logic used at the end of the patchset actually follows the C style. It defines how the various variables depend on each other from top to bottom. > > config SOFTLOCKUP_DETECTOR: > ... blah blah blah ... This one is actually defined in the menu-like order: config SOFTLOCKUP_DETECTOR config BOOTPARAM_SOFTLOCKUP_PANIC depends on SOFTLOCKUP_DETECTOR It is because the custom option depends on the top level one. This is exactly what I would like to achieve with HARDLOCKUP variables in this patchset. Best Regards, Petr
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index ed7b01c4bd41..3e91fa33c7a0 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1035,62 +1035,6 @@ config BOOTPARAM_SOFTLOCKUP_PANIC Say N if unsure. -# Both the "perf" and "buddy" hardlockup detectors count hrtimer -# interrupts. This config enables functions managing this common code. -config HARDLOCKUP_DETECTOR_COUNTS_HRTIMER - bool - select SOFTLOCKUP_DETECTOR - -config HARDLOCKUP_DETECTOR_PERF - bool - depends on HAVE_HARDLOCKUP_DETECTOR_PERF - select HARDLOCKUP_DETECTOR_COUNTS_HRTIMER - -config HARDLOCKUP_DETECTOR_BUDDY - bool - depends on SMP - select HARDLOCKUP_DETECTOR_COUNTS_HRTIMER - -# For hardlockup detectors you can have one directly provided by the arch -# or use a "non-arch" one. If you're using a "non-arch" one that is -# further divided the perf hardlockup detector (which, confusingly, needs -# arch-provided perf support) and the buddy hardlockup detector (which just -# needs SMP). In either case, using the "non-arch" code conflicts with -# the NMI watchdog code (which is sometimes used directly and sometimes used -# by the arch-provided hardlockup detector). -config HAVE_HARDLOCKUP_DETECTOR_NON_ARCH - bool - depends on (HAVE_HARDLOCKUP_DETECTOR_PERF || SMP) && !HAVE_NMI_WATCHDOG - default y - -config HARDLOCKUP_DETECTOR_PREFER_BUDDY - bool "Prefer the buddy CPU hardlockup detector" - depends on HAVE_HARDLOCKUP_DETECTOR_PERF && SMP - help - Say Y here to prefer the buddy hardlockup detector over the perf one. - - With the buddy detector, each CPU uses its softlockup hrtimer - to check that the next CPU is processing hrtimer interrupts by - verifying that a counter is increasing. - - This hardlockup detector is useful on systems that don't have - an arch-specific hardlockup detector or if resources needed - for the hardlockup detector are better used for other things. - -# This will select the appropriate non-arch hardlockdup detector -config HARDLOCKUP_DETECTOR_NON_ARCH - bool - depends on HAVE_HARDLOCKUP_DETECTOR_NON_ARCH - select HARDLOCKUP_DETECTOR_BUDDY if !HAVE_HARDLOCKUP_DETECTOR_PERF || HARDLOCKUP_DETECTOR_PREFER_BUDDY - select HARDLOCKUP_DETECTOR_PERF if HAVE_HARDLOCKUP_DETECTOR_PERF && !HARDLOCKUP_DETECTOR_PREFER_BUDDY - -# -# Enables a timestamp based low pass filter to compensate for perf based -# hard lockup detection which runs too fast due to turbo modes. -# -config HARDLOCKUP_CHECK_TIMESTAMP - bool - # # arch/ can define HAVE_HARDLOCKUP_DETECTOR_ARCH to provide their own hard # lockup detector rather than the perf based detector. @@ -1111,6 +1055,62 @@ config HARDLOCKUP_DETECTOR chance to run. The current stack trace is displayed upon detection and the system will stay locked up. +config HARDLOCKUP_DETECTOR_PREFER_BUDDY + bool "Prefer the buddy CPU hardlockup detector" + depends on HAVE_HARDLOCKUP_DETECTOR_PERF && SMP + help + Say Y here to prefer the buddy hardlockup detector over the perf one. + + With the buddy detector, each CPU uses its softlockup hrtimer + to check that the next CPU is processing hrtimer interrupts by + verifying that a counter is increasing. + + This hardlockup detector is useful on systems that don't have + an arch-specific hardlockup detector or if resources needed + for the hardlockup detector are better used for other things. + +config HARDLOCKUP_DETECTOR_PERF + bool + depends on HAVE_HARDLOCKUP_DETECTOR_PERF + select HARDLOCKUP_DETECTOR_COUNTS_HRTIMER + +config HARDLOCKUP_DETECTOR_BUDDY + bool + depends on SMP + select HARDLOCKUP_DETECTOR_COUNTS_HRTIMER + +# Both the "perf" and "buddy" hardlockup detectors count hrtimer +# interrupts. This config enables functions managing this common code. +config HARDLOCKUP_DETECTOR_COUNTS_HRTIMER + bool + select SOFTLOCKUP_DETECTOR + +# For hardlockup detectors you can have one directly provided by the arch +# or use a "non-arch" one. If you're using a "non-arch" one that is +# further divided the perf hardlockup detector (which, confusingly, needs +# arch-provided perf support) and the buddy hardlockup detector (which just +# needs SMP). In either case, using the "non-arch" code conflicts with +# the NMI watchdog code (which is sometimes used directly and sometimes used +# by the arch-provided hardlockup detector). +config HAVE_HARDLOCKUP_DETECTOR_NON_ARCH + bool + depends on (HAVE_HARDLOCKUP_DETECTOR_PERF || SMP) && !HAVE_NMI_WATCHDOG + default y + +# This will select the appropriate non-arch hardlockdup detector +config HARDLOCKUP_DETECTOR_NON_ARCH + bool + depends on HAVE_HARDLOCKUP_DETECTOR_NON_ARCH + select HARDLOCKUP_DETECTOR_BUDDY if !HAVE_HARDLOCKUP_DETECTOR_PERF || HARDLOCKUP_DETECTOR_PREFER_BUDDY + select HARDLOCKUP_DETECTOR_PERF if HAVE_HARDLOCKUP_DETECTOR_PERF && !HARDLOCKUP_DETECTOR_PREFER_BUDDY + +# +# Enables a timestamp based low pass filter to compensate for perf based +# hard lockup detection which runs too fast due to turbo modes. +# +config HARDLOCKUP_CHECK_TIMESTAMP + bool + config BOOTPARAM_HARDLOCKUP_PANIC bool "Panic (Reboot) On Hard Lockups" depends on HARDLOCKUP_DETECTOR
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. Only one hardlockup detector can be compiled in. The selection is done using quite complex dependencies between several CONFIG variables. The following patches will try to make it more straightforward. As a first step, reorder the definitions of the various CONFIG variables. The logical order is: 1. HAVE_* variables define available variants. They are typically defined in the arch/ config files. 2. HARDLOCKUP_DETECTOR y/n variable defines whether the hardlockup detector is enabled at all. 3. HARDLOCKUP_DETECTOR_PREFER_BUDDY y/n variable defines whether the buddy detector should be preferred over the perf one. Note that the arch specific variants are always preferred when available. 4. HARDLOCKUP_DETECTOR_PERF/BUDDY variables define whether the given detector is enabled in the end. 5. HAVE_HARDLOCKUP_DETECTOR_NON_ARCH and HARDLOCKUP_DETECTOR_NON_ARCH are temporary variables that are going to be removed in a followup patch. The patch just shuffles the definitions. It should not change the existing behavior. Signed-off-by: Petr Mladek <pmladek@suse.com> --- lib/Kconfig.debug | 112 +++++++++++++++++++++++----------------------- 1 file changed, 56 insertions(+), 56 deletions(-)