perf/x86/intel: Apply mid ACK for small core

PATCH Applied [email protected] 4 participants 7 messages Aug 3, 2021 – Aug 6, 2021

Subsystems (3)

Stable branch x86 perf events

lore

Share & tools

lore

[email protected]

author

2021-08-03 13:27 UTC #0 lore raw

PATCH v2 1/1 perf/x86/intel: Apply mid ACK for small core

From: Kan Liang <[email protected]>

A warning as below may be occasionally triggered in an ADL machine when these conditions occur,
- Two perf record commands run one by one. Both record a PEBS event.
- Both runs on small cores.
- They have different adaptive PEBS configuration (PEBS_DATA_CFG).

[  673.663291] WARNING: CPU: 4 PID: 9874 at
arch/x86/events/intel/ds.c:1743
setup_pebs_adaptive_sample_data+0x55e/0x5b0
[  673.663348] RIP: 0010:setup_pebs_adaptive_sample_data+0x55e/0x5b0
[  673.663357] Call Trace:
[  673.663357]  <NMI>
[  673.663357]  intel_pmu_drain_pebs_icl+0x48b/0x810
[  673.663360]  perf_event_nmi_handler+0x41/0x80
[  673.663368]  </NMI>
[  673.663370]  __perf_event_task_sched_in+0x2c2/0x3a0

Different from the big core, the small core requires the ACK right before re-enabling counters in the NMI handler, otherwise a stale PEBS record may be dumped into the later NMI handler, which trigger the warning.

Add a new mid_ack flag to track the case. Add all PMI handler bits in the struct x86_hybrid_pmu to track the bits for different types of PMUs. Apply mid ACK for the small cores on an Alder Lake machine.

The existing hybrid() macro has a compile error when taking address of a bit-field variable. Add a new macro hybrid_bit() to get the bit-field value of a given PMU.

Fixes f83d2f91d259 ("perf/x86/intel: Add Alder Lake Hybrid support")

Reported-by Ammy Yi <[email protected]>

Tested-by Ammy Yi <[email protected]>

Reviewed-by Andi Kleen <[email protected]>

Signed-off-by Kan Liang <[email protected]>

Cc [email protected]

---

The V1 patch set can be found at
https://lore.kernel.org/lkml/[email protected]/

Changes since v1:
- Introduce mid ACK. The early ACK in V1 may trigger other issue based on the latest test result.
- Add comments regarding early, mid and late ACK.

 arch/x86/events/intel/core.c | 23 +++++++++++++++--------
 arch/x86/events/perf_event.h | 15 +++++++++++++++
 2 files changed, 30 insertions(+), 8 deletions(-)

arch/x86/events/intel/core.c +15 -8

index d76be3b..511d1f9 100644
@@ -2904,24 +2904,28 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status)
  */
 static int intel_pmu_handle_irq(struct pt_regs *regs)
 {
-	struct cpu_hw_events *cpuc;
+	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
+	bool late_ack = hybrid_bit(cpuc->pmu, late_ack);
+	bool mid_ack = hybrid_bit(cpuc->pmu, mid_ack);
 	int loops;
 	u64 status;
 	int handled;
 	int pmu_enabled;
 
-	cpuc = this_cpu_ptr(&cpu_hw_events);
-
 	/*
 	 * Save the PMU state.
 	 * It needs to be restored when leaving the handler.
 	 */
 	pmu_enabled = cpuc->enabled;
 	/*
-	 * No known reason to not always do late ACK,
-	 * but just in case do it opt-in.
+	 * In general, the early ACK is only applied for old platforms.
+	 * For the big core starts from Haswell, the late ACK should be
+	 * applied.
+	 * For the small core after Tremont, we have to do the ACK right
+	 * before re-enabling counters, which is in the middle of the
+	 * NMI handler.
 	 */
-	if (!x86_pmu.late_ack)
+	if (!late_ack && !mid_ack)
 		apic_write(APIC_LVTPC, APIC_DM_NMI);
 	intel_bts_disable_local();
 	cpuc->enabled = 0;

@@ -2958,6 +2962,8 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
 		goto again;
 
 done:
+	if (mid_ack)
+		apic_write(APIC_LVTPC, APIC_DM_NMI);
 	/* Only restore PMU state when it's active. See x86_pmu_disable(). */
 	cpuc->enabled = pmu_enabled;
 	if (pmu_enabled)

@@ -2969,7 +2975,7 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
 	 * have been reset. This avoids spurious NMIs on
 	 * Haswell CPUs.
 	 */
-	if (x86_pmu.late_ack)
+	if (late_ack)
 		apic_write(APIC_LVTPC, APIC_DM_NMI);
 	return handled;
 }

