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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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.


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.

~Andrew