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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZIGrFiGhUiO6OOsa@alley>
Date:   Thu, 8 Jun 2023 12:19:02 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        kgdb-bugreport@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
        Nicholas Piggin <npiggin@...il.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        linuxppc-dev@...ts.ozlabs.org,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        sparclinux@...r.kernel.org,
        "David S . Miller" <davem@...emloft.net>,
        linux-perf-users@...r.kernel.org
Subject: Re: [PATCH 1/7] watchdog/hardlockup: Sort hardlockup detector
 related config values a logical way

On Wed 2023-06-07 16:34:20, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jun 7, 2023 at 8:25 AM Petr Mladek <pmladek@...e.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