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]

Date:   Fri, 21 Oct 2016 16:50:21 -0500
From:   Babu Moger <babu.moger@...cle.com>
To:     Andrew Morton <akpm@...ux-foundation.org>,
        Don Zickus <dzickus@...hat.com>
Cc:     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

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.

> 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.
>