Re: [Patch v5 16/19] perf/x86: Activate back-to-back NMI detection for arch-PEBS induced NMIs

[Peter Zijlstra <peterz@infradead.org>]

On Wed, Dec 03, 2025 at 02:54:57PM +0800, Dapeng Mi wrote:
> When two or more identical PEBS events with the same sampling period are
> programmed on a mix of PDIST and non-PDIST counters, multiple
> back-to-back NMIs can be triggered.

This is a hardware defect -- albeit a fairly common one.


> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index da48bcde8fce..a130d3f14844 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -3351,8 +3351,7 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status)
>  	 */
>  	if (__test_and_clear_bit(GLOBAL_STATUS_ARCH_PEBS_THRESHOLD_BIT,
>  				 (unsigned long *)&status)) {
> -		handled++;
> -		static_call(x86_pmu_drain_pebs)(regs, &data);
> +		handled += static_call(x86_pmu_drain_pebs)(regs, &data);
>  
>  		if (cpuc->events[INTEL_PMC_IDX_FIXED_SLOTS] &&
>  		    is_pebs_counter_event_group(cpuc->events[INTEL_PMC_IDX_FIXED_SLOTS]))

Note that the old code would return handled++, while the new code:

> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index a01c72c03bd6..c7cdcd585574 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -2759,7 +2759,7 @@ __intel_pmu_pebs_events(struct perf_event *event,
>  	__intel_pmu_pebs_last_event(event, iregs, regs, data, at, count, setup_sample);
>  }
>  
> -static void intel_pmu_drain_pebs_core(struct pt_regs *iregs, struct perf_sample_data *data)
> +static int intel_pmu_drain_pebs_core(struct pt_regs *iregs, struct perf_sample_data *data)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  	struct debug_store *ds = cpuc->ds;
> @@ -2768,7 +2768,7 @@ static void intel_pmu_drain_pebs_core(struct pt_regs *iregs, struct perf_sample_
>  	int n;
>  
>  	if (!x86_pmu.pebs_active)
> -		return;
> +		return 0;
>  
>  	at  = (struct pebs_record_core *)(unsigned long)ds->pebs_buffer_base;
>  	top = (struct pebs_record_core *)(unsigned long)ds->pebs_index;
> @@ -2779,22 +2779,24 @@ static void intel_pmu_drain_pebs_core(struct pt_regs *iregs, struct perf_sample_
>  	ds->pebs_index = ds->pebs_buffer_base;
>  
>  	if (!test_bit(0, cpuc->active_mask))
> -		return;
> +		return 0;
>  
>  	WARN_ON_ONCE(!event);
>  
>  	if (!event->attr.precise_ip)
> -		return;
> +		return 0;
>  
>  	n = top - at;
>  	if (n <= 0) {
>  		if (event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD)
>  			intel_pmu_save_and_restart_reload(event, 0);
> -		return;
> +		return 0;
>  	}
>  
>  	__intel_pmu_pebs_events(event, iregs, data, at, top, 0, n,
>  				setup_pebs_fixed_sample_data);
> +
> +	return 0;
>  }
>  
>  static void intel_pmu_pebs_event_update_no_drain(struct cpu_hw_events *cpuc, u64 mask)
> @@ -2817,7 +2819,7 @@ static void intel_pmu_pebs_event_update_no_drain(struct cpu_hw_events *cpuc, u64
>  	}
>  }
>  
> -static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_data *data)
> +static int intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_data *data)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  	struct debug_store *ds = cpuc->ds;
> @@ -2830,7 +2832,7 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_d
>  	u64 mask;
>  
>  	if (!x86_pmu.pebs_active)
> -		return;
> +		return 0;
>  
>  	base = (struct pebs_record_nhm *)(unsigned long)ds->pebs_buffer_base;
>  	top = (struct pebs_record_nhm *)(unsigned long)ds->pebs_index;
> @@ -2846,7 +2848,7 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_d
>  
>  	if (unlikely(base >= top)) {
>  		intel_pmu_pebs_event_update_no_drain(cpuc, mask);
> -		return;
> +		return 0;
>  	}
>  
>  	for (at = base; at < top; at += x86_pmu.pebs_record_size) {
> @@ -2931,6 +2933,8 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_d
>  						setup_pebs_fixed_sample_data);
>  		}
>  	}
> +
> +	return 0;
>  }
>  
>  static __always_inline void
> @@ -2984,7 +2988,7 @@ __intel_pmu_handle_last_pebs_record(struct pt_regs *iregs,
>  
>  }
>  
> -static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_data *data)
> +static int intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_data *data)
>  {
>  	short counts[INTEL_PMC_IDX_FIXED + MAX_FIXED_PEBS_EVENTS] = {};
>  	void *last[INTEL_PMC_IDX_FIXED + MAX_FIXED_PEBS_EVENTS];
> @@ -2997,7 +3001,7 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
>  	u64 mask;
>  
>  	if (!x86_pmu.pebs_active)
> -		return;
> +		return 0;
>  
>  	base = (struct pebs_basic *)(unsigned long)ds->pebs_buffer_base;
>  	top = (struct pebs_basic *)(unsigned long)ds->pebs_index;
> @@ -3010,7 +3014,7 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
>  
>  	if (unlikely(base >= top)) {
>  		intel_pmu_pebs_event_update_no_drain(cpuc, mask);
> -		return;
> +		return 0;
>  	}
>  
>  	if (!iregs)
> @@ -3032,9 +3036,11 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
>  
>  	__intel_pmu_handle_last_pebs_record(iregs, regs, data, mask, counts, last,
>  					    setup_pebs_adaptive_sample_data);
> +
> +	return 0;
>  }

will now return handled+=0 for all these. Which is a change in
behaviour. Also:

> -static void intel_pmu_drain_arch_pebs(struct pt_regs *iregs,
> +static int intel_pmu_drain_arch_pebs(struct pt_regs *iregs,
>  				      struct perf_sample_data *data)
>  {
>  	short counts[INTEL_PMC_IDX_FIXED + MAX_FIXED_PEBS_EVENTS] = {};
> @@ -3044,13 +3050,14 @@ static void intel_pmu_drain_arch_pebs(struct pt_regs *iregs,
>  	struct x86_perf_regs perf_regs;
>  	struct pt_regs *regs = &perf_regs.regs;
>  	void *base, *at, *top;
> +	u64 events_bitmap = 0;
>  	u64 mask;
>  
>  	rdmsrq(MSR_IA32_PEBS_INDEX, index.whole);
>  
>  	if (unlikely(!index.wr)) {
>  		intel_pmu_pebs_event_update_no_drain(cpuc, X86_PMC_IDX_MAX);
> -		return;
> +		return 0;
>  	}
>  
>  	base = cpuc->pebs_vaddr;
> @@ -3089,6 +3096,7 @@ static void intel_pmu_drain_arch_pebs(struct pt_regs *iregs,
>  
>  		basic = at + sizeof(struct arch_pebs_header);
>  		pebs_status = mask & basic->applicable_counters;
> +		events_bitmap |= pebs_status;
>  		__intel_pmu_handle_pebs_record(iregs, regs, data, at,
>  					       pebs_status, counts, last,
>  					       setup_arch_pebs_sample_data);
> @@ -3108,6 +3116,8 @@ static void intel_pmu_drain_arch_pebs(struct pt_regs *iregs,
>  	__intel_pmu_handle_last_pebs_record(iregs, regs, data, mask,
>  					    counts, last,
>  					    setup_arch_pebs_sample_data);
> +
	/*
	 * Comment that explains the arch pebs defect goes here.
	 */
> +	return hweight64(events_bitmap);
>  }