Re: [PATCH 4/4] perf/x86/intel: Throttle PEBS events only from pmi

[Jiri Olsa <jolsa@redhat.com>]

On Tue, Jan 24, 2017 at 05:41:22PM +0100, Peter Zijlstra wrote:

SNIP

> 
> So I don't particularly like these patches... they make a wee bit of a
> mess.
> 
> Under the assumption that draining a single event is on the same order
> of cost as a regular PMI, then accounting a drain of multiple events as
> an equal amount of interrupts makes sense.

I understood it like we're trying more to protect the interrupt
context abuse rather than account the samples here.. but yes,
we compare (hwc->interrupts >= max_samples_per_tick) for throttling
so I guess we should not cross max_samples_per_tick in any case

> 
> We should not disregard this work. Now it looks like both (BTS & PEBS)
> drain methods only count a single interrupt, that's something we maybe
> ought to fix too.

right, we account just single hwc->interrupts++, and BTS drain does
not have the throttle logic

I tried to put the perf_event_overflow call into both PEBS and BTS
drains (below, untested). I think it's going to be slower especially
for BTS code, which bypassed single event allocations and allocated
one buffer for all samples at once.

jirka


---
diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index be202390bbd3..703bd82c69b1 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -520,10 +520,7 @@ int intel_pmu_drain_bts_buffer(void)
 	};
 	struct perf_event *event = cpuc->events[INTEL_PMC_IDX_FIXED_BTS];
 	struct bts_record *at, *base, *top;
-	struct perf_output_handle handle;
-	struct perf_event_header header;
 	struct perf_sample_data data;
-	unsigned long skip = 0;
 	struct pt_regs regs;
 
 	if (!event)
@@ -562,40 +559,18 @@ int intel_pmu_drain_bts_buffer(void)
 		 */
 		if (event->attr.exclude_kernel &&
 		    (kernel_ip(at->from) || kernel_ip(at->to)))
-			skip++;
-	}
-
-	/*
-	 * Prepare a generic sample, i.e. fill in the invariant fields.
-	 * We will overwrite the from and to address before we output
-	 * the sample.
-	 */
-	rcu_read_lock();
-	perf_prepare_sample(&header, &data, event, &regs);
-
-	if (perf_output_begin(&handle, event, header.size *
-			      (top - base - skip)))
-		goto unlock;
-
-	for (at = base; at < top; at++) {
-		/* Filter out any records that contain kernel addresses. */
-		if (event->attr.exclude_kernel &&
-		    (kernel_ip(at->from) || kernel_ip(at->to)))
 			continue;
 
 		data.ip		= at->from;
 		data.addr	= at->to;
 
-		perf_output_sample(&handle, &header, &data, event);
-	}
+		if (perf_event_overflow(event, &data, &regs)) {
+			x86_pmu_stop(event, 0);
+			break;
+		}
 
-	perf_output_end(&handle);
+	}
 
-	/* There's new data available. */
-	event->hw.interrupts++;
-	event->pending_kill = POLL_IN;
-unlock:
-	rcu_read_unlock();
 	return 1;
 }
 
@@ -1243,25 +1218,18 @@ static void __intel_pmu_pebs_event(struct perf_event *event,
 	    !(event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD))
 		return;
 
-	while (count > 1) {
+	while (count > 0) {
 		setup_pebs_sample_data(event, iregs, at, &data, &regs);
-		perf_event_output(event, &data, &regs);
+
+		if (perf_event_overflow(event, &data, &regs)) {
+			x86_pmu_stop(event, 0);
+			break;
+		}
+
 		at += x86_pmu.pebs_record_size;
 		at = get_next_pebs_record_by_bit(at, top, bit);
 		count--;
 	}
-
-	setup_pebs_sample_data(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;
-	}
-
 }
 
 static void intel_pmu_drain_pebs_core(struct pt_regs *iregs)