Skip to contentSkip to conversation

perf/x86: fix load latency/precise store data source issues

PATCH In review Stephane Eranian 2 participants 2 messages Aug 5, 2014

Subsystems (2)
x86 perf events
lore
Share & tools
lore

Stephane Eranian

author
#0 lore raw

PATCH 1/1 perf/x86: fix load latency/precise store data source issues

This patch fixes some issues introduced by Andi's 'Revamp PEBS' event selection patch (which is under review right now).

Most of the issues were related to the encoding of the data source, for PEBS events in general and load/store events on Haswell.

This patchd does:
- the default of 0 in perf_sample_data_init() was wrong. 0 is not a valid value. So defined PERF_MEM_NA (not available)

- On HSW, renamed your precise_store_hsw() to datala_hsw() because you are actually processing both loads and stores, except the load latency event which goes thru normal function

- precise_store_data_hsw() was returning bogus data source for store events. dse.mem_lvl instead of dse.val

Signed-off-by Stephane Eranian <[email protected]>
---
 arch/x86/kernel/cpu/perf_event_intel_ds.c | 23 ++++++++++++++---------
 include/linux/perf_event.h                |  9 ++++++++-
 2 files changed, 22 insertions(+), 10 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_ds.c +14 -9
index a9b60f3..1aca254 100644
@@ -108,14 +108,17 @@ static u64 precise_store_data(u64 status)
 	return val;
 }
 
-static u64 precise_store_data_hsw(struct perf_event *event, u64 status)
+static u64 precise_datala_hsw(struct perf_event *event, u64 status)
 {
 	union perf_mem_data_src dse;
 	u64 cfg = event->hw.config & INTEL_ARCH_EVENT_MASK;
 
-	dse.val = 0;
-	dse.mem_op = PERF_MEM_OP_NA;
-	dse.mem_lvl = PERF_MEM_LVL_NA;
+	dse.val = PERF_MEM_NA;
+
+	if (event->hw.flags & PERF_X86_EVENT_PEBS_ST_HSW)
+		dse.mem_op = PERF_MEM_OP_STORE;
+	else if (event->hw.flags & PERF_X86_EVENT_PEBS_LD_HSW)
+		dse.mem_op = PERF_MEM_OP_LOAD;
 
 	/*
 	 * L1 info only valid for following events:

@@ -126,7 +129,7 @@ static u64 precise_store_data_hsw(struct perf_event *event, u64 status)
 	 * MEM_UOPS_RETIRED.ALL_STORES
 	 */
 	if (cfg != 0x12d0 && cfg != 0x22d0 && cfg != 0x42d0 && cfg != 0x82d0)
-		return dse.mem_lvl;
+		return dse.val;
 
 	if (status & 1)
 		dse.mem_lvl = PERF_MEM_LVL_L1 | PERF_MEM_LVL_HIT;

@@ -861,16 +864,18 @@ static void __intel_pmu_pebs_event(struct perf_event *event,
 		 * data.data_src encodes the data source
 		 */
 		if (sample_type & PERF_SAMPLE_DATA_SRC) {
+			u64 val;
 			if (fll)
-				data.data_src.val = load_latency_data(pebs->dse);
+				val = load_latency_data(pebs->dse);
 			else if (event->hw.flags &
 					(PERF_X86_EVENT_PEBS_ST_HSW|
 					 PERF_X86_EVENT_PEBS_LD_HSW|
 					 PERF_X86_EVENT_PEBS_NA_HSW))
-				data.data_src.val =
-					precise_store_data_hsw(event, pebs->dse);
+				val = precise_datala_hsw(event, pebs->dse);
 			else
-				data.data_src.val = precise_store_data(pebs->dse);
+				val = precise_store_data(pebs->dse);
+
+			data.data_src.val = val;
 		}
 	}
include/linux/perf_event.h +8 -1
index 707617a..8b206aa 100644
@@ -604,6 +604,13 @@ struct perf_sample_data {
 	u64				txn;
 };
 
+/* default value for data source */
+#define PERF_MEM_NA (PERF_MEM_S(OP, NA)   |\
+		    PERF_MEM_S(LVL, NA)   |\
+		    PERF_MEM_S(SNOOP, NA) |\
+		    PERF_MEM_S(LOCK, NA)  |\
+		    PERF_MEM_S(TLB, NA))
+
 static inline void perf_sample_data_init(struct perf_sample_data *data,
 					 u64 addr, u64 period)
 {

@@ -616,7 +623,7 @@ static inline void perf_sample_data_init(struct perf_sample_data *data,
 	data->regs_user.regs = NULL;
 	data->stack_user_size = 0;
 	data->weight = 0;
-	data->data_src.val = 0;
+	data->data_src.val = PERF_MEM_NA;
 	data->txn = 0;
 }
Signature · 1 line
1.8.3.2
#1 lore raw
Quoted 11 lines · Stephane Eranian

This patch fixes some issues introduced by Andi's 'Revamp PEBS' event selection patch (which is under review right now).

Most of the issues were related to the encoding of the data source, for PEBS events in general and load/store events on Haswell.

This patchd does:
- the default of 0 in perf_sample_data_init() was wrong. 0 is not a valid value. So defined PERF_MEM_NA (not available)

Looks good.

Quoted 4 lines

- On HSW, renamed your precise_store_hsw() to datala_hsw() because you are actually processing both loads and stores, except the load latency event which goes thru normal function

Pleae don't mix cleanups with bug fixes.

Quoted 3 lines

- precise_store_data_hsw() was returning bogus data source for store events. dse.mem_lvl instead of dse.val

This was already fixed in the second patch (and it wasn't introduced by my patch):

commit 57f6c0e81f5b82b341c4c4ddd621531788c50433

Author: Andi Kleen <[email protected]>
Date:   Fri Jul 18 17:41:48 2014 -0700

perf, x86: Fix haswell mem hierarchy flags reporting

This fixes a bug introduced with

commit 722e76e60f2775c21b087ff12c5e678cf0ebcaaf
Author: Stephane Eranian <[email protected]>
Date: Thu May 15 17:56:44 2014 +0200

fix Haswell precise store data source encoding

When returning early we need to return the complete value of the memory hierarchy, not just the mem_lvl. Otherwise any load/store/na flags set early get lost.

Signed-off-by Andi Kleen <[email protected]>

-Andi