Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Babu Moger <babu.moger@oracle.com>]

On 10/24/2016 10:19 AM, Don Zickus wrote:
> 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@redhat.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@redhat.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.

Don. Sure. I have started on this. Will send RFC version sometime this week.

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