[PATCH 1/2] perf/x86/intel/ds: Clarify adaptive PEBS processing

[PATCH 1/2] perf/x86/intel/ds: Clarify adaptive PEBS processing

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

Modify the pebs_basic and pebs_meminfo structs to make the bitfields
more explicit to ease readability of the code.

Co-developed-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 arch/x86/events/intel/ds.c        | 43 ++++++++++++++-----------------
 arch/x86/include/asm/perf_event.h | 16 ++++++++++--
 2 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index 8afc4ad3cd16..4d0f7c49295a 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -1917,8 +1917,6 @@ static void adaptive_pebs_save_regs(struct pt_regs *regs,
 }
 
 #define PEBS_LATENCY_MASK			0xffff
-#define PEBS_CACHE_LATENCY_OFFSET		32
-#define PEBS_RETIRE_LATENCY_OFFSET		32
 
 /*
  * With adaptive PEBS the layout depends on what fields are configured.
@@ -1932,8 +1930,7 @@ static void setup_pebs_adaptive_sample_data(struct perf_event *event,
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 	struct pebs_basic *basic = __pebs;
 	void *next_record = basic + 1;
-	u64 sample_type;
-	u64 format_size;
+	u64 sample_type, format_group;
 	struct pebs_meminfo *meminfo = NULL;
 	struct pebs_gprs *gprs = NULL;
 	struct x86_perf_regs *perf_regs;
@@ -1945,7 +1942,7 @@ static void setup_pebs_adaptive_sample_data(struct perf_event *event,
 	perf_regs->xmm_regs = NULL;
 
 	sample_type = event->attr.sample_type;
-	format_size = basic->format_size;
+	format_group = basic->format_group;
 	perf_sample_data_init(data, 0, event->hw.last_period);
 	data->period = event->hw.last_period;
 
@@ -1967,7 +1964,7 @@ static void setup_pebs_adaptive_sample_data(struct perf_event *event,
 
 	if (sample_type & PERF_SAMPLE_WEIGHT_STRUCT) {
 		if (x86_pmu.flags & PMU_FL_RETIRE_LATENCY)
-			data->weight.var3_w = format_size >> PEBS_RETIRE_LATENCY_OFFSET & PEBS_LATENCY_MASK;
+			data->weight.var3_w = basic->retire_latency;
 		else
 			data->weight.var3_w = 0;
 	}
@@ -1977,12 +1974,12 @@ static void setup_pebs_adaptive_sample_data(struct perf_event *event,
 	 * But PERF_SAMPLE_TRANSACTION needs gprs->ax.
 	 * Save the pointer here but process later.
 	 */
-	if (format_size & PEBS_DATACFG_MEMINFO) {
+	if (format_group & PEBS_DATACFG_MEMINFO) {
 		meminfo = next_record;
 		next_record = meminfo + 1;
 	}
 
-	if (format_size & PEBS_DATACFG_GP) {
+	if (format_group & PEBS_DATACFG_GP) {
 		gprs = next_record;
 		next_record = gprs + 1;
 
@@ -1995,14 +1992,13 @@ static void setup_pebs_adaptive_sample_data(struct perf_event *event,
 			adaptive_pebs_save_regs(regs, gprs);
 	}
 
-	if (format_size & PEBS_DATACFG_MEMINFO) {
+	if (format_group & PEBS_DATACFG_MEMINFO) {
 		if (sample_type & PERF_SAMPLE_WEIGHT_TYPE) {
-			u64 weight = meminfo->latency;
+			u64 latency = x86_pmu.flags & PMU_FL_INSTR_LATENCY ?
+					meminfo->cache_latency : meminfo->mem_latency;
 
-			if (x86_pmu.flags & PMU_FL_INSTR_LATENCY) {
-				data->weight.var2_w = weight & PEBS_LATENCY_MASK;
-				weight >>= PEBS_CACHE_LATENCY_OFFSET;
-			}
+			if (x86_pmu.flags & PMU_FL_INSTR_LATENCY)
+				data->weight.var2_w = meminfo->instr_latency;
 
 			/*
 			 * Although meminfo::latency is defined as a u64,
@@ -2010,12 +2006,13 @@ static void setup_pebs_adaptive_sample_data(struct perf_event *event,
 			 * in practice on Ice Lake and earlier platforms.
 			 */
 			if (sample_type & PERF_SAMPLE_WEIGHT) {
-				data->weight.full = weight ?:
+				data->weight.full = latency ?:
 					intel_get_tsx_weight(meminfo->tsx_tuning);
 			} else {
-				data->weight.var1_dw = (u32)(weight & PEBS_LATENCY_MASK) ?:
+				data->weight.var1_dw = (u32)latency ?:
 					intel_get_tsx_weight(meminfo->tsx_tuning);
 			}
+
 			data->sample_flags |= PERF_SAMPLE_WEIGHT_TYPE;
 		}
 
@@ -2036,16 +2033,16 @@ static void setup_pebs_adaptive_sample_data(struct perf_event *event,
 		}
 	}
 
-	if (format_size & PEBS_DATACFG_XMMS) {
+	if (format_group & PEBS_DATACFG_XMMS) {
 		struct pebs_xmm *xmm = next_record;
 
 		next_record = xmm + 1;
 		perf_regs->xmm_regs = xmm->xmm;
 	}
 
-	if (format_size & PEBS_DATACFG_LBRS) {
+	if (format_group & PEBS_DATACFG_LBRS) {
 		struct lbr_entry *lbr = next_record;
-		int num_lbr = ((format_size >> PEBS_DATACFG_LBR_SHIFT)
+		int num_lbr = ((format_group >> PEBS_DATACFG_LBR_SHIFT)
 					& 0xff) + 1;
 		next_record = next_record + num_lbr * sizeof(struct lbr_entry);
 
@@ -2055,11 +2052,11 @@ static void setup_pebs_adaptive_sample_data(struct perf_event *event,
 		}
 	}
 
-	WARN_ONCE(next_record != __pebs + (format_size >> 48),
-			"PEBS record size %llu, expected %llu, config %llx\n",
-			format_size >> 48,
+	WARN_ONCE(next_record != __pebs + basic->format_size,
+			"PEBS record size %u, expected %llu, config %llx\n",
+			basic->format_size,
 			(u64)(next_record - __pebs),
-			basic->format_size);
+			format_group);
 }
 
 static inline void *
diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
index 91b73571412f..cd8023d5ea46 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -422,7 +422,9 @@ static inline bool is_topdown_idx(int idx)
  */
 
 struct pebs_basic {
-	u64 format_size;
+	u64 format_group:32,
+	    retire_latency:16,
+	    format_size:16;
 	u64 ip;
 	u64 applicable_counters;
 	u64 tsc;
@@ -431,7 +433,17 @@ struct pebs_basic {
 struct pebs_meminfo {
 	u64 address;
 	u64 aux;
-	u64 latency;
+	union {
+		/* pre Alder Lake */
+		u64 mem_latency;
+		/* Alder Lake and later */
+		struct {
+			u64 instr_latency:16;
+			u64 pad2:16;
+			u64 cache_latency:16;
+			u64 pad3:16;
+		};
+	};
 	u64 tsx_tuning;
 };
 
-- 
2.38.1

[PATCH 2/2] perf/x86/intel/ds: Simplify the PEBS records processing for adaptive PEBS

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

The current code may iterate all the PEBS records in the DS area several
times. The first loop is to find all active events and calculate the
available records for each event. Then iterate the whole buffer again
and again to process available records until all active events are
processed.

The algorithm is inherited from the old generations. The old PEBS
hardware does not deal well with the situation when events happen near
each other. SW has to drop the error records. Multiple iterations are
required.

The hardware limit has been addressed on newer platforms with adaptive
PEBS. A simple one-iteration algorithm is introduced.

The samples are output by record order with the patch, rather than the
event order. It doesn't impact the post-processing. The perf tool always
sorts the records by time before presenting them to the end user.

In an NMI, the last record has to be specially handled. Add a
unprocessed[] variable to track the last unprocessed record of each
event.

Save and restart the event after all records are processed.

Test:

11 PEBS events are used in the perf test. Only the basic information is
collected. 
perf record -e instructions:up,...,instructions:up -c 2000003 benchmark

The ftrace is used to record the duration of the
intel_pmu_drain_pebs_icl().

The average duration reduced from 62.04us to 57.94us.

A small improvement can be observed with the new algorithm.
Also, the implementation becomes simpler and more straightforward.

Suggested-by: Stephane Eranian <eranian@google.com>
Reviewed-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 arch/x86/events/intel/ds.c | 85 ++++++++++++++++++++++++++++++++------
 1 file changed, 72 insertions(+), 13 deletions(-)

diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index 4d0f7c49295a..cbf2ab9ed4c8 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -2400,12 +2400,38 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_d
 	}
 }
 
+static inline void __intel_pmu_pebs_event_output(struct perf_event *event,
+						 struct pt_regs *iregs,
+						 void *record, bool last,
+						 struct perf_sample_data *data)
+{
+	struct x86_perf_regs perf_regs;
+	struct pt_regs *regs = &perf_regs.regs;
+	static struct pt_regs dummy_iregs;
+
+	if (!iregs)
+		iregs = &dummy_iregs;
+
+	setup_pebs_adaptive_sample_data(event, iregs, record, data, regs);
+	if (last) {
+		/*
+		 * 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
+		perf_event_output(event, data, regs);
+}
+
 static void 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 *unprocessed[INTEL_PMC_IDX_FIXED + MAX_FIXED_PEBS_EVENTS];
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 	struct debug_store *ds = cpuc->ds;
 	struct perf_event *event;
+	struct pebs_basic *basic;
 	void *base, *at, *top;
 	int bit;
 	u64 mask;
@@ -2426,30 +2452,63 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
 		return;
 	}
 
-	for (at = base; at < top; at += cpuc->pebs_record_size) {
+	for (at = base; at < top; at += basic->format_size) {
 		u64 pebs_status;
 
-		pebs_status = get_pebs_status(at) & cpuc->pebs_enabled;
-		pebs_status &= mask;
+		basic = at;
+		if (WARN_ON_ONCE(basic->format_size != cpuc->pebs_record_size))
+			continue;
+
+		pebs_status = basic->applicable_counters & cpuc->pebs_enabled & mask;
+		for_each_set_bit(bit, (unsigned long *)&pebs_status, X86_PMC_IDX_MAX) {
+			event = cpuc->events[bit];
+
+			if (WARN_ON_ONCE(!event) ||
+			    WARN_ON_ONCE(!event->attr.precise_ip))
+				continue;
+
+			/*
+			 * Need at least one record to call the overflow handler later.
+			 * Initialize the unprocessed[] variable with the first record.
+			 */
+			if (!counts[bit]++) {
+				unprocessed[bit] = at;
+				continue;
+			}
+
+			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], false, data);
 
-		for_each_set_bit(bit, (unsigned long *)&pebs_status, X86_PMC_IDX_MAX)
-			counts[bit]++;
+			unprocessed[bit] = at;
+		}
 	}
 
 	for_each_set_bit(bit, (unsigned long *)&mask, X86_PMC_IDX_MAX) {
-		if (counts[bit] == 0)
+		if (!counts[bit])
 			continue;
 
 		event = cpuc->events[bit];
-		if (WARN_ON_ONCE(!event))
-			continue;
 
-		if (WARN_ON_ONCE(!event->attr.precise_ip))
-			continue;
+		if (!iregs) {
+			/*
+			 * The PEBS records may be drained in the non-overflow context,
+			 * e.g., large PEBS + context switch. Perf should treat the
+			 * last record the same as other PEBS records, and doesn't
+			 * invoke the generic overflow handler.
+			 */
+			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], false, data);
+		} else
+			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], true, data);
 
-		__intel_pmu_pebs_event(event, iregs, data, base,
-				       top, bit, counts[bit],
-				       setup_pebs_adaptive_sample_data);
+		if (event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD) {
+			/*
+			 * Now, auto-reload is only enabled in fixed period mode.
+			 * The reload value is always hwc->sample_period.
+			 * May need to change it, if auto-reload is enabled in
+			 * freq mode later.
+			 */
+			intel_pmu_save_and_restart_reload(event, counts[bit]);
+		} else
+			intel_pmu_save_and_restart(event);
 	}
 }
 
