Messages in this thread | | | Subject | Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable | From | Babu Moger <> | Date | Mon, 24 Oct 2016 19:55:50 -0500 |
| |
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. >>>
| |