perf,x86: Fix shared registers mutual exclusion bug

PATCH Superseded Stephane Eranian 3 participants 4 messages Jun 5, 2013 – Jun 20, 2013

Subsystems (2)

x86 perf events

lore

Share & tools

lore

Stephane Eranian

author

2013-06-05 14:43 UTC #0 lore raw

PATCH 1/1 perf,x86: Fix shared registers mutual exclusion bug

This patch fixes a problem with the shared registers mutual exclusion code and incremental event scheduling by the generic perf_event code.

There was a bug whereby the mutual exclusion on the shared registers was not enforced because of incremental scheduling abort due to event constraints.

Example on Nehalem:
group1= ref-cycles,OFFCORE_RESPONSE_0:PF_RFO
group2= ref-cycles

The ref-cycles event can only be measured by 1 counter. Yet, there are 2 instances here. The first group can be scheduled and is committed. Then, the generic code tries to schedule group2 and this fails (because there is no more counter to support the 2nd instance of ref-cycles).

But in x86_schedule_events() error path, put_event_contraints() is invoked on ALL the events and not just the ones that just failed. That causes the "lock" on the shared offcore_response MSR to be released. Yet the first group is actually scheduled and is exposed to reprogramming of that shared msr by the sibling HT thread (when they are shared by HT threads). In other words, there is no guarantee on what is measured for the offcore_response event.

This patch fixes the problem by tagging committed events with the PERF_X86_EVENT_COMMITTED tag. In the error path of x86_schedule_events(), only the events NOT tagged have their constraint released. The tag is eventually removed when the event in descheduled.

Example was given with offcore_response but also applies to LBR_SELECT and LDLAT shared registers.

Signed-off-by Stephane Eranian <[email protected]>

---
 arch/x86/kernel/cpu/perf_event.c       |   26 +++++++++++++++++++++++++-
 arch/x86/kernel/cpu/perf_event.h       |    9 +++++----
 arch/x86/kernel/cpu/perf_event_intel.c |    2 --
 include/linux/perf_event.h             |    3 ++-
 4 files changed, 32 insertions(+), 8 deletions(-)

arch/x86/kernel/cpu/perf_event.c +25 -1

index 1025f3c..d2ff6b8 100644
@@ -726,6 +726,7 @@ int x86_schedule_events(struct cpu_hw_events *cpuc, int n, int *assign)
 {
 	struct event_constraint *c, *constraints[X86_PMC_IDX_MAX];
 	unsigned long used_mask[BITS_TO_LONGS(X86_PMC_IDX_MAX)];
+	struct perf_event *e;
 	int i, wmin, wmax, num = 0;
 	struct hw_perf_event *hwc;

@@ -767,13 +768,31 @@ int x86_schedule_events(struct cpu_hw_events *cpuc, int n, int *assign)
 		num = perf_assign_events(constraints, n, wmin, wmax, assign);
 
 	/*
+	 * Mark the event as committed, so we do not put_contraint
+	 * in case new events are added and fail scheduling.
+	 */
+	if (!num && assign) {
+		for (i = 0; i < n; i++) {
+			e = cpuc->event_list[i];
+			e->hw.flags |= PERF_X86_EVENT_COMMITTED;
+		}
+	}
+	/*
 	 * scheduling failed or is just a simulation,
 	 * free resources if necessary
 	 */
 	if (!assign || num) {
 		for (i = 0; i < n; i++) {
+			e = cpuc->event_list[i];
+			/*
+			 * do not put_contraint() on comitted events,
+			 * because they are good to go
+			 */
+			if ((e->hw.flags & PERF_X86_EVENT_COMMITTED))
+				continue;
+
 			if (x86_pmu.put_event_constraints)
-				x86_pmu.put_event_constraints(cpuc, cpuc->event_list[i]);
+				x86_pmu.put_event_constraints(cpuc, e);
 		}
 	}
 	return num ? -EINVAL : 0;

@@ -1153,6 +1172,11 @@ static void x86_pmu_del(struct perf_event *event, int flags)
 	int i;
 
 	/*
+	 * event is descheduled
+	 */
+	event->hw.flags &= ~PERF_X86_EVENT_COMMITTED;
+
+	/*
 	 * If we're called during a txn, we don't need to do anything.
 	 * The events never got scheduled and ->cancel_txn will truncate
 	 * the event_list.

arch/x86/kernel/cpu/perf_event.h +5 -4

index ba9aadf..571fadb 100644
@@ -60,13 +60,14 @@ struct event_constraint {
 	u64	cmask;
 	int	weight;
 	int	overlap;
-	int	flags;
+	int	flags; /* hw.flags from constraint */
 };
 /*
- * struct event_constraint flags
+ * struct hw_perf_event.flags flags
  */