-- 
2.38.1

Re: [PATCH 2/2] perf/x86/intel/ds: Simplify the PEBS records processing for adaptive PEBS

[Peter Zijlstra]

On Wed, Nov 13, 2024 at 07:14:27AM -0800, kan.liang@linux.intel.com wrote:

> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index 4d0f7c49295a..cbf2ab9ed4c8 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -2400,12 +2400,38 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_d
>  	}
>  }
>  
> +static inline void __intel_pmu_pebs_event_output(struct perf_event *event,
> +						 struct pt_regs *iregs,
> +						 void *record, bool last,
> +						 struct perf_sample_data *data)
> +{
> +	struct x86_perf_regs perf_regs;
> +	struct pt_regs *regs = &perf_regs.regs;
> +	static struct pt_regs dummy_iregs;
> +
> +	if (!iregs)
> +		iregs = &dummy_iregs;
> +
> +	setup_pebs_adaptive_sample_data(event, iregs, record, data, regs);
> +	if (last) {
> +		/*
> +		 * 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
> +		perf_event_output(event, data, regs);
> +}

*sigh*... more unification please.

>  static void 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 *unprocessed[INTEL_PMC_IDX_FIXED + MAX_FIXED_PEBS_EVENTS];
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  	struct debug_store *ds = cpuc->ds;
>  	struct perf_event *event;
> +	struct pebs_basic *basic;
>  	void *base, *at, *top;
>  	int bit;
>  	u64 mask;
> @@ -2426,30 +2452,63 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
>  		return;
>  	}
>  
> -	for (at = base; at < top; at += cpuc->pebs_record_size) {
> +	for (at = base; at < top; at += basic->format_size) {
>  		u64 pebs_status;
>  
> -		pebs_status = get_pebs_status(at) & cpuc->pebs_enabled;
> -		pebs_status &= mask;
> +		basic = at;
> +		if (WARN_ON_ONCE(basic->format_size != cpuc->pebs_record_size))
> +			continue;
> +
> +		pebs_status = basic->applicable_counters & cpuc->pebs_enabled & mask;
> +		for_each_set_bit(bit, (unsigned long *)&pebs_status, X86_PMC_IDX_MAX) {
> +			event = cpuc->events[bit];
> +
> +			if (WARN_ON_ONCE(!event) ||
> +			    WARN_ON_ONCE(!event->attr.precise_ip))
> +				continue;
> +
> +			/*
> +			 * Need at least one record to call the overflow handler later.
> +			 * Initialize the unprocessed[] variable with the first record.
> +			 */
> +			if (!counts[bit]++) {
> +				unprocessed[bit] = at;
> +				continue;
> +			}
> +
> +			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], false, data);
>  
> -		for_each_set_bit(bit, (unsigned long *)&pebs_status, X86_PMC_IDX_MAX)
> -			counts[bit]++;
> +			unprocessed[bit] = at;
> +		}
>  	}
>  
>  	for_each_set_bit(bit, (unsigned long *)&mask, X86_PMC_IDX_MAX) {
> -		if (counts[bit] == 0)
> +		if (!counts[bit])
>  			continue;
>  
>  		event = cpuc->events[bit];
> -		if (WARN_ON_ONCE(!event))
> -			continue;
>  
> -		if (WARN_ON_ONCE(!event->attr.precise_ip))
> -			continue;
> +		if (!iregs) {
> +			/*
> +			 * The PEBS records may be drained in the non-overflow context,
> +			 * e.g., large PEBS + context switch. Perf should treat the
> +			 * last record the same as other PEBS records, and doesn't
> +			 * invoke the generic overflow handler.
> +			 */
> +			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], false, data);
> +		} else
> +			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], true, data);

