Re: [PATCH] perf/x86/intel/ds: Fix x86_pmu_stop warning for large PEBS

[Peter Zijlstra]

On Mon, Jan 13, 2020 at 06:09:35AM -0800, kan.liang@xxxxxxxxxxxxxxx wrote:
> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> 
> A warning as below may be triggered when sampling large PEBS.

> [  410.729822] WARNING: CPU: 0 PID: 16397 at arch/x86/events/core.c:1422
> x86_pmu_stop+0x95/0xa0

> For large PEBS, the PEBS buffer can be drained from either NMI handler
> or !NMI e.g. context switch. Current implementation doesn't handle them
> differently. For !nmi, perf also call the generic overflow handler for
> the last PEBS record. That may trigger the interrupt throttle, and stop
> the event. That's wrong.
> 
> Here is an example for !NMI scenario, context switch.
> Let's say the max_samples_per_tick is adjusted to 2 for some reason.
> A context switch happens right after a NMI.
> When an old task is scheduled out, it will drain the PEBS buffer, and
> then delete the event.
> When draining the PEBS buffer, perf_event_overflow() will be called for
> the last PEBS record. Since the max_samples_per_tick is only 2, the
> interrupt throttle must be triggered. The event will be stopped.
> After the draining, the scheduler will delete the event, which stops the
> event again. The warning is triggered.
> 
> Perf should handle the NMI and !NMI differently for large PEBS.
> For NMI, the generic overflow handler is required for the last PEBS
> record.
> But, for !NMI, there is no overflow. The generic overflow handler should
> not be invoked. Perf should treat the last record exactly the same as
> the rest of PEBS records.

Hurmph. there's something there, but the above is hard to read.

drain_pebs() is called from:

 - handle_pmi_common()		-- sample context
 - intel_pmu_pebs_sched_task()  -- non sample context
 - intel_pmu_pebs_disable()     -- non sample context
 - intel_pmu_auto_reload_read() -- possible sample context

So the question is what to do for PERF_SAMPLE_READ + PERF_FORMAT_GROUP.

I don't think throttling there is right either, but that does mean the
simple in_nmi() test you use is wrong.

Perhaps we can do something with how intel_pmu_drain_pebs_buffer()
passes in dummy regs pointer to distinguish between the sample and non
sample context.

> ---
>  arch/x86/events/intel/ds.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index 7c896d7e8b6c..51baff083938 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1780,15 +1780,22 @@ static void __intel_pmu_pebs_event(struct perf_event *event,
>  
>  	setup_sample(event, iregs, at, &data, regs);
>  
> -	/*
> -	 * All but the last records are processed.
> -	 * The last one is left to be able to call the overflow handler.
> -	 */
> -	if (perf_event_overflow(event, &data, regs)) {
> -		x86_pmu_stop(event, 0);
> -		return;
> +	if (in_nmi()) {
> +		/*
> +		 * All but the last records are processed.
> +		 * The last one is left to be able to call the overflow handler.
> +		 */
> +		if (perf_event_overflow(event, &data, regs))
> +			x86_pmu_stop(event, 0);
> +	} else {
> +		/*
> +		 * For !NMI, e.g context switch, there is no overflow.
> +		 * The generic overflow handler should not be invoked.
> +		 * Perf should treat the last record exactly the same as the
> +		 * rest of PEBS records.
> +		 */
> +		perf_event_output(event, &data, regs);
>  	}

Maybe write it like so?

diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index 4b94ae4ae369..b66be085c7a4 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -1747,25 +1747,22 @@ static void __intel_pmu_pebs_event(struct perf_event *event,
 	} else if (!intel_pmu_save_and_restart(event))
 		return;
 
-	while (count > 1) {
+	while (count > /* cond */) {
 		setup_sample(event, iregs, at, &data, regs);
 		perf_event_output(event, &data, regs);
 		at += cpuc->pebs_record_size;
 		at = get_next_pebs_record_by_bit(at, top, bit);
-		count--;
+		if (!--count)
+			return;
 	}
 
-	setup_sample(event, iregs, at, &data, regs);
-
 	/*
 	 * All but the last records are processed.
 	 * The last one is left to be able to call the overflow handler.
 	 */
-	if (perf_event_overflow(event, &data, regs)) {
+	setup_sample(event, iregs, at, &data, regs);
+	if (perf_event_overflow(event, &data, regs))
 		x86_pmu_stop(event, 0);
-		return;
-	}
-
 }
 
 static void intel_pmu_drain_pebs_core(struct pt_regs *iregs)