Re: [PATCH 3/4] watchdog: Split up config options

[Don Zickus]

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.

With sparc being special and needing SOFTLOCKUP to call its nmi
enable/disable hooks.


Is there a particular chunk of code you had in mind that did not make sense
with HARDLOCKUP_DETECTOR enabled?

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

Then the HARDLOCKUP_DETECTOR needs one or the other to work correctly with
the arch/<arch>/Kconfig explicitly stating which one to use?

Cheers,
Don



> 
> Thanks,
> Nick