*sigh*, this is confusing as all hell. Both are last, but one says
last=false.

> -		__intel_pmu_pebs_event(event, iregs, data, base,
> -				       top, bit, counts[bit],
> -				       setup_pebs_adaptive_sample_data);
> +		if (event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD) {
> +			/*
> +			 * Now, auto-reload is only enabled in fixed period mode.
> +			 * The reload value is always hwc->sample_period.
> +			 * May need to change it, if auto-reload is enabled in
> +			 * freq mode later.
> +			 */
> +			intel_pmu_save_and_restart_reload(event, counts[bit]);
> +		} else
> +			intel_pmu_save_and_restart(event);

And this is randomly ignoring the return value where previously we would
abort.

>  	}
>  }

How's this completely untested delta?

---
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -2167,24 +2167,33 @@ intel_pmu_save_and_restart_reload(struct
 	return 0;
 }
 
+void (*setup_fn)(struct perf_event *, struct pt_regs *, void *,
+		 struct perf_sample_data *, struct pt_regs *);
+
+static struct pt_regs dummy_iregs;
+
 static __always_inline void
 __intel_pmu_pebs_event(struct perf_event *event,
 		       struct pt_regs *iregs,
-		       struct perf_sample_data *data,
-		       void *base, void *top,
-		       int bit, int count,
-		       void (*setup_sample)(struct perf_event *,
-					    struct pt_regs *,
-					    void *,
-					    struct perf_sample_data *,
-					    struct pt_regs *))
+		       struct pt_regs *regs,
+		       struct perf_event_sample_data *data,
+		       void *at,
+		       setup_fn setup_sample)
+{
+	setup_sample(event, iregs, at, data, regs);
+	perf_event_output(event, data, regs);
+}
+
+static __always_inline void
+__intel_pmu_pebs_last_event(struct perf_event *event,
+			    struct pt_regs *iregs,
+			    struct pt_regs *regs,
+			    struct perf_event_sample_data *data,
+			    void *at,
+			    int count,
+			    setup_fn setup_sample)
 {
-	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 	struct hw_perf_event *hwc = &event->hw;
-	struct x86_perf_regs perf_regs;
-	struct pt_regs *regs = &perf_regs.regs;
-	void *at = get_next_pebs_record_by_bit(base, top, bit);
-	static struct pt_regs dummy_iregs;
 
 	if (hwc->flags & PERF_X86_EVENT_AUTO_RELOAD) {
 		/*
@@ -2194,21 +2203,12 @@ __intel_pmu_pebs_event(struct perf_event
 		 * freq mode later.
 		 */
 		intel_pmu_save_and_restart_reload(event, count);
-	} else if (!intel_pmu_save_and_restart(event))
-		return;
-
-	if (!iregs)
-		iregs = &dummy_iregs;
-
-	while (count > 1) {
-		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--;
+	} else {
+		intel_pmu_save_and_restart(event);
 	}
 
 	setup_sample(event, iregs, at, data, regs);
+
 	if (iregs == &dummy_iregs) {
 		/*
 		 * The PEBS records may be drained in the non-overflow context,
@@ -2227,6 +2227,34 @@ __intel_pmu_pebs_event(struct perf_event
 	}
 }
 
+static __always_inline void
+__intel_pmu_pebs_events(struct perf_event *event,
+			struct pt_regs *iregs,
+			struct perf_sample_data *data,
+			void *base, void *top,
+			int bit, int count,
+			setup_fn setup_sample) {
+{
+	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
+	struct hw_perf_event *hwc = &event->hw;
+	struct x86_perf_regs perf_regs;
+	struct pt_regs *regs = &perf_regs.regs;
+	void *at = get_next_pebs_record_by_bit(base, top, bit);
+	int cnt = count;
+
+	if (!iregs)
+		iregs = &dummy_iregs;
+
+	while (cnt > 1) {
+		__intel_pmu_pebs_event(event, iregs, regs, data, at, setup_sample);
+		at += cpuc->pebs_record_size;
+		at = get_next_pebs_record_by_bit(at, top, bit);
+		cnt--;
+	}
+
+	__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)
 {
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
@@ -2261,7 +2289,7 @@ static void intel_pmu_drain_pebs_core(st
 		return;
 	}
 
-	__intel_pmu_pebs_event(event, iregs, data, at, top, 0, n,
+	__intel_pmu_pebs_events(event, iregs, data, at, top, 0, n,
 			       setup_pebs_fixed_sample_data);
 }
 
@@ -2393,43 +2421,21 @@ static void intel_pmu_drain_pebs_nhm(str
 		}
 
 		if (counts[bit]) {
-			__intel_pmu_pebs_event(event, iregs, data, base,
-					       top, bit, counts[bit],
-					       setup_pebs_fixed_sample_data);
+			__intel_pmu_pebs_events(event, iregs, data, base,
+						top, bit, counts[bit],
+						setup_pebs_fixed_sample_data);
 		}
 	}
 }
 
-static inline void __intel_pmu_pebs_event_output(struct perf_event *event,
-						 struct pt_regs *iregs,
-						 void *record, bool last,
-						 struct perf_sample_data *data)
-{
-	struct x86_perf_regs perf_regs;
-	struct pt_regs *regs = &perf_regs.regs;
-	static struct pt_regs dummy_iregs;
-
-	if (!iregs)
-		iregs = &dummy_iregs;
-
-	setup_pebs_adaptive_sample_data(event, iregs, record, data, regs);
-	if (last) {
-		/*
-		 * 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
-		perf_event_output(event, data, regs);
-}
-
 static void 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 *unprocessed[INTEL_PMC_IDX_FIXED + MAX_FIXED_PEBS_EVENTS];
+	void *last[INTEL_PMC_IDX_FIXED + MAX_FIXED_PEBS_EVENTS];
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 	struct debug_store *ds = cpuc->ds;
+	struct x86_perf_regs perf_regs;
+	struct pt_regs *regs = &perf_regs.regs;
 	struct perf_event *event;
 	struct pebs_basic *basic;
 	void *base, *at, *top;
@@ -2452,6 +2458,12 @@ static void intel_pmu_drain_pebs_icl(str
 		return;
 	}
 
+	if (!iregs)
+		iregs = &dummy_iregs;
+
+	/*
+	 * Process all but the last event for each counter.
+	 */
 	for (at = base; at < top; at += basic->format_size) {
 		u64 pebs_status;
 
@@ -2467,18 +2479,12 @@ static void intel_pmu_drain_pebs_icl(str
 			    WARN_ON_ONCE(!event->attr.precise_ip))
 				continue;
 
-			/*
-			 * Need at least one record to call the overflow handler later.
-			 * Initialize the unprocessed[] variable with the first record.
-			 */
-			if (!counts[bit]++) {
-				unprocessed[bit] = at;
-				continue;
+			if (counts[bit]++) {
+				__intel_pmu_pebs_event(event, iregs, regs, data, last[bit],
+						       setup_pebs_adaptive_sample_data)
 			}
 
-			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], false, data);
-
-			unprocessed[bit] = at;
+			last[bit] = at;
 		}
 	}
 
