Re: [PATCH 1/7] watchdog/hardlockup: Sort hardlockup detector related config values a logical way

Petr Mladek
2023-06-08T10:19:29Z

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