-#define PERF_X86_EVENT_PEBS_LDLAT	0x1 /* ld+ldlat data address sampling */
-#define PERF_X86_EVENT_PEBS_ST		0x2 /* st data address sampling */
+#define PERF_X86_EVENT_COMMITTED	0x1 /* event passed commit_txn */
+#define PERF_X86_EVENT_PEBS_LDLAT	0x2 /* ld+ldlat data addr sampling */
+#define PERF_X86_EVENT_PEBS_ST		0x4 /* st data addr sampling */
 
 struct amd_nb {
 	int nb_id;  /* NorthBridge id */

arch/x86/kernel/cpu/perf_event_intel.c +0 -2

index f60d41f..8225c84 100644
@@ -1425,7 +1425,6 @@ x86_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event)
 	if (x86_pmu.event_constraints) {
 		for_each_event_constraint(c, x86_pmu.event_constraints) {
 			if ((event->hw.config & c->cmask) == c->code) {
-				/* hw.flags zeroed at initialization */
 				event->hw.flags |= c->flags;
 				return c;
 			}

@@ -1473,7 +1472,6 @@ intel_put_shared_regs_event_constraints(struct cpu_hw_events *cpuc,
 static void intel_put_event_constraints(struct cpu_hw_events *cpuc,
 					struct perf_event *event)
 {
-	event->hw.flags = 0;
 	intel_put_shared_regs_event_constraints(cpuc, event);
 }

include/linux/perf_event.h +2 -1

index 74a4e14..998b927 100644
@@ -178,7 +178,8 @@ struct perf_event;
 /*
  * Common implementation detail of pmu::{start,commit,cancel}_txn
  */
-#define PERF_EVENT_TXN 0x1
+#define PERF_EVENT_TXN    0x1
+#define PERF_EVENT_COMMIT 0x2 /* event committed */
 
 /**
  * struct pmu - generic performance monitoring unit

Signature · 1 line

1.7.9.5

Peter Zijlstra

maintainer

2013-06-18 09:11 UTC #1 lore raw

Quoted 30 lines · Stephane Eranian

This patch fixes a problem with the shared registers mutual exclusion code and incremental event scheduling by the generic perf_event code.
There was a bug whereby the mutual exclusion on the shared registers was not enforced because of incremental scheduling abort due to event constraints.
Example on Nehalem:
group1= ref-cycles,OFFCORE_RESPONSE_0:PF_RFO
group2= ref-cycles
The ref-cycles event can only be measured by 1 counter. Yet, there are 2 instances here. The first group can be scheduled and is committed. Then, the generic code tries to schedule group2 and this fails (because there is no more counter to support the 2nd instance of ref-cycles).
But in x86_schedule_events() error path, put_event_contraints() is invoked on ALL the events and not just the ones that just failed. That causes the "lock" on the shared offcore_response MSR to be released. Yet the first group is actually scheduled and is exposed to reprogramming of that shared msr by the sibling HT thread (when they are shared by HT threads). In other words, there is no guarantee on what is measured for the offcore_response event.
This patch fixes the problem by tagging committed events with the PERF_X86_EVENT_COMMITTED tag. In the error path of x86_schedule_events(), only the events NOT tagged have their constraint released. The tag is eventually removed when the event in descheduled.

Quoted 4 lines

Example was given with offcore_response but also applies to LBR_SELECT and LDLAT shared registers.
Signed-off-by Stephane Eranian <[email protected]>

I'm getting conflicts against other patches -- most notably I think the contraints stack opt from Andrew Hunter.

I'll try and get Ingo to finally pick up my queued patches so we can rebase.

Stephane Eranian

author

2013-06-18 18:43 UTC #2 lore raw

Quoted 31 lines · Peter Zijlstra

Quoted 30 lines · Stephane Eranian
This patch fixes a problem with the shared registers mutual exclusion code and incremental event scheduling by the generic perf_event code.
There was a bug whereby the mutual exclusion on the shared registers was not enforced because of incremental scheduling abort due to event constraints.
Example on Nehalem:
group1= ref-cycles,OFFCORE_RESPONSE_0:PF_RFO
group2= ref-cycles
The ref-cycles event can only be measured by 1 counter. Yet, there are 2 instances here. The first group can be scheduled and is committed. Then, the generic code tries to schedule group2 and this fails (because there is no more counter to support the 2nd instance of ref-cycles).
But in x86_schedule_events() error path, put_event_contraints() is invoked on ALL the events and not just the ones that just failed. That causes the "lock" on the shared offcore_response MSR to be released. Yet the first group is actually scheduled and is exposed to reprogramming of that shared msr by the sibling HT thread (when they are shared by HT threads). In other words, there is no guarantee on what is measured for the offcore_response event.
This patch fixes the problem by tagging committed events with the PERF_X86_EVENT_COMMITTED tag. In the error path of x86_schedule_events(), only the events NOT tagged have their constraint released. The tag is eventually removed when the event in descheduled.

Quoted 8 lines

Quoted 4 lines
Example was given with offcore_response but also applies to LBR_SELECT and LDLAT shared registers.
Signed-off-by Stephane Eranian <[email protected]>
I'm getting conflicts against other patches -- most notably I think the contraints stack opt from Andrew Hunter.

Yes, that would not surprise me. I wrote this patch without assuming Andrew's patch would be there. But we need to add it. Then we can fix the shared_regs patch.

Quoted 2 lines

I'll try and get Ingo to finally pick up my queued patches so we can rebase.

Ok, thanks.

Quoted 1 line

Ingo Molnar

2013-06-20 12:07 UTC #3 lore raw

Quoted 40 lines · Stephane Eranian

Quoted 31 lines · Peter Zijlstra
Quoted 30 lines · Stephane Eranian
This patch fixes a problem with the shared registers mutual exclusion code and incremental event scheduling by the generic perf_event code.
There was a bug whereby the mutual exclusion on the shared registers was not enforced because of incremental scheduling abort due to event constraints.
Example on Nehalem:
group1= ref-cycles,OFFCORE_RESPONSE_0:PF_RFO
group2= ref-cycles
The ref-cycles event can only be measured by 1 counter. Yet, there are 2 instances here. The first group can be scheduled and is committed. Then, the generic code tries to schedule group2 and this fails (because there is no more counter to support the 2nd instance of ref-cycles).
But in x86_schedule_events() error path, put_event_contraints() is invoked on ALL the events and not just the ones that just failed. That causes the "lock" on the shared offcore_response MSR to be released. Yet the first group is actually scheduled and is exposed to reprogramming of that shared msr by the sibling HT thread (when they are shared by HT threads). In other words, there is no guarantee on what is measured for the offcore_response event.
This patch fixes the problem by tagging committed events with the PERF_X86_EVENT_COMMITTED tag. In the error path of x86_schedule_events(), only the events NOT tagged have their constraint released. The tag is eventually removed when the event in descheduled.
Quoted 8 lines
Quoted 4 lines
Example was given with offcore_response but also applies to LBR_SELECT and LDLAT shared registers.
Signed-off-by Stephane Eranian <[email protected]>
I'm getting conflicts against other patches -- most notably I think the contraints stack opt from Andrew Hunter.

Quoted 8 lines

Yes, that would not surprise me. I wrote this patch without assuming Andrew's patch would be there. But we need to add it. Then we can fix the shared_regs patch.
Quoted 2 lines
I'll try and get Ingo to finally pick up my queued patches so we can rebase.
Ok, thanks.

That happened yesterday, so latest -tip should be a good base to work on.

Thanks,

Ingo