Re: [PATCH] sched/psi: clamp negative cpu_clock() skew

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Sep 28, 2026 at 04:37:43PM -0700, David Stevens wrote:
> When working with timestamps, psi is careful to only compare timestamps
> from cpu_clock(cpu) with the same cpu argument. However, on systems with
> a stable clock, that cpu argument is ignored. This means that despite
> psi's best efforts, there will be some cross-CPU cpu_clock() timestamp
> comparisons.
> 
> The cross-CPU skew in cpu_clock() timestamps is generally orders of
> magnitude smaller than psi measurement intervals, so the cyclic times
> counters naturally absorb the jitter. However, if get_recent_times()
> executes immediately after a task on another CPU enters an active stall
> state, then negative skew can cause the u32 active state duration
> calculation to underflow. If that state had not been active since the
> previous execution of get_recent_times(), then times and times_prev will
> be equal and the very large underflow value will be passed out to
> collect_percpu_times(). There, it will be zero extended and folded into
> the u64 total accumulator as a very large delta of about 4 seconds
> scaled by that CPU's share of nonidle time.
> 
> That large jump can lead to spurious wakeups for the PSI_POLL aggregator
> or incorrect values for the PSI_AVGS aggregator. Spurious triggers or
> nonsense psi values (e.g. full>some, psi values >100%) can cause
> userspace to take unnecessary corrective action, such as a userspace OOM
> daemon killing processes to relieve (non-existent) memory pressure.
> 
> When running browser workloads on an Intel N100 with a sustained
> psi.mem.some of 2-3%, this underflow is observed once every couple of
> hours.
> 
> Clamping elapsed time to >= 0 prevents this jump from happening. It does
> result in the total accumulator being slightly elevated compared to what
> it should actually be, but that error is bounded by the skew and is in
> practice always smaller than what is added today. Note that expanding
> times to u64 to prevent the u32->u64 conversion from causing issues
> would result in userspace-facing total counters no longer being
> monotonic, which could break consumers that derive rates via successive
> total values.
> 
> Clock skew can cause issues in two more places. First, negative clock
> skew in record_times() can propagate through the times accumulator to
> the delta calculation against times_prev in get_recent_times() and cause
> it to underflow. Second, the elapsed time values calculated by
> successive executions of get_recent_times() can appear to go backwards
> if the first execution has positive skew and the second has negative
> skew. This can cause the delta calculation to underflow, since elapsed
> is propagated in the times_prev value. However, both of these require
> that the growth in stall time between iterations be non-zero but smaller
> than the inter-CPU skew. Neither issue has been observed in practice and
> they are not addressed here.
> 
> Cc: stable@vger.kernel.org
> Fixes: eb414681d5a0 ("psi: pressure stall information for CPU, memory, and IO")
> Signed-off-by: David Stevens <stevensd@google.com>
> ---
>  kernel/sched/psi.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index 4e152410653d..893ad16ff42c 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -305,8 +305,24 @@ static void get_recent_times(struct psi_group *group, int cpu,
>  		 * (u32) and our reported pressure close to what's
>  		 * actually happening.
>  		 */
> -		if (state_mask & (1 << s))
> -			times[s] += now - state_start;
> +		if (state_mask & (1 << s)) {
> +			s64 elapsed = now - state_start;
> +
> +			/*
> +			 * When sched_clock_stable(), cpu_clock(cpu) ignores
> +			 * the cpu argument, so we can end up doing cross-CPU
> +			 * comparisons. A negative clock skew can result in
> +			 * underflow to a very large u32.
> +			 *
> +			 * While the u32 cyclic accumulators in groupc could
> +			 * handle a very large u32 from underflow, folding it
> +			 * into the u64 totals in collect_percpu_times()
> +			 * would result in huge jumps. Clamp to avoid that.
> +			 */

I am confused. If sched_clock_stable() you are 1) running x86 and 2)
that promises CPU A and CPU B doing RDTSC cannot observe non monotonic
movement.

If you are somehow able to see non monotonic movement, then 1) your
hardware is fucked, and 2) you should not have sched_clock_stable().

What kind of machine are you seeing this on?

> +			if (elapsed < 0)
> +				elapsed = 0;
> +			times[s] += elapsed;
> +		}
>  
>  		delta = times[s] - groupc->times_prev[aggregator][s];
>  		groupc->times_prev[aggregator][s] = times[s];
> 
> base-commit: 1fb28c664a19df8d45a6afa04d28d102b04ea680
> -- 
> 2.56.0.rc1.315.gc6ed9934b7-goog
>