make SW_WATCHDOG dynamic
ClosedPublic

Authored by karels on Sun, Dec 31, 2:49 PM.

Details

Summary

Enable the hardclock-based watchdog previously considional on the SW_WATCHDOG
option whenever hardware watchdogs are not found, and watchdogd attempts to
enable the watchdog. The SW_WATCHDOG option still causes the sofware
watchdog to be enabled even if there is a hardware watchdog. This does
not change the other software-based watchdog enabled by the --softtimeout
option to watchdogd.

Note that the code to reprime the watchdog during kernel core dumps is
no longer conditional on SW_WATCHDOG. I think this was previously a bug.

Test Plan

tested with software timeout, looking for hardware testing

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
karels created this revision.Sun, Dec 31, 2:49 PM
imp accepted this revision.Sun, Dec 31, 5:11 PM

LGTM, though I haven't through through all the implications of weird cases (like loading a hardware watchdog after boot)....

This revision is now accepted and ready to land.Sun, Dec 31, 5:11 PM
alfred accepted this revision.Sun, Dec 31, 5:15 PM

This adds the watchdog code to the kernel witn no knob to turn it off or remove it, please make it possible to NOT have this code in the kernel.

Thanks. About loadable hardware watchdogs: that should work if they attach before watchdogd starts.

I'll wait for any comments from manpages, although the changes are pretty simple.

Sorry, my last comment was responding to imp@; it crossed with the comment from rgrimes.

Rod, I really don't see the point of ifdef'ing this code to remove it, especially just the SW_WATCHDOG part. It is probably more likely than not that it will not overflow a page, i.e. the kernel will not grow at all with this addition. Removing all the watchdog code would be hard, as it is part of the driver KPI. And who is really going to set the option to save a few hundred bytes from a kernel that is currently 21 MB (amd64 JGENERIC)?

Two things, first it is not about saving the 100 bytes, it is about not having this function in the kernel at all, I simple do not need it, nor have I needed it for 40 years on any system.
Second, I think we have a POLA problem here, what changes for people that already have SW_WATCHDOG in there kernel config?
Would not this change the behavior of there systems? I think it is a bad idea to take over the existing "option" and changing what it does or how it behaves.
I still think it would just of been simpler to add options SW_WATCHDOG to the GENERIC kernel to solve your desire, that is self documenting more or less as no change is being made to how that behaves, no need to notifiy in RELEASE NOTES (if you push forward with this change as it stands now be certain to mark it for release notes).
Also is not that what you have been doing up to now?
Methods, not policy, your imposing your policy on others.
Also a comparison to the "bloated" generic of 21MB vs a trimmed kernel of 5.5MB is not reasonble to make as a basis for "this is only a tiny bit of code". I agree that it is a tiny bit of code, but it is code most have lived without for decades and should continue to have that option.

Rod, perhaps you have never needed this functionality; but have you ever had a system hang? Along the same lines, there are probably about 1000 bits of code that you haven't needed in the kernel; should we add options to remove each of them? This way lies madness. There are two hardclock routines; which are you using? Should we ifdef out the other?

On the other issue, the SW_WATCHDOG option produces exactly the same results now as it did before, whether or not there is a hardware watchdog. The only difference is when there is no hardware watchdog present, and watchdog is started without --softtimeout, in which case it would previously error out.

bjk added a subscriber: bjk.Mon, Jan 1, 12:49 AM
bjk added inline comments.
share/man/man4/watchdog.4
45 ↗(On Diff #37309)

So we're going to mention that these configuration ioctls exist but not say what they are or what they do?

karels added a comment.Mon, Jan 1, 2:11 AM

IIRC, there are ten of these ioctls, although I didn't add any. I thought I'd correct the obvious out-of-date statement, but wasn't sure I could correctly document all of those added over time. Suggestions?

bjk added a comment.Mon, Jan 1, 5:03 AM

IIRC, there are ten of these ioctls, although I didn't add any. I thought I'd correct the obvious out-of-date statement, but wasn't sure I could correctly document all of those added over time. Suggestions?

Well, I can't insist that you actually add documentation for them all, but it would be a fine compromise to just list them all in a parenthetical after this statement and not document them any further, for now.

karels updated this revision to Diff 37352.Mon, Jan 1, 4:15 PM

Update man page

Add information to man page about configuration ioctls from the header file.

This revision now requires review to proceed.Mon, Jan 1, 4:15 PM
karels added a comment.Mon, Jan 1, 4:16 PM

A list of ioctl names didn't seem very useful to me, so I extracted the information from the header file.

bjk added a comment.Tue, Jan 2, 5:03 AM

Thank you for putting in the list of ioctls; it is nice to see!

share/man/man4/watchdog.4
105 ↗(On Diff #37352)

"set the action when a pre-timeout occurs (see
.Li WD_SOFT_*
below)"

107 ↗(On Diff #37352)

"Use an internal"

114 ↗(On Diff #37352)

We should probably consistently hyphenate "pre-timeout"

180 ↗(On Diff #37352)

Does it make sense to say something about "as can be configured" here and list the default behavior?

karels updated this revision to Diff 37394.Tue, Jan 2, 5:31 AM

Tweak man page

Updates suggested by bjk.

bjk accepted this revision.Tue, Jan 2, 5:35 AM

Thanks, looks good for manpages. (I don't know if you're going to wait for further list discussion before committing.)

share/man/man4/watchdog.4
28 ↗(On Diff #37394)

Please update .Dd when committing (though a couple days off is not a big deal).

This revision is now accepted and ready to land.Tue, Jan 2, 5:35 AM
karels added inline comments.Tue, Jan 2, 5:40 AM
share/man/man4/watchdog.4
180 ↗(On Diff #37352)

This is not configurable via ioctl. The default is defined in the NOTES file (src/sys/conf/NOTES), which I think is the proper place.

Closed by commit rS327505: make SW_WATCHDOG dynamic (authored by karels, committed by ). · Explain WhyWed, Jan 3, 12:56 AM
This revision was automatically updated to reflect the committed changes.