public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Calvin Owens <calvin@wbinvd.org>
Cc: linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
	x86@kernel.org, Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Namhyung Kim <namhyung@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	James Clark <james.clark@linaro.org>,
	Thomas Gleixner <tglx@kernel.org>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>, Andi Kleen <ak@linux.intel.com>
Subject: Re: [PATCH v2 2/2] perf: Don't throttle based on NMI watchdog events
Date: Fri, 1 May 2026 22:54:01 +0200	[thread overview]
Message-ID: <20260501205401.GI1026330@noisy.programming.kicks-ass.net> (raw)
In-Reply-To: <33d87384aa5f96af76949d1399476779dd4f4fce.1777483573.git.calvin@wbinvd.org>

On Wed, Apr 29, 2026 at 10:36:11AM -0700, Calvin Owens wrote:
> The throttling logic in perf_sample_event_took() assumes the NMI is
> running at the maximum allowed sample rate. While this makes sense most
> of the time, it wildly overestimates the runtime of the NMI for the perf
> hardware watchdog:
> 
>     # bpftrace -e 'kprobe:perf_sample_event_took { \
> 	    printf("%s: cpu=%02d time_taken=%dns\n", \
> 	    strftime("%H:%M:%S.%f", nsecs), cpu(), arg0); }'
>     03:12:13.087003: cpu=00 time_taken=3190ns
>     03:12:13.486789: cpu=01 time_taken=2918ns
>     03:12:18.075288: cpu=03 time_taken=3308ns
>     03:12:19.797207: cpu=02 time_taken=2581ns
>     03:12:23.110317: cpu=00 time_taken=2823ns
>     03:12:23.510308: cpu=01 time_taken=2943ns
>     03:12:29.229348: cpu=03 time_taken=3669ns
>     03:12:31.656306: cpu=02 time_taken=3262ns
> 
> The NMI for the watchdog runs for 2-4us every ten seconds, but the
> math done in perf_sample_event_took() concludes it is running for
> 200-400ms every second!

For arguments sake, lets say this is an even 3us, this means we can run:

  250ms / 3us = 83333

such NMIs every second to consume 25% of CPU time. Which is in line with
the numbers it then reports no?

> When it is the only PMU event running, it can take minutes to hours of
> samples from the watchdog for the moving average to accumulate to
> something near the real mean, which causes the same little "litany" of
> sample rate throttles to happen every time Linux boots with the perf
> hardware watchdog enabled:
> 
>     perf: interrupt took too long (2526 > 2500), lowering kernel.perf_event_max_sample_rate to 79000
>     perf: interrupt took too long (3177 > 3157), lowering kernel.perf_event_max_sample_rate to 62000
>     perf: interrupt took too long (3979 > 3971), lowering kernel.perf_event_max_sample_rate to 50000
>     perf: interrupt took too long (4983 > 4973), lowering kernel.perf_event_max_sample_rate to 40000
> 
> This serves no purpose: it doesn't actually affect the runtime of the
> watchdog NMI at all. It confuses users, because it suggests their
> machine is spinning its wheels in interrupts when it isn't.
> 
> Because the watchdog NMI is so infrequent, we can avoid throttling it by
> making the throttling a two-step process: load and update a timestamp
> whenever we think we need to throttle, and only actually proceed to
> throttle if the last time that happened was less than one second ago.
> 
> This is inelegant, but it avoids touching the hot path and preserves
> current throttling behavior for real PMU use, at the cost of delaying
> the throttling by a single NMI.

This makes no sense, and it quite broken. There is no throttling and you
still need to update the numbers.

I'm thinking less AI and more real human should be involved here. If you
cannot make sense of neither the code nor the AI babbling, step away.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 6d1f8bad7e1c..c2a33cb194ce 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -623,6 +623,7 @@ core_initcall(init_events_core_sysctls);
>   */
>  #define NR_ACCUMULATED_SAMPLES 128
>  static DEFINE_PER_CPU(u64, running_sample_length);
> +static DEFINE_PER_CPU(u64, last_throttle_clock);
>  
>  static u64 __report_avg;
>  static u64 __report_allowed;
> @@ -643,6 +644,8 @@ void perf_sample_event_took(u64 sample_len_ns)
>  	u64 max_len = READ_ONCE(perf_sample_allowed_ns);
>  	u64 running_len;
>  	u64 avg_len;
> +	u64 last;
> +	u64 now;
>  	u32 max;
>  
>  	if (max_len == 0)
> @@ -663,6 +666,19 @@ 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. But
> +	 * never skip throttling if NMIs are nesting, or if any NMI runs
> +	 * for longer than one second.
> +	 */
> +	now = local_clock();
> +	last = __this_cpu_read(last_throttle_clock);
> +	if (__this_cpu_cmpxchg(last_throttle_clock, last, now) == last &&
> +	    now - last > NSEC_PER_SEC && sample_len_ns < NSEC_PER_SEC)
> +		return;
> +
>  	__report_avg = avg_len;
>  	__report_allowed = max_len;
>  
> -- 
> 2.47.3
> 

  parent reply	other threads:[~2026-05-01 20:54 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-29 17:36 [PATCH v2 0/2] Two semi-related perf throttling fixes Calvin Owens
2026-04-29 17:36 ` [PATCH v2 1/2] perf/x86: Avoid double accounting of PMU NMI latencies Calvin Owens
2026-04-29 17:36 ` [PATCH v2 2/2] perf: Don't throttle based on NMI watchdog events Calvin Owens
2026-04-29 22:08   ` Calvin Owens
2026-04-29 22:15     ` Ian Rogers
2026-04-29 22:41       ` Calvin Owens
2026-05-01 17:07     ` Calvin Owens
2026-05-01 20:54   ` Peter Zijlstra [this message]
2026-05-02  9:52     ` Calvin Owens

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260501205401.GI1026330@noisy.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=acme@kernel.org \
    --cc=adrian.hunter@intel.com \
    --cc=ak@linux.intel.com \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=calvin@wbinvd.org \
    --cc=dave.hansen@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=irogers@google.com \
    --cc=james.clark@linaro.org \
    --cc=jolsa@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=tglx@kernel.org \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox