On Thu, 8 Jun 2017 12:05:02 -0400 Don Zickus <dzickus@xxxxxxxxxx> wrote: > On Wed, Jun 07, 2017 at 01:50:26PM +1000, Nicholas Piggin wrote: > > > > > > I _think_ having > > > > > > depends on LOCKUP_DETECTOR > > > depends on HAVE_NMI_WATCHDOG || HAVE_PERF_EVENTS_NMI > > > select HARDLOCKUP_DETECTOR_PERF if !HAVE_NMI_WATCHDOG > > > > > > will work because your new definition of HARDLOCKUP_DETECTOR is a > > > combination of HAVE_NMI_WATCHDOG && HARDLOCKUP_DETECTOR_PERF ?? > > > > > > Did I get that right? > > > > Well in some ways, except that most of the NMI watchdogs do not seem to > > heed the HARDLOCKUP_DETECTOR configuration sysctls and commands. > > > > NMI_WATCHDOG by itself was supposed to be for an arch that wnats to do > > completely it own thing. > > > > sparc is somewhere in the middle. It uses some of the HLD stuff, but > > not all. That makes it a bit tricky. > > Hmm, I can see sparc relying on SOFTLOCKUP, but the HARDLOCKUP code with > your patches seems really small. The only sysctl is nmi_watchdog and > hardlockup_panic. The former is needed but the later is only implemented in > the HARDLOCKUP_DETECTOR_PERF case. > > So by having HAVE_NMI_WATCHDOG, you sorta need a sysctl knob which the > HARDLOCKUP_DETECTOR seems to provide on top of the default knobs. I would say there's a few others like the cpumask, threshold, panic, backtrace, etc. > With sparc being special and needing SOFTLOCKUP to call its nmi > enable/disable hooks. Yes, although we could just remove that dependency (it's minimal code to start them up with hotplug). That said, I would hope to actually go the other way in general and move architectures to using the generic parameters as much as possible. > Is there a particular chunk of code you had in mind that did not make sense > with HARDLOCKUP_DETECTOR enabled? Perhaps the couple of other archs that have their own NMI watchdogs. > > > > > > > > I almost wonder if arches should set either HAVE_NMI_WATCHDOG or > > > HAVE_PERF_NMI_WATCHDOG and then use those two to determine > > > HARDLOCKUP_DETECTOR. Would that make the config options slightly less > > > confusing? > > > > This would probably be the right direction to go in, but it will take > > slightly more I think. We first need to remove HAVE_NMI_WATCHDOG from > > meaning that an arch has its own watchdog and does not want any HLD > > stuff. I think with arch_touch_nmi_watchdog(), we can probably get there. > > > > While transitioning, we could add a new option instead, > > > > HAVE_ARCH_HARDLOCKUP_DETECTOR > > > > I think HAVE_PERF_EVENTS_NMI is sufficient to imply it will use the PERF > > HLD. Possibly you could just change the name to be a bit more regular, > > HAVE_PERF_NMI_HARDLOCKUP_DETECTOR > > Actually, I don't think I can just rename it as it has a specific use to let > OPROFILE know the perf events are being NMI triggered as opposed to IRQ > triggered. > > Though I like the direction you are going. Then arches either have one or > the other. Or in the ppc case it is dependent on what ppc platform is being > used. Okay, glad we're on the same page conceptually :) > > Then the HARDLOCKUP_DETECTOR needs one or the other to work correctly with > the arch/<arch>/Kconfig explicitly stating which one to use? Yeah I guess the arch would advertise it has the PERF_HLD or ARCH_HLD if it provides its own. HARDLOCKUP_DETECTOR option would then depend on one of the two being defined. I could try redoing the series with those changes to Kconfig and see how it looks? Thanks, Nick
