lkml.org 
[lkml]   [2016]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable
From
Date

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

\
 
 \ /
  Last update: 2016-10-25 02:56    [W:0.068 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site