@@ -6123,7 +6129,6 @@ __init int intel_pmu_init(void)
 		static_branch_enable(&perf_is_hybrid);
 		x86_pmu.num_hybrid_pmus = X86_HYBRID_NUM_PMUS;
 
-		x86_pmu.late_ack = true;
 		x86_pmu.pebs_aliases = NULL;
 		x86_pmu.pebs_prec_dist = true;
 		x86_pmu.pebs_block = true;

@@ -6161,6 +6166,7 @@ __init int intel_pmu_init(void)
 		pmu = &x86_pmu.hybrid_pmu[X86_HYBRID_PMU_CORE_IDX];
 		pmu->name = "cpu_core";
 		pmu->cpu_type = hybrid_big;
+		pmu->late_ack = true;
 		if (cpu_feature_enabled(X86_FEATURE_HYBRID_CPU)) {
 			pmu->num_counters = x86_pmu.num_counters + 2;
 			pmu->num_counters_fixed = x86_pmu.num_counters_fixed + 1;

@@ -6186,6 +6192,7 @@ __init int intel_pmu_init(void)
 		pmu = &x86_pmu.hybrid_pmu[X86_HYBRID_PMU_ATOM_IDX];
 		pmu->name = "cpu_atom";
 		pmu->cpu_type = hybrid_small;
+		pmu->mid_ack = true;
 		pmu->num_counters = x86_pmu.num_counters;
 		pmu->num_counters_fixed = x86_pmu.num_counters_fixed;
 		pmu->max_pebs_events = x86_pmu.max_pebs_events;

arch/x86/events/perf_event.h +15 -0

index ad87cb3..eec7ce8 100644
@@ -655,6 +655,10 @@ struct x86_hybrid_pmu {
 	struct event_constraint		*event_constraints;
 	struct event_constraint		*pebs_constraints;
 	struct extra_reg		*extra_regs;
+
+	unsigned int			late_ack	:1,
+					mid_ack		:1,
+					enabled_ack	:1;
 };
 
 static __always_inline struct x86_hybrid_pmu *hybrid_pmu(struct pmu *pmu)

@@ -685,6 +689,16 @@ extern struct static_key_false perf_is_hybrid;
 	__Fp;						\
 }))
 
