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

[Nicholas Piggin <npiggin@gmail.com>]

On Thu, 8 Jun 2017 12:05:02 -0400
Don Zickus <dzickus@redhat.com> 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