Re: [PATCH v4 05/17] watchdog/hardlockup: Rename touch_nmi_watchdog() to touch_hardlockup_watchdog()

[Petr Mladek <pmladek@suse.com>]

On Fri 2023-05-05 09:37:35, Doug Anderson wrote:
> Hi,
> 
> On Thu, May 4, 2023 at 7:51 PM Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > On Fri May 5, 2023 at 8:13 AM AEST, Douglas Anderson wrote:
> > > In preparation for the buddy hardlockup detector, rename
> > > touch_nmi_watchdog() to touch_hardlockup_watchdog() to make it clear
> > > that it will touch whatever hardlockup detector is configured. We'll
> > > add a #define for the old name (touch_nmi_watchdog) so that we don't
> > > have to touch every piece of code referring to the old name.
> >
> > Is this really helpful? Now it's got two names Could just leave it.
> > If you insist then it'd be better just to rename everything in one
> > go at the end of a merge window IMO. Conflicts would be trivial.
> 
> I'm not picky here. I changed the name since Petr requested names to
> be changed for any code I was touching [1] and so I threw this out as
> a proposal. I agree that having two names can be confusing, but in
> this case it didn't feel too terrible to me.

IMHO, it is worth renaming to make the code easier to follow.
Especially after adding the buddy hardlockup detector that is
not using NMI context.

And I agree that that we should rename all callers as well.
Otherwise, it might be seen just as an extra churn.

> I'd love to hear Petr's opinion on this name change. I'm happy with:
> 
> a) This patch as it is.
> 
> b) Dropping this patch (or perhaps just changing it to add comments).
> 
> c) Changing this patch to rename all 70 uses of the old name. Assuming
> this will go through Andrew Morton's tree, I'd be interested in
> whether he's OK w/ this.
> 
> d) Dropping this patch from this series but putting it on the
> backburner to try to do later (so that the rename can happen at a time
> when it's least disruptive).

d) sounds reasonable given that there is about 70 callers.

> 
> > > Ideally this change would also rename the arch_touch_nmi_watchdog(),
> > > but that is harder since arch_touch_nmi_watchdog() is exported with
> > > EXPORT_SYMBOL() and thus is ABI. Add a comment next to the call to
> > > hopefully alleviate some of the confusion here.
> >
> > We don't keep ABI fixed upstream.
> 
> I'm happy to be corrected, but my understanding was that kernel devs
> made an effort not to mess with things exported via "EXPORT_SYMBOL",
> but things exported via "EXPORT_SYMBOL_GPL" were fair game.

My understanding is that kernel guarantees ABI compatibility only for
the userspace (do-not-break-userspace rule). But the kernel ABI
is not guaranteed [*]

It actually has even a positive side effect because it motivates
module developers to upstream the code.

Of course, there should be a good reason for the change. And I think
that we have a good reason.

[*] This is valid for upstream. Another story is with linux
    distributions. They usually maintain the kernel KABI
    stability to some degree when backporting upstream changes.

Best Regards,
Petr