@@ -2487,28 +2493,8 @@ static void intel_pmu_drain_pebs_icl(str
 			continue;
 
 		event = cpuc->events[bit];
-
-		if (!iregs) {
-			/*
-			 * The PEBS records may be drained in the non-overflow context,
-			 * e.g., large PEBS + context switch. Perf should treat the
-			 * last record the same as other PEBS records, and doesn't
-			 * invoke the generic overflow handler.
-			 */
-			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], false, data);
-		} else
-			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], true, data);
-
-		if (event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD) {
-			/*
-			 * Now, auto-reload is only enabled in fixed period mode.
-			 * The reload value is always hwc->sample_period.
-			 * May need to change it, if auto-reload is enabled in
-			 * freq mode later.
-			 */
-			intel_pmu_save_and_restart_reload(event, counts[bit]);
-		} else
-			intel_pmu_save_and_restart(event);
+		__intel_pmu_pebs_last_event(event, iregs, regs, data, last[bit],
+					    counts[bit], setup_pebs_adaptive_sample_data);
 	}
 }
 

Re: [PATCH 2/2] perf/x86/intel/ds: Simplify the PEBS records processing for adaptive PEBS

[Liang, Kan]

On 2024-11-14 9:19 a.m., Peter Zijlstra wrote:
> On Wed, Nov 13, 2024 at 07:14:27AM -0800, kan.liang@linux.intel.com wrote:
> 
>> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
>> index 4d0f7c49295a..cbf2ab9ed4c8 100644
>> --- a/arch/x86/events/intel/ds.c
>> +++ b/arch/x86/events/intel/ds.c
>> @@ -2400,12 +2400,38 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_d
>>  	}
>>  }
>>  
>> +static inline void __intel_pmu_pebs_event_output(struct perf_event *event,
>> +						 struct pt_regs *iregs,
>> +						 void *record, bool last,
>> +						 struct perf_sample_data *data)
>> +{
>> +	struct x86_perf_regs perf_regs;
>> +	struct pt_regs *regs = &perf_regs.regs;
>> +	static struct pt_regs dummy_iregs;
>> +
>> +	if (!iregs)
>> +		iregs = &dummy_iregs;
>> +
>> +	setup_pebs_adaptive_sample_data(event, iregs, record, data, regs);
>> +	if (last) {
>> +		/*
>> +		 * 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
>> +		perf_event_output(event, data, regs);
>> +}
> 
> *sigh*... more unification please.
> 
>>  static void 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 *unprocessed[INTEL_PMC_IDX_FIXED + MAX_FIXED_PEBS_EVENTS];
>>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>>  	struct debug_store *ds = cpuc->ds;
>>  	struct perf_event *event;
>> +	struct pebs_basic *basic;
>>  	void *base, *at, *top;
>>  	int bit;
>>  	u64 mask;
>> @@ -2426,30 +2452,63 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
>>  		return;
>>  	}
>>  
>> -	for (at = base; at < top; at += cpuc->pebs_record_size) {
>> +	for (at = base; at < top; at += basic->format_size) {
>>  		u64 pebs_status;
>>  
>> -		pebs_status = get_pebs_status(at) & cpuc->pebs_enabled;
>> -		pebs_status &= mask;
>> +		basic = at;
>> +		if (WARN_ON_ONCE(basic->format_size != cpuc->pebs_record_size))
>> +			continue;
>> +
>> +		pebs_status = basic->applicable_counters & cpuc->pebs_enabled & mask;
>> +		for_each_set_bit(bit, (unsigned long *)&pebs_status, X86_PMC_IDX_MAX) {
>> +			event = cpuc->events[bit];
>> +
>> +			if (WARN_ON_ONCE(!event) ||
>> +			    WARN_ON_ONCE(!event->attr.precise_ip))
>> +				continue;
>> +
>> +			/*
>> +			 * Need at least one record to call the overflow handler later.
>> +			 * Initialize the unprocessed[] variable with the first record.
>> +			 */
>> +			if (!counts[bit]++) {
>> +				unprocessed[bit] = at;
>> +				continue;
>> +			}
>> +
>> +			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], false, data);
>>  
>> -		for_each_set_bit(bit, (unsigned long *)&pebs_status, X86_PMC_IDX_MAX)
>> -			counts[bit]++;
>> +			unprocessed[bit] = at;
>> +		}
>>  	}
>>  
>>  	for_each_set_bit(bit, (unsigned long *)&mask, X86_PMC_IDX_MAX) {
>> -		if (counts[bit] == 0)
>> +		if (!counts[bit])
>>  			continue;
>>  
>>  		event = cpuc->events[bit];
>> -		if (WARN_ON_ONCE(!event))
>> -			continue;
>>  
>> -		if (WARN_ON_ONCE(!event->attr.precise_ip))
>> -			continue;
>> +		if (!iregs) {
>> +			/*
>> +			 * The PEBS records may be drained in the non-overflow context,
>> +			 * e.g., large PEBS + context switch. Perf should treat the
>> +			 * last record the same as other PEBS records, and doesn't
>> +			 * invoke the generic overflow handler.
>> +			 */
>> +			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], false, data);
>> +		} else
>> +			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], true, data);
> 
> *sigh*, this is confusing as all hell. Both are last, but one says
> last=false.
> 
>> -		__intel_pmu_pebs_event(event, iregs, data, base,
>> -				       top, bit, counts[bit],
>> -				       setup_pebs_adaptive_sample_data);
>> +		if (event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD) {
>> +			/*
>> +			 * Now, auto-reload is only enabled in fixed period mode.
>> +			 * The reload value is always hwc->sample_period.
>> +			 * May need to change it, if auto-reload is enabled in
>> +			 * freq mode later.
>> +			 */
>> +			intel_pmu_save_and_restart_reload(event, counts[bit]);
>> +		} else
>> +			intel_pmu_save_and_restart(event);
> 
> And this is randomly ignoring the return value where previously we would
> abort.

