make SW_WATCHDOG dynamic
ClosedPublic
Actions

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

Details

Reviewers

alfred
jeff
imp
bjk

Group Reviewers

manpages

Commits

rS327920: MFC r327505:
rS327505: make SW_WATCHDOG dynamic

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 - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• karels created this revision.Dec 31 2017, 2:49 PM

Herald added a reviewer: manpages. · View Herald TranscriptDec 31 2017, 2:49 PM

Herald added a subscriber: imp. · View Herald Transcript

Harbormaster completed remote builds in B13999: Diff 37309.Dec 31 2017, 2:49 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.Dec 31 2017, 5:11 PM

Macro lgtm:

alfred accepted this revision.Dec 31 2017, 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.Jan 1 2018, 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?

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?

In D13713#286993, @karels wrote:

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.

Update man page

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

This revision now requires review to proceed.Jan 1 2018, 4:15 PM

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

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?

Tweak man page

Updates suggested by bjk.

Harbormaster completed remote builds in B14058: Diff 37394.Jan 2 2018, 5:31 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.Jan 2 2018, 5:35 AM

• karels added inline comments.Jan 2 2018, 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.