Re: [PATCH] x86: adjust initial setting of watchdog kind

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.03.2024 09:59, Jan Beulich wrote:
> On 19.03.2024 21:35, Andrew Cooper wrote:
>> On 25/01/2024 2:12 pm, Jan Beulich wrote:
>>> "watchdog_timeout=0" is documented to disable the watchdog. Make sure
>>> this also is true when there's a subsequent "watchdog" command line
>>> option (and no further "watchdog_timeout=" one).
>>
>> We also document that latest takes precedence, at which point "watchdog"
>> would re-activate.
> 
> True,

Actually - no. Latest takes precedence doesn't matter here. "watchdog"
following "watchdog_timeout=0" is simply asking to enable the watchdog
with a timeout of 0, meaning infinity in practice. Which still is as
good as "watchdog=off".

> so perhaps ...
> 
>>> While there also switch watchdog_setup() to returning void, bringing it
>>> in line with the !CONFIG_WATCHDOG case. Further amend command line
>>> documentation to also mention the implicit effect of specifying a non-
>>> zero timeout.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> ---
>>> Alternatively "watchdog" following "watchdog_timeout=0" could be taken
>>> to mean to use the default timeout again.
> 
> ... this alternative wants following.
> 
>> I realise that watchdog_timeout is my fault, but in fairness it was an
>> early change of mine in Xen and didn't exactly get the kind of review it
>> would get these days.  It also wasn't used by XenServer in the end - we
>> just stayed at a default 5s.
>>
>> I'm very tempted to suggest deleting watchdog_timeout, and extending
>> watchdog= to have `force | <bool> | <int>s` so you could specify e.g.
>> `watchdog=10s`.

This being a set of alternatives also isn't quite right. "force" needs to
be possible to combine with a timeout value. Yet if we make it "List of",
which I was ...

>> The watchdog is off by default so I don't expect this will impact
>> people.  It is also more convenient for the end user, and means that we
>> don't have have the current split approach of two separate options
>> fighting for control over each other.
> 
> While I'd be happy to fold the two options, I don't think the watchdog
> being off by default is relevant here. People using just the
> watchdog_timeout= option with a non-zero value will already have the
> watchdog enabled. They'd need to pay attention to an eventual CHANGELOG
> entry and change their command line.
> 
> Furthermore consolidating the two options isn't going to remove any
> of the problems. What effect would e.g. "watchdog=off,10s" have? The
> principle of "latest takes precedence" assigns clear meaning to
> "watchdog=off watchdog=10s", but the above remains as ambiguous as
> e.g. "watchdog=force,0s". I'd be inclined to follow those to the
> letter, i.e. "watchdog=off,10s" sets the timeout to 10 but disables
> the watchdog while "watchdog=force,0s" simply results in a non-
> functioning watchdog (due to 0s effectively meaning 4 billion seconds
> and hence for all practical purposes "never").

... assuming anyway (despite you having it written differently), we'll
have said problems again. So perhaps

<bool> | List of [ force | <int>s ]

with a timeout of 0 disabling the watchdog and a non-zero one enabling it?

Jan