Re: [PATCH v4 11/17] watchdog/hardlockup: Rename some "NMI watchdog" constants/function

From: Petr Mladek <pmladek@suse.com>
To: Nicholas Piggin <npiggin@gmail.com>
Cc: Douglas Anderson <dianders@chromium.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Sumit Garg <sumit.garg@linaro.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Matthias Kaehlcke <mka@chromium.org>,
	Stephane Eranian <eranian@google.com>,
	Stephen Boyd <swboyd@chromium.org>,
	ricardo.neri@intel.com, Tzung-Bi Shih <tzungbi@chromium.org>,
	Lecopzer Chen <lecopzer.chen@mediatek.com>,
	kgdb-bugreport@lists.sourceforge.net,
	Masayoshi Mizuma <msys.mizuma@gmail.com>,
	Guenter Roeck <groeck@chromium.org>,
	Pingfan Liu <kernelfans@gmail.com>,
	Andi Kleen <ak@linux.intel.com>, Ian Rogers <irogers@google.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-perf-users@vger.kernel.org, ito-yuichi@fujitsu.com,
	Randy Dunlap <rdunlap@infradead.org>,
	Chen-Yu Tsai <wens@csie.org>,
	christophe.leroy@csgroup.eu, davem@davemloft.net,
	sparclinux@vger.kernel.org, mpe@ellerman.id.au,
	Will Deacon <will@kernel.org>,
	ravi.v.shankar@intel.com, linuxppc-dev@lists.ozlabs.org,
	Marc Zyngier <maz@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Daniel Thompson <daniel.thompson@linaro.org>
Subject: Re: [PATCH v4 11/17] watchdog/hardlockup: Rename some "NMI watchdog" constants/function
Date: Fri, 12 May 2023 13:21:34 +0200	[thread overview]
Message-ID: <ZF4hPiEjvr4_ditV@alley> (raw)
In-Reply-To: <CSE0GBQQDUAY.1QAJIC3D3OBVU@wheely>

On Fri 2023-05-05 13:06:41, Nicholas Piggin wrote:
> On Fri May 5, 2023 at 8:13 AM AEST, Douglas Anderson wrote:
> > Do a search and replace of:
> > - NMI_WATCHDOG_ENABLED => HARD_WATCHDOG_ENABLED
> > - watchdog_nmi_ => watchdog_hardlockup_
> 
> These are just making prefixes inconsistent again.

Yeah, HARD_WATCHDOG_ENABLED does not fit in. I would personally
rename:

  - NMI_WATCHDOG_ENABLED => WATCHDOG_HARDLOCKUP_ENABLED
  - SOFT_WATCHDOG_ENABLED => WATCHDOG_SOFTOCKUP_ENABLED

to follow the new name space.

> If you really want to do a prefix, I would call it hardlockup which

I wish, we found a good short prefix. My problem with hardlockup_
is that for example "hardlockup_enable() looks ugly.

Also some stuff is common for both softlockup and hardlockup
detectors. And some stuff will be common for both perf and
buddy hardlockup detectors.

Possible alternatives:

   a) watchdog_, watchdog_sl_ and watchdog_hl_, watchdog_hl_buddy_
   b) wd_, wd_hardlockup_, wd_softlockup_, wd_hardlockup_buddy_
   c) wd_, wd_hl_, wd_sl_, wd_hl_buddy_
   d_ wd_, wdhl_, wdsl_, wdhl_buddy_

If you want something shorter then c) looks the best to me.

The wd_ prefix seems to be already used in:

   + arch/powerpc/kernel/watchdog.c
   + kernel/time/clocksource.c

, but it is not used in the core watchdog code at all so it
would require renaming almost everything.

> probably best matches existing code and sysctl / boot stuff, and
> concentrate on non-static symbols.

Yeah, we could hardly change the sysctl interface visible to
userspace. But we could change at least the internal code.

And if we are changing the API anyway because of the
nmi/perf/buddy/hardlockup/hard mess then lets choose
something that will help to distinguish the common watchdog
vs. softlockup/hardlockup/buddy/perf-specific watchdog code.

And I would change it to the watchdog_hardlockup_ as it is
done in this patchset:

   + the names were mostly long even before
   + the code mostly stayed within the 80-chars per-line limit
   + the patches are ready

> No problem with minor things like this that touch arch/powerpc
> going through Andrew's tree though. I'm sure sparc maintainers
> wouldn't mind either.

Good to know.

Best Regards,
Petr