Re: [PATCH v4 07/17] watchdog/hardlockup: Move perf hardlockup checking/panic to common watchdog.c

[Petr Mladek <pmladek@suse.com>]

On Fri 2023-05-05 09:37:50, Doug Anderson wrote:
> Hi,
> 
> On Thu, May 4, 2023 at 7:58 PM Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > On Fri May 5, 2023 at 8:13 AM AEST, Douglas Anderson wrote:
> > > The perf hardlockup detector works by looking at interrupt counts and
> > > seeing if they change from run to run. The interrupt counts are
> > > managed by the common watchdog code via its watchdog_timer_fn().
> > >
> > > Currently the API between the perf detector and the common code is a
> > > function: is_hardlockup(). When the hard lockup detector sees that
> > > function return true then it handles printing out debug info and
> > > inducing a panic if necessary.
> > >
> > > Let's change the API a little bit in preparation for the buddy
> > > hardlockup detector. The buddy hardlockup detector wants to print
> >
> > I think the name change is a gratuitous. Especially since it's now
> > static.
> >
> > watchdog_hardlockup_ is a pretty long prefix too, hardlockup_
> > should be enough?
> >
> > Seems okay otherwise though.
> 
> I went back and forth on names far too much when constructing this
> patch series. Mostly I was trying to balance what looked good to me
> and what Petr suggested [1]. I'm not super picky about the names and
> I'm happy to change them all to a "hardlockup_" prefix. I'd love to
> hear Petr's opinion.

Sigh, the original code was a real mess of naming schemes. It is hard
to say how to move forward.

My opinion:

  + watchdog_hardlockup_check(): looks fine. It is consistent with
	watchdog_hardlockup_enable()/disable().

  + watchdog_hardlockup_is_lockedup() is really overly complicated.
	I would personally keep is_hardlockup(). It is static.
	And it will be consitent with is_softlockup().

  + watchdog_hardlockup_interrupt_count() looks better then
	the original. It clearly shows that it makes sense only
	for the hardlockup detector ("bug" fixed by this patch).

	Well, I would personally call it watchdog_hardlockup_kick()
	and remove the comment.


That said, I could live with this patch. It is better than the
original.

Best Regards,
Petr