Re: [PATCH] x86: increase NMI timer frequency if necessary

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.03.2024 21:51, Andrew Cooper wrote:
> On 25/01/2024 4:55 pm, Jan Beulich wrote:
>> Since the performance counters used for the NMI watchdog count non-
>> halted cycles, they may count at a rate higher than cpu_khz.
> 
> Is this in theory, or observed in practice?

It's been over two months since doing the experiments, so I can only go
from memory, but my recollection is that I actually observed higher
rates, just not high enough (yet) for the watchdog (without this change)
to start malfunctioning.

> It is my understanding that perf counters count in P0 reference cycles,
> and not at the Turbo/CBS rate.
> 
>>  Thus the
>> watchdog tick may occur more frequently than invocations of the timer
>> if we don't account for the ratio between nominal and maximum CPU clock
>> speeds, which would be a problem in particular when "watchdog_timeout=1"
>> is in effect (for high enough ratios even larger timout values may pose
>> a problem).
>>
>> Leverage the so far display-only data we collect on newer Intel and AMD
>> CPUs. On older CPUs we just have to (continue to) hope that the default
>> frequency of 1 Hz is okay(-ish) to use.
>>
>> While adding the new variable, also move the (now adjacent) cpu_khz to
>> .data.ro_after_init.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> This renders the "log" in the function names somewhat stale, but I don't
>> think this strictly warrants renaming the functions right away.
> 
> I'm not comfortable with this change.  It's adding to a complicated
> timing problem, rather than simplifying it.

The actual change to the watchdog logic is minimal - a build-time constant
is replaced by a boot-time determined value.

> The real problem we've got is that the NMI handler is guessing at the
> timeout by counting NMIs, not by actually counting time.  There are
> several ways to fix this even with the current rendezvous logic.  When
> the NMI handler can actually say "if ( NOW() - last > timeout )", then
> the exact frequently of NMIs becomes far less important.

But that would come with its own downsides: The logic within the NMI
handler should be as simple as possible, involving as little as possible
other code. NOW(), for example, cannot really be used there without
first fiddling with the time rendezvous (to make sure an NMI hitting in
the middle of an update to the scaling values will know how to use
consistent data; that could e.g. be done by flip-flopping between two
instances of the data, with a "selector" always flipped last).

Jan