Re: [PATCH 2/2] perf: Don't throttle based on NMI watchdog events

[Calvin Owens]

On Tuesday 03/31 at 08:25 -0700, Calvin Owens wrote:
> @@ -663,6 +666,17 @@ void perf_sample_event_took(u64 sample_len_ns)
>  	if (avg_len <= max_len)
>  		return;
>  
> +	/*
> +	 * Very infrequent events like the perf counter hard watchdog
> +	 * can trigger spurious throttling: skip throttling if the prior
> +	 * NMI got here more than one second before this NMI began.
> +	 */
> +	now = local_clock();
> +	delta = now - __this_cpu_read(last_throttle_clock);
> +	__this_cpu_write(last_throttle_clock, now);
> +	if (delta - sample_len_ns > NSEC_PER_SEC)
> +		return;

Bah, Sashiko caught something obvious I missed:

https://sashiko.dev/#/patchset/cover.1774969692.git.calvin%40wbinvd.org

>> When the outer handler completes, its sample_len_ns (total execution
>> time) will be strictly greater than delta (time since the inner
>> handler finished).  This guarantees delta < sample_len_ns, causing the
>> subtraction to underflow to a massive positive value.
>>
>> The condition > NSEC_PER_SEC will then evaluate to true, and the outer
>> handler will erroneously skip the perf throttling logic. Should this
>> check be rewritten to avoid subtraction, perhaps by using if (delta >
>> sample_len_ns + NSEC_PER_SEC)?

The solution it proposed makes sense to me.

>  	__report_avg = avg_len;
>  	__report_allowed = max_len;
>  
> -- 
> 2.47.3
>