+#define hybrid_bit(_pmu, _field)			\
+({							\
+	bool __Fp = x86_pmu._field;			\
+							\
+	if (is_hybrid() && (_pmu))			\
+		__Fp = hybrid_pmu(_pmu)->_field;	\
+							\
+	__Fp;						\
+})
+
 enum hybrid_pmu_type {
 	hybrid_big		= 0x40,
 	hybrid_small		= 0x20,

@@ -754,6 +768,7 @@ struct x86_pmu {
 
 	/* PMI handler bits */
 	unsigned int	late_ack		:1,
+			mid_ack			:1,
 			enabled_ack		:1;
 	/*
 	 * sysfs attrs

Signature · 1 line

2.7.4

Peter Zijlstra

maintainer

2021-08-03 14:55 UTC #1 lore raw

Quoted 19 lines · On Tue, Aug 03, 2021 at 06:25:28AM -0700, [email protected] wrote:

From: Kan Liang <[email protected]>
A warning as below may be occasionally triggered in an ADL machine when these conditions occur,
- Two perf record commands run one by one. Both record a PEBS event.
- Both runs on small cores.
- They have different adaptive PEBS configuration (PEBS_DATA_CFG).
[  673.663291] WARNING: CPU: 4 PID: 9874 at
arch/x86/events/intel/ds.c:1743
setup_pebs_adaptive_sample_data+0x55e/0x5b0
[  673.663348] RIP: 0010:setup_pebs_adaptive_sample_data+0x55e/0x5b0
[  673.663357] Call Trace:
[  673.663357]  <NMI>
[  673.663357]  intel_pmu_drain_pebs_icl+0x48b/0x810
[  673.663360]  perf_event_nmi_handler+0x41/0x80
[  673.663368]  </NMI>
[  673.663370]  __perf_event_task_sched_in+0x2c2/0x3a0

Quoted 8 lines

Different from the big core, the small core requires the ACK right before re-enabling counters in the NMI handler, otherwise a stale PEBS record may be dumped into the later NMI handler, which trigger the warning.
Add a new mid_ack flag to track the case. Add all PMI handler bits in the struct x86_hybrid_pmu to track the bits for different types of PMUs. Apply mid ACK for the small cores on an Alder Lake machine.

Why do we need a new option? Why isn't early (as in not late) good enough?

Liang, Kan

author

2021-08-03 15:20 UTC #2 lore raw

Quoted 25 lines · Peter Zijlstra

Quoted 24 lines · On Tue, Aug 03, 2021 at 06:25:28AM -0700, [email protected] wrote:
From: Kan Liang <[email protected]>
A warning as below may be occasionally triggered in an ADL machine when these conditions occur,
- Two perf record commands run one by one. Both record a PEBS event.
- Both runs on small cores.
- They have different adaptive PEBS configuration (PEBS_DATA_CFG).
[  673.663291] WARNING: CPU: 4 PID: 9874 at
arch/x86/events/intel/ds.c:1743
setup_pebs_adaptive_sample_data+0x55e/0x5b0
[  673.663348] RIP: 0010:setup_pebs_adaptive_sample_data+0x55e/0x5b0
[  673.663357] Call Trace:
[  673.663357]  <NMI>
[  673.663357]  intel_pmu_drain_pebs_icl+0x48b/0x810
[  673.663360]  perf_event_nmi_handler+0x41/0x80
[  673.663368]  </NMI>
[  673.663370]  __perf_event_task_sched_in+0x2c2/0x3a0
Different from the big core, the small core requires the ACK right before re-enabling counters in the NMI handler, otherwise a stale PEBS record may be dumped into the later NMI handler, which trigger the warning.

Quoted 7 lines

Quoted 3 lines
Add a new mid_ack flag to track the case. Add all PMI handler bits in the struct x86_hybrid_pmu to track the bits for different types of PMUs. Apply mid ACK for the small cores on an Alder Lake machine.
Why do we need a new option? Why isn't early (as in not late) good enough?

The early ACK can fix this issue, however it triggers a spurious NMI during the stress test. I'm told to do the ACK right before re-enabling counters for small cores. That indeed fixes all the issues.

Thanks,
Kan

Peter Zijlstra

maintainer

2021-08-03 16:17 UTC #3 lore raw

Quoted 32 lines · Kan

Quoted 29 lines · Peter Zijlstra
Quoted 27 lines · On Tue, Aug 03, 2021 at 06:25:28AM -0700, [email protected] wrote:
From: Kan Liang <[email protected]>
A warning as below may be occasionally triggered in an ADL machine when these conditions occur,
- Two perf record commands run one by one. Both record a PEBS event.
- Both runs on small cores.
- They have different adaptive PEBS configuration (PEBS_DATA_CFG).
[  673.663291] WARNING: CPU: 4 PID: 9874 at
arch/x86/events/intel/ds.c:1743
setup_pebs_adaptive_sample_data+0x55e/0x5b0
[  673.663348] RIP: 0010:setup_pebs_adaptive_sample_data+0x55e/0x5b0
[  673.663357] Call Trace:
[  673.663357]  <NMI>
[  673.663357]  intel_pmu_drain_pebs_icl+0x48b/0x810
[  673.663360]  perf_event_nmi_handler+0x41/0x80
[  673.663368]  </NMI>
[  673.663370]  __perf_event_task_sched_in+0x2c2/0x3a0
Different from the big core, the small core requires the ACK right before re-enabling counters in the NMI handler, otherwise a stale PEBS record may be dumped into the later NMI handler, which trigger the warning.
Add a new mid_ack flag to track the case. Add all PMI handler bits in the struct x86_hybrid_pmu to track the bits for different types of PMUs. Apply mid ACK for the small cores on an Alder Lake machine.

Quoted 7 lines

Quoted 3 lines
Why do we need a new option? Why isn't early (as in not late) good enough?
The early ACK can fix this issue, however it triggers a spurious NMI during the stress test. I'm told to do the ACK right before re-enabling counters for small cores. That indeed fixes all the issues.

Any chance that would also work for the chips that now use late_ack?

I'm just (desperately) trying to minimize the amount of quirks here ;-)

Liang, Kan

author

2021-08-03 17:01 UTC #4 lore raw

Quoted 37 lines · Peter Zijlstra

Quoted 36 lines · Kan
Quoted 32 lines · Peter Zijlstra
Quoted 19 lines · On Tue, Aug 03, 2021 at 06:25:28AM -0700, [email protected] wrote:
From: Kan Liang <[email protected]>
A warning as below may be occasionally triggered in an ADL machine when these conditions occur,
- Two perf record commands run one by one. Both record a PEBS event.
- Both runs on small cores.
- They have different adaptive PEBS configuration (PEBS_DATA_CFG).
[  673.663291] WARNING: CPU: 4 PID: 9874 at
arch/x86/events/intel/ds.c:1743
setup_pebs_adaptive_sample_data+0x55e/0x5b0
[  673.663348] RIP: 0010:setup_pebs_adaptive_sample_data+0x55e/0x5b0
[  673.663357] Call Trace:
[  673.663357]  <NMI>
[  673.663357]  intel_pmu_drain_pebs_icl+0x48b/0x810
[  673.663360]  perf_event_nmi_handler+0x41/0x80
[  673.663368]  </NMI>
[  673.663370]  __perf_event_task_sched_in+0x2c2/0x3a0
Quoted 8 lines
Different from the big core, the small core requires the ACK right before re-enabling counters in the NMI handler, otherwise a stale PEBS record may be dumped into the later NMI handler, which trigger the warning.
Add a new mid_ack flag to track the case. Add all PMI handler bits in the struct x86_hybrid_pmu to track the bits for different types of PMUs. Apply mid ACK for the small cores on an Alder Lake machine.
Why do we need a new option? Why isn't early (as in not late) good enough?

Quoted 6 lines

Quoted 3 lines
The early ACK can fix this issue, however it triggers a spurious NMI during the stress test. I'm told to do the ACK right before re-enabling counters for small cores. That indeed fixes all the issues.
Any chance that would also work for the chips that now use late_ack?

Let me check and do some tests.

Thanks,
Kan

Andi Kleen

2021-08-03 18:00 UTC #5 lore raw

Quoted 5 lines

Quoted 3 lines
Any chance that would also work for the chips that now use late_ack?
Let me check and do some tests.

I suspect we won't be able to test all, so we'll probably have to keep all three options.

-Andi

tip-bot2 for Kan Liang

merge bot Applied

2021-08-06 12:51 UTC #6 lore raw

In reply to [email protected]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     acade6379930dfa7987f4bd9b26d1a701cc1b542
Gitweb:        https://git.kernel.org/tip/acade6379930dfa7987f4bd9b26d1a701cc1b542
Author:        Kan Liang <[email protected]>
AuthorDate:    Tue, 03 Aug 2021 06:25:28 -07:00
Committer:     Peter Zijlstra <[email protected]>
CommitterDate: Fri, 06 Aug 2021 14:25:15 +02:00

perf/x86/intel: Apply mid ACK for small core

A warning as below may be occasionally triggered in an ADL machine when these conditions occur:

- Two perf record commands run one by one. Both record a PEBS event.
- Both runs on small cores.
- They have different adaptive PEBS configuration (PEBS_DATA_CFG).

[ ] WARNING: CPU: 4 PID: 9874 at arch/x86/events/intel/ds.c:1743 setup_pebs_adaptive_sample_data+0x55e/0x5b0 [ ] RIP: 0010:setup_pebs_adaptive_sample_data+0x55e/0x5b0
[ ] Call Trace:

  [ ]  <NMI>
  [ ]  intel_pmu_drain_pebs_icl+0x48b/0x810
  [ ]  perf_event_nmi_handler+0x41/0x80
  [ ]  </NMI>
  [ ]  __perf_event_task_sched_in+0x2c2/0x3a0

The existing hybrid() macro has a compile error when taking address of a bit-field variable. Add a new macro hybrid_bit() to get the bit-field value of a given PMU.

Fixes f83d2f91d259 ("perf/x86/intel: Add Alder Lake Hybrid support")

Reported-by Ammy Yi <[email protected]>

Signed-off-by Kan Liang <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Reviewed-by Andi Kleen <[email protected]>

Tested-by Ammy Yi <[email protected]>

Link https://lkml.kernel.org/r/[email protected] open here

---
 arch/x86/events/intel/core.c | 23 +++++++++++++++--------
 arch/x86/events/perf_event.h | 15 +++++++++++++++
 2 files changed, 30 insertions(+), 8 deletions(-)

arch/x86/events/intel/core.c +15 -8

index fca7a6e..ac6fd2d 100644
@@ -2904,24 +2904,28 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status)
  */
 static int intel_pmu_handle_irq(struct pt_regs *regs)
 {
-	struct cpu_hw_events *cpuc;
+	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
+	bool late_ack = hybrid_bit(cpuc->pmu, late_ack);
+	bool mid_ack = hybrid_bit(cpuc->pmu, mid_ack);
 	int loops;
 	u64 status;
 	int handled;
 	int pmu_enabled;
 
-	cpuc = this_cpu_ptr(&cpu_hw_events);
-
 	/*
 	 * Save the PMU state.
 	 * It needs to be restored when leaving the handler.
 	 */
 	pmu_enabled = cpuc->enabled;
 	/*
-	 * No known reason to not always do late ACK,
-	 * but just in case do it opt-in.
+	 * In general, the early ACK is only applied for old platforms.
+	 * For the big core starts from Haswell, the late ACK should be
+	 * applied.
+	 * For the small core after Tremont, we have to do the ACK right
+	 * before re-enabling counters, which is in the middle of the
+	 * NMI handler.
 	 */
-	if (!x86_pmu.late_ack)
+	if (!late_ack && !mid_ack)
 		apic_write(APIC_LVTPC, APIC_DM_NMI);
 	intel_bts_disable_local();
 	cpuc->enabled = 0;

@@ -2958,6 +2962,8 @@ again:
 		goto again;
 
 done:
+	if (mid_ack)
+		apic_write(APIC_LVTPC, APIC_DM_NMI);
 	/* Only restore PMU state when it's active. See x86_pmu_disable(). */
 	cpuc->enabled = pmu_enabled;
 	if (pmu_enabled)

@@ -2969,7 +2975,7 @@ done:
 	 * have been reset. This avoids spurious NMIs on
 	 * Haswell CPUs.
 	 */
-	if (x86_pmu.late_ack)
+	if (late_ack)
 		apic_write(APIC_LVTPC, APIC_DM_NMI);
 	return handled;
 }

@@ -6129,7 +6135,6 @@ __init int intel_pmu_init(void)
 		static_branch_enable(&perf_is_hybrid);
 		x86_pmu.num_hybrid_pmus = X86_HYBRID_NUM_PMUS;
 
-		x86_pmu.late_ack = true;
 		x86_pmu.pebs_aliases = NULL;
 		x86_pmu.pebs_prec_dist = true;
 		x86_pmu.pebs_block = true;

@@ -6167,6 +6172,7 @@ __init int intel_pmu_init(void)
 		pmu = &x86_pmu.hybrid_pmu[X86_HYBRID_PMU_CORE_IDX];
 		pmu->name = "cpu_core";
 		pmu->cpu_type = hybrid_big;
+		pmu->late_ack = true;
 		if (cpu_feature_enabled(X86_FEATURE_HYBRID_CPU)) {
 			pmu->num_counters = x86_pmu.num_counters + 2;
 			pmu->num_counters_fixed = x86_pmu.num_counters_fixed + 1;

@@ -6192,6 +6198,7 @@ __init int intel_pmu_init(void)
 		pmu = &x86_pmu.hybrid_pmu[X86_HYBRID_PMU_ATOM_IDX];
 		pmu->name = "cpu_atom";
 		pmu->cpu_type = hybrid_small;
+		pmu->mid_ack = true;
 		pmu->num_counters = x86_pmu.num_counters;
 		pmu->num_counters_fixed = x86_pmu.num_counters_fixed;
 		pmu->max_pebs_events = x86_pmu.max_pebs_events;

arch/x86/events/perf_event.h +15 -0

index 2938c90..e3ac05c 100644
@@ -656,6 +656,10 @@ struct x86_hybrid_pmu {
 	struct event_constraint		*event_constraints;
 	struct event_constraint		*pebs_constraints;
 	struct extra_reg		*extra_regs;
+
+	unsigned int			late_ack	:1,
+					mid_ack		:1,
+					enabled_ack	:1;
 };
 
 static __always_inline struct x86_hybrid_pmu *hybrid_pmu(struct pmu *pmu)

@@ -686,6 +690,16 @@ extern struct static_key_false perf_is_hybrid;
 	__Fp;						\
 }))
 
+#define hybrid_bit(_pmu, _field)			\
+({							\
+	bool __Fp = x86_pmu._field;			\
+							\
+	if (is_hybrid() && (_pmu))			\
+		__Fp = hybrid_pmu(_pmu)->_field;	\
+							\
+	__Fp;						\
+})
+
 enum hybrid_pmu_type {
 	hybrid_big		= 0x40,
 	hybrid_small		= 0x20,

@@ -755,6 +769,7 @@ struct x86_pmu {
 
 	/* PMI handler bits */
 	unsigned int	late_ack		:1,
+			mid_ack			:1,
 			enabled_ack		:1;
 	/*
 	 * sysfs attrs