linux-kernel - Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi

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: <20161024151950.GS35881@redhat.com>
Date:   Mon, 24 Oct 2016 11:19:50 -0400
From:   Don Zickus <dzickus@...hat.com>
To:     Babu Moger <babu.moger@...cle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, mingo@...nel.org,
        ak@...ux.intel.com, jkosina@...e.cz,
        baiyaowei@...s.chinamobile.com, atomlin@...hat.com,
        uobergfe@...hat.com, tj@...nel.org, hidehiro.kawai.ez@...achi.com,
        johunt@...mai.com, davem@...emloft.net, sparclinux@...r.kernel.org,
        linux-kernel@...r.kernel.org, sam@...nborg.org
Subject: Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and
 arch_watchdog_nmi_disable

On Fri, Oct 21, 2016 at 04:50:21PM -0500, Babu Moger wrote:
> Don,
> 
> On 10/21/2016 2:19 PM, Andrew Morton wrote:
> >On Fri, 21 Oct 2016 11:11:14 -0400 Don Zickus <dzickus@...hat.com> wrote:
> >
> >>On Thu, Oct 20, 2016 at 08:25:27PM -0700, Andrew Morton wrote:
> >>>On Thu, 20 Oct 2016 12:14:14 -0400 Don Zickus <dzickus@...hat.com> wrote:
> >>>
> >>>>>>-static int watchdog_nmi_enable(unsigned int cpu) { return 0; }
> >>>>>>-static void watchdog_nmi_disable(unsigned int cpu) { return; }
> >>>>>>+/*
> >>>>>>+ * These two functions are mostly architecture specific
> >>>>>>+ * defining them as weak here.
> >>>>>>+ */
> >>>>>>+int __weak arch_watchdog_nmi_enable(unsigned int cpu) { return 0; }
> >>>>>>+void __weak arch_watchdog_nmi_disable(unsigned int cpu) { return; }
> >>>>>>+
> >>>>>>  #endif /* CONFIG_HARDLOCKUP_DETECTOR */
> >>>>>This is a strange way of using __weak.
> >>>>>
> >>>>>Take a look at (one of many examples) kernel/module.c:module_alloc().
> >>>>>We simply provide a default implementation and some other compilation
> >>>>>unit can override (actually replace) that at link time.  No strange
> >>>>>ifdeffing needed.
> >>>>Yeah, this is mostly because of how we enable the hardlockup detector.
> >>>>
> >>>>Some arches use the perf hw and enable CONFIG_HARDLOCKUP_DETECTOR.  Other
> >>>>arches just use their own variant of nmi and set CONFIG_HAVE_NMI_WATCHDOG and
> >>>>the rest of the arches do not use this.
> >>>>
> >>>>So the thought was if CONFIG_HARDLOCKUP_DETECTOR use that implementation,
> >>>>everyone else use the __weak version.  Then the arches like sparc can override
> >>>>the weak version with their own nmi enablement.
> >>>>
> >>>>I don't know how to represent those 3 states correctly and the above is what
> >>>>we end up with.
> >>><head spins>
> >>>
> >>>Is there a suitable site where we could capture these considerations in
> >>>a code comment?
> >>Hi Andrew,
> >>
> >>I am not sure I understand your question.  When you say 'site', are you
> >>referring to the kernel/watchdog.c file?
> >Yes, somewhere in there I guess.
> >
> >The problem with this sort of thing is that the implementation is
> >splattered over multiple places in one file or in several files so
> >there's no clear place to document what's happening.  But I think this
> >situation *should* be documented somewhere.  Or maybe that just isn't
> >worthwhile - feel free to disagree!
> >
> >>The other approach that might help de-clutter this file, is to pull out the
> >>HARDLOCKUP_DETECTOR changes (as they are arch specific) and move it to say
> >>kernel/watchdog_hw_ld.c.  Then all the nmi hooks in kernel/watchdog.c can be
> >>__weak and overridden by the kernel_watchdog_hw_ld.c file or the sparc
> >>files.
> >>
> >>This would leave kernel/watchdog.c with just a framework and the
> >>arch-agnostic softlockup detector.  Probably easier to read and digest.
> 
> Don, Yes. I am fine with your idea.  Let me know if you need any help here.
> If you want I can
> start working this cleanup myself. I might take sometime as I need to spend
> sometime
> understanding the whole watchdog stuff first. If you have already started
> working on this
> then I will let you continue.

Hi Babu,

Feel free to start looking at it.  I am trying to wrap up a couple of things
here and will only be able to little poke at it the next couple of days.
But for the most part you might be able to rip out anything with
CONFIG_HARDLOCKUP_DETECTOR and put it into another file.  Then just clean up
the pieces.

Cheers,
Don

> 
> >Well, it depends how the code ends up looking.  It's best to separate
> >functional changes from cleanups.  Generally I think it's best to do
> >"cleanup comes first", because it's then simpler to revert the
> >functional change if it has problems.  Plus people are more
> >*interested* in the functional change so it's best to have that at
> >top-of-tree.
> >
>