Re: [PATCH 0/5] x86/nmi: Watchdog fixes/improvement Part 1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.08.2026 19:56, Andrew Cooper wrote:
> On 05/08/2026 2:42 pm, Jan Beulich wrote:
>> On 05.08.2026 14:45, Andrew Cooper wrote:
>>> This is the start of a very long rabbit hole to address the
>>> mis-classification of some watchdog NMIs as non-watchdog NMIs.  For
>>> now, just some simple and hopefully non-controvertial changes.
>>>
>>> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2733861049
>>>
>>> Andrew Cooper (5):
>>>   x86/nmi: Drop {reserve,release}_lapic_nmi()
>>>   x86/nmi: Drop K7_NMI_EVENT
>>>   x86/nmi: Misc style fixes
>>>   x86/nmi: Check MSR_MISC_ENABLE for all Intel platforms
>>>   x86/nmi: Don't configure EvtSel repeatedly
>>>
>>>  xen/arch/x86/include/asm/apic.h |   2 -
>>>  xen/arch/x86/nmi.c              | 153 ++++++++++----------------------
>>>  2 files changed, 47 insertions(+), 108 deletions(-)
>> This series, once again, is putting me in a difficult position: Should I look
>> at it, or should I let it sit for two years or more, just like my earlier
>> fixes in this area [1], [2] are? (Of course, as always so far, I will look at
>> the patches, and I will likely also accept them going in ahead of mine. But I
>> cannot exclude that at some point I might actually stop doing so, seeing how
>> many of my patches are in that state. While at the same time none of yours
>> are, afaict, i.e. as per the track record that I keep of what still needs
>> responding to.)
>>
>> Yes, you did respond to [1], but is not being comfortable with a change 
>> really
>> a reason to block it, when it _is_ an improvement, and when the alternative
>> hasn't materialized in all the time?
>>
>> Jan
>>
>> [1] https://lists.xen.org/archives/html/xen-devel/2024-01/msg01365.html
>> [2] https://lists.xen.org/archives/html/xen-devel/2024-04/msg00194.html
> 
> I'd forgotten about these.
> 
> Patch 1, I'm (still) distinctly uneasy about, but I dispute your claim
> that it is an improvement.  You are adding complexity and not fixing
> anything AFAICT.
> 
> The watchdog counts NMIs (and counts incorrectly; this is the root issue
> I'm needing to fix).  A timeout is declared when a fixed number of NMIs
> (10, in default configuration) pass without the timer softirq having run.
> 
> The rate of NMIs varies with P states, including lower than cpu_khz, and
> differs between cores.  In some but not all hardware, we could switch
> from Unhalted Cycles to Unhalted Reference Cycles, but even that has a
> bit caveat saying that the definition changed in 12th Generation.
> 
> You are making the rate of the timer softirq dynamic, but it is an
> arbitrary fixed rate still unconnected to the rate of NMIs.

And I'm not claiming to address that (independent) issue. What the patch
does fix is a watchdog timeout occurring too early when a CPU runs in
turbo mode for perhaps an extended period of time.

> The only fix is to make it safe for the NMI handler to read real time. 
> Until that time, in a choice between your patch and saying "well don't
> set watchdog_timeout=1 then", I'd firmly favour the latter because at
> least it means there's less to revert when a real fix does come along.

As said in the description, if the ratio between max and normal is high
enough, even the default of 5 could be a problem.

> For patch 2, I had figured that bug out independently though inspection,
> and yes I do agree it's an issue.  I was debating removing
> watchdog_timeout=, and agree with that aspect of the patch.  However,
> watchdog_force needs deleting to fix the incorrect counting, and with
> your /* reset to defaults */ you're breaking the incremental property we
> have of command line parsing elsewhere; specifically "watchdog=force
> watchdog=10s" now sets force to false.
> 
> I will make sure to address this bug in my series, but I think it will
> be a fairly different patch when the other dust has settled.

Okay, we'll see if and when that arrives. With your intent to address
this differently, I don't see a reason then to try and adjust the cmdline
behavior. FTR, with watchdog= in particular I'm rather uncertain whether
the common (but unwritten) "incremental" policy is appropriate.

Jan