Re: [PATCH 1/2] watchdog: introduce watchdog.open_timeout commandline parameter

[Guenter Roeck]

On Wed, Nov 29, 2017 at 11:56:57AM +0100, Rasmus Villemoes wrote:
> On 2017-11-28 23:14, Guenter Roeck wrote:
> > On Tue, Nov 28, 2017 at 11:35:49AM +0100, Rasmus Villemoes wrote:
> >>
> >> The unit is milliseconds rather than seconds because that covers more
> >> use cases. For example, one can effectively disable the kernel handling
> >> by setting the open_timeout to 1 ms. There are also customers with very
> >> strict requirements that may want to set the open_timeout to something
> >> like 4500 ms, which combined with a hardware watchdog that must be
> >> pinged every 250 ms ensures userspace is up no more than 5 seconds after
> >> the bootloader hands control to the kernel (250 ms until the driver gets
> >> registered and kernel handling starts, 4500 ms of kernel handling, and
> >> then up to 250 ms from the last ping until userspace takes over).
> > 
> > This is quite vague, especially since it doesn't count the time from
> > boot to starting the watchdog driver,
> 
> My example is bad, and I now realize one cannot really get such precise
> guarantees. But the example _did_ actually account for the time from
> boot to device registration - it allowed 250 ms for the kernel to get
> that far.
> 
> which can vary even across boots.
> > Why not make it specific, for example by adjusting the open timeout with
> > ktime_get_boot_ns() ?
> 
> If by "boot" we mean the moment the bootloader hands control to the
> kernel, ktime_get_boot_ns() doesn't give that either - at best, it gives
> an approximation of the time since timekeeping_init(), but it's not very
> accurate that early (I simply injected printks of ktime_get_boot_ns at
> various places in init/main.c and timestamped the output lines). If it
> overshoots, we'd be subtracting more of the allowance than we should,
> and I don't think we have any way of knowing when that happens or to
> correct for it. So I'd rather keep the code simple and let it count from
> the time the watchdog framework knows about the device, which is also
> around the time when the kernel's timekeeping is reasonably accurate.
> 
We should try to make things as accurate as possible. "It won't be perfect"
is not an argument against that.

> > I would actually make it even more specific and calculate the open
> > timeout such that the system would reboot after open_timeout, not
> > after <open_timeout + hardware_timeout>. Any reason for not doing that ?
> > The upside would be more accuracy, and I don't really see a downside.
> 
> I don't think it would be that much more accurate - we schedule the
> pings at a frequency of half the max_hw_heartbeat_ms==$x, with the
> current code we'd get rebooted somewhere between [open_deadline + $x/2,
> open_deadline + $x], and subtracting $x from the open_timeout that would
> become [open_deadline - $x/2, open_deadline]. I'd rather not have the
> reboot happen before the open_deadline. Sure, we could subtract $x/2
> instead. Then there's the case where ->max_hw_heartbeat_ms is not set,
> so we have to use ->timeout for $x, and then there's the case of $x (or
> $x/2) being greater than $open_timeout. I'd really like to keep the code
> simple. If it helps, I'd be happy to document the exact semantics of the
> open_timeout with a nice ascii art timeline.
> 
It was not much of a problem to get that right after a watchdog was opened,
by timing pings accordingly such that the HW times out when it should.
It should not be that hard to get it working for the pre-open case as well.

Guenter