I should give more explains regarding this. Sorry for the confusion.
It's safe to ignore the return value.
The !intel_pmu_save_and_restart() only happens when !hwc->event_base
or the period_left > 0.
- The !hwc->event_base is impossible for the PEBS event
which is only available on GP and fixed counters.
- The period_left should be always <=0. The check only happens for the
non AUTO_RELOAD event and single PEBS. It means that the event must
overflowed. So the period_left should be < 0 after the x86_pmu_update().

> 
>>  	}
>>  }
> 
> How's this completely untested delta?
> 

Thanks. The delta looks good. I will do more tests to double check.
But I probably do two changes as below.

- I will split the patches into two patches.
The first patch is a cleanup patch which factor out the common
__intel_pmu_pebs_events() and __intel_pmu_pebs_last_event(), which
impact all generations. The second patch is to simplify the adaptive
PEBS processing.

- I will move the above intel_pmu_save_and_restart() related codes to
the end where all samples are processed. In the current delta patch,
it's moved to right before processing the last sample.
For the current usage, it doesn't matter when perf updates and reset
event counts. Because all counters stop when draining the PEBS buffer.
However, it does matter for the new counters snapshotting feature.
Because the event counts are from the PEBS records.
https://lore.kernel.org/lkml/20240731143835.771618-4-kan.liang@linux.intel.com/

Thanks,
Kan
> ---
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -2167,24 +2167,33 @@ intel_pmu_save_and_restart_reload(struct
>  	return 0;
>  }
>  
> +void (*setup_fn)(struct perf_event *, struct pt_regs *, void *,
> +		 struct perf_sample_data *, struct pt_regs *);
> +
> +static struct pt_regs dummy_iregs;
> +
>  static __always_inline void
>  __intel_pmu_pebs_event(struct perf_event *event,
>  		       struct pt_regs *iregs,
> -		       struct perf_sample_data *data,
> -		       void *base, void *top,
> -		       int bit, int count,
> -		       void (*setup_sample)(struct perf_event *,
> -					    struct pt_regs *,
> -					    void *,
> -					    struct perf_sample_data *,
> -					    struct pt_regs *))
> +		       struct pt_regs *regs,
> +		       struct perf_event_sample_data *data,
> +		       void *at,
> +		       setup_fn setup_sample)
> +{
> +	setup_sample(event, iregs, at, data, regs);
> +	perf_event_output(event, data, regs);
> +}
> +
> +static __always_inline void
> +__intel_pmu_pebs_last_event(struct perf_event *event,
> +			    struct pt_regs *iregs,
> +			    struct pt_regs *regs,
> +			    struct perf_event_sample_data *data,
> +			    void *at,
> +			    int count,
> +			    setup_fn setup_sample)
>  {
> -	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  	struct hw_perf_event *hwc = &event->hw;
> -	struct x86_perf_regs perf_regs;
> -	struct pt_regs *regs = &perf_regs.regs;
> -	void *at = get_next_pebs_record_by_bit(base, top, bit);
> -	static struct pt_regs dummy_iregs;
>  
>  	if (hwc->flags & PERF_X86_EVENT_AUTO_RELOAD) {
>  		/*
> @@ -2194,21 +2203,12 @@ __intel_pmu_pebs_event(struct perf_event
>  		 * freq mode later.
>  		 */
>  		intel_pmu_save_and_restart_reload(event, count);
> -	} else if (!intel_pmu_save_and_restart(event))
> -		return;
> -
> -	if (!iregs)
> -		iregs = &dummy_iregs;
> -
> -	while (count > 1) {
> -		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--;
> +	} else {
> +		intel_pmu_save_and_restart(event);
>  	}
>  
>  	setup_sample(event, iregs, at, data, regs);
> +
>  	if (iregs == &dummy_iregs) {
>  		/*
>  		 * The PEBS records may be drained in the non-overflow context,
> @@ -2227,6 +2227,34 @@ __intel_pmu_pebs_event(struct perf_event
>  	}
>  }
>  
> +static __always_inline void
> +__intel_pmu_pebs_events(struct perf_event *event,
> +			struct pt_regs *iregs,
> +			struct perf_sample_data *data,
> +			void *base, void *top,
> +			int bit, int count,
> +			setup_fn setup_sample) {
> +{
> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct x86_perf_regs perf_regs;
> +	struct pt_regs *regs = &perf_regs.regs;
> +	void *at = get_next_pebs_record_by_bit(base, top, bit);
> +	int cnt = count;
> +
> +	if (!iregs)
> +		iregs = &dummy_iregs;
> +
> +	while (cnt > 1) {
> +		__intel_pmu_pebs_event(event, iregs, regs, data, at, setup_sample);
> +		at += cpuc->pebs_record_size;
> +		at = get_next_pebs_record_by_bit(at, top, bit);
> +		cnt--;
> +	}
> +
> +	__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)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> @@ -2261,7 +2289,7 @@ static void intel_pmu_drain_pebs_core(st
>  		return;
>  	}
>  
> -	__intel_pmu_pebs_event(event, iregs, data, at, top, 0, n,
> +	__intel_pmu_pebs_events(event, iregs, data, at, top, 0, n,
>  			       setup_pebs_fixed_sample_data);
>  }
>  
> @@ -2393,43 +2421,21 @@ static void intel_pmu_drain_pebs_nhm(str
>  		}
>  
>  		if (counts[bit]) {
> -			__intel_pmu_pebs_event(event, iregs, data, base,
> -					       top, bit, counts[bit],
> -					       setup_pebs_fixed_sample_data);
> +			__intel_pmu_pebs_events(event, iregs, data, base,
> +						top, bit, counts[bit],
> +						setup_pebs_fixed_sample_data);
>  		}>  	}
>  }
>  
> -static inline void __intel_pmu_pebs_event_output(struct perf_event *event,
> -						 struct pt_regs *iregs,
> -						 void *record, bool last,
> -						 struct perf_sample_data *data)
> -{
> -	struct x86_perf_regs perf_regs;
> -	struct pt_regs *regs = &perf_regs.regs;
> -	static struct pt_regs dummy_iregs;
> -
> -	if (!iregs)
> -		iregs = &dummy_iregs;
> -
> -	setup_pebs_adaptive_sample_data(event, iregs, record, data, regs);
> -	if (last) {
> -		/*
> -		 * 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
> -		perf_event_output(event, data, regs);
> -}
> -
>  static void 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 *unprocessed[INTEL_PMC_IDX_FIXED + MAX_FIXED_PEBS_EVENTS];
> +	void *last[INTEL_PMC_IDX_FIXED + MAX_FIXED_PEBS_EVENTS];
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  	struct debug_store *ds = cpuc->ds;
> +	struct x86_perf_regs perf_regs;
> +	struct pt_regs *regs = &perf_regs.regs;
>  	struct perf_event *event;
>  	struct pebs_basic *basic;
>  	void *base, *at, *top;
> @@ -2452,6 +2458,12 @@ static void intel_pmu_drain_pebs_icl(str
>  		return;
>  	}
>  
> +	if (!iregs)
> +		iregs = &dummy_iregs;
> +
> +	/*
> +	 * Process all but the last event for each counter.
> +	 */
>  	for (at = base; at < top; at += basic->format_size) {
>  		u64 pebs_status;
>  
> @@ -2467,18 +2479,12 @@ static void intel_pmu_drain_pebs_icl(str
>  			    WARN_ON_ONCE(!event->attr.precise_ip))
>  				continue;
>  
> -			/*
> -			 * Need at least one record to call the overflow handler later.
> -			 * Initialize the unprocessed[] variable with the first record.
> -			 */
> -			if (!counts[bit]++) {
> -				unprocessed[bit] = at;
> -				continue;
> +			if (counts[bit]++) {
> +				__intel_pmu_pebs_event(event, iregs, regs, data, last[bit],
> +						       setup_pebs_adaptive_sample_data)
>  			}
>  
> -			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], false, data);
> -
> -			unprocessed[bit] = at;
> +			last[bit] = at;
>  		}
>  	}
>  
> @@ -2487,28 +2493,8 @@ static void intel_pmu_drain_pebs_icl(str
>  			continue;
>  
>  		event = cpuc->events[bit];
> -
> -		if (!iregs) {
> -			/*
> -			 * The PEBS records may be drained in the non-overflow context,
> -			 * e.g., large PEBS + context switch. Perf should treat the
> -			 * last record the same as other PEBS records, and doesn't
> -			 * invoke the generic overflow handler.
> -			 */
> -			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], false, data);
> -		} else
> -			__intel_pmu_pebs_event_output(event, iregs, unprocessed[bit], true, data);
> -
> -		if (event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD) {
> -			/*
> -			 * Now, auto-reload is only enabled in fixed period mode.
> -			 * The reload value is always hwc->sample_period.
> -			 * May need to change it, if auto-reload is enabled in
> -			 * freq mode later.
> -			 */
> -			intel_pmu_save_and_restart_reload(event, counts[bit]);
> -		} else
> -			intel_pmu_save_and_restart(event);
> +		__intel_pmu_pebs_last_event(event, iregs, regs, data, last[bit],
> +					    counts[bit], setup_pebs_adaptive_sample_data);
>  	}
>  }
>  
>