Skip to contentSkip to conversation

perf_events: update extra shared registers management (v2)

PATCH In review Stephane Eranian 3 participants 14 messages May 20, 2011 – Jul 1, 2011

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

Stephane Eranian

author
#0 lore raw

cover perf_events: update extra shared registers management (v2)

The following short series of patches improves the code which manages the extra shared regs used by some events on Intel processors. Those events require an extra MSR which may be shared between siblings CPUs when HT is on. When HT is off, the kernel still needs to ensure that events within an event group do not try to program different values into that extra MSR.

This series improves the current code for managing the register sharing by using static allocation instead of dynamically trying to find a table slot to host that extra MSR. This greatly simplifies the code. The patch also prepare the kernel for more registers with those kinds of constraints (e.g, LBR_SELECT, LD_LAT).

The patch also adds the missing group validation of events using those extra MSRs. Up until now, one could put two instances of the those events which had incompatible values for the extra MSR. There was no upfront check and the group would never be scheduled. Now, such group cannot be constructed anymore (fail early).

Finally, the third patch adds the SandyBridge support for the offcore_response events (which use these shared MSR). It also removes the offcore_response events from the SandyBridge constraint event table. Those events don't have any constraints contrary to what's published in the documentation.

The second version updates PATCH 1/3 which was an older version with reg->idx initialization problems.

[PATCH 0/3] introduction
[PATCH 1/3] rework of the register sharing logic [PATCH 2/3] add missing shared regs validation [PATCH 3/3] add Intel SandyBridge offcore_response support

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

---

maintainer
#1 lore raw

How about something like the below on top of your patches?

---
Subject: perf, intel: Try alternative OFFCORE encoding
From: Peter Zijlstra <[email protected]>
Date: Mon May 23 11:08:15 CEST 2011

Since the OFFCORE registers are fully symmetric, try the other when the speficied one is already taken.

Signed-off-by Peter Zijlstra <[email protected]>
---
 arch/x86/kernel/cpu/perf_event.c       |    5 ++-
 arch/x86/kernel/cpu/perf_event_intel.c |   45 ++++++++++++++++++++++++++-------
 2 files changed, 40 insertions(+), 10 deletions(-)
arch/x86/kernel/cpu/perf_event.c +4 -1
Index: linux-2.6/arch/x86/kernel/cpu/perf_event.c
===================================================================
@@ -326,9 +326,12 @@ struct x86_pmu {
 	 * Extra registers for events
 	 */
 	struct extra_reg *extra_regs;
-	bool regs_no_ht_sharing;
+	unsigned int er_flags;
 };
 
+#define ERF_NO_HT_SHARING	1
+#define ERF_HAS_RSP_1		2
+
 static struct x86_pmu x86_pmu __read_mostly;
 
 static DEFINE_PER_CPU(struct cpu_hw_events, cpu_hw_events) = {
arch/x86/kernel/cpu/perf_event_intel.c +36 -9

100 lines not on this page · raw message

maintainer
#2 lore raw

arch/x86/kernel/cpu/perf_event_intel.c intel_bts_constraints(struct perf_event · line 1035

Quoted 9 lines · Peter Zijlstra
arch/x86/kernel/cpu/perf_event_intel.c +9 -0
+       if (event->hw.extra_reg.idx == EXTRA_REG_RSP_0) {
+               event->attr.config = 0x01bb;
+               event->hw.extra_reg.idx = EXTRA_REG_RSP_1;
+               event->hw.extra_reg.msr = MSR_OFFCORE_RSP_1;
+       } else if (event->hw.extra_reg.idx == EXTRA_REG_RSP_1) {
+               event->attr.config = 0x01b7;
+               event->hw.extra_reg.idx = EXTRA_REG_RSP_0;
+               event->hw.extra_reg.msr = MSR_OFFCORE_RSP_0;
+       }

clearly I meant to write:

event->hw.config &= ~X86_RAW_EVENT_MASK;
event->hw.config |= 0x01b[b7];

:-)

author
#3 lore raw
Quoted 11 lines · Peter Zijlstra
Quoted 9 lines · Peter Zijlstra

+       if (event->hw.extra_reg.idx == EXTRA_REG_RSP_0) {
+               event->attr.config = 0x01bb;
+               event->hw.extra_reg.idx = EXTRA_REG_RSP_1;
+               event->hw.extra_reg.msr = MSR_OFFCORE_RSP_1;
+       } else if (event->hw.extra_reg.idx == EXTRA_REG_RSP_1) {
+               event->attr.config = 0x01b7;
+               event->hw.extra_reg.idx = EXTRA_REG_RSP_0;
+               event->hw.extra_reg.msr = MSR_OFFCORE_RSP_0;
+       }

Quoted 5 lines

clearly I meant to write:

event->hw.config &= ~X86_RAW_EVENT_MASK;
event->hw.config |= 0x01b[b7];

Yes, to preserve the other bits.
This patch looks good, I will test it some more.
Thanks.

author
#4 lore raw
Quoted 12 lines · Stephane Eranian
Quoted 11 lines · Peter Zijlstra
Quoted 9 lines · Peter Zijlstra

+       if (event->hw.extra_reg.idx == EXTRA_REG_RSP_0) {
+               event->attr.config = 0x01bb;
+               event->hw.extra_reg.idx = EXTRA_REG_RSP_1;
+               event->hw.extra_reg.msr = MSR_OFFCORE_RSP_1;
+       } else if (event->hw.extra_reg.idx == EXTRA_REG_RSP_1) {
+               event->attr.config = 0x01b7;
+               event->hw.extra_reg.idx = EXTRA_REG_RSP_0;
+               event->hw.extra_reg.msr = MSR_OFFCORE_RSP_0;
+       }

Quoted 3 lines
Quoted 3 lines

clearly I meant to write:

event->hw.config &= ~X86_RAW_EVENT_MASK;

Not quite, you want INTEL_ARCH_EVENT_MASK instead
because you only want to modify umask+event code.

There is a major issue as it stands, though. You can
get into an infinite loop bouncing between RSP_0 and RSP_1 in case there is no solution in the group, i.e., you have 3 values for the extra MSR. I think you need to count the number of times you've called intel_try_alt_er() with success or maintain some sort of bitmask of possible alternate choices and when you exhaust that, you simply fail.

maintainer
#5 lore raw
Quoted 7 lines · Stephane Eranian

There is a major issue as it stands, though. You can
get into an infinite loop bouncing between RSP_0 and RSP_1 in case there is no solution in the group, i.e., you have 3 values for the extra MSR. I think you need to count the number of times you've called intel_try_alt_er() with success or maintain some sort of bitmask of possible alternate choices and when you exhaust that, you simply fail.

That should be sorted by the compare with the initial idx value, no? Once its back where it started out it'll bail.

author
#6 lore raw
Quoted 9 lines · Peter Zijlstra
Quoted 7 lines · Stephane Eranian

There is a major issue as it stands, though. You can
get into an infinite loop bouncing between RSP_0 and RSP_1 in case there is no solution in the group, i.e., you have 3 values for the extra MSR. I think you need to count the number of times you've called intel_try_alt_er() with success or maintain some sort of bitmask of possible alternate choices and when you exhaust that, you simply fail.

Quoted 3 lines

That should be sorted by the compare with the initial idx value, no? Once its back where it started out it'll bail.

Nope.

Take:
- ev1=rsp_0:0x1001
- ev2=rsp_0:0x1002
- ev3=rsp_1:0x1008

ev1-> rsp_0
ev2-> rsp_0, conflict, then try yields rsp_1 -> ok
ev3 -> rsp_1, conflict, then rsp_0, but fails, try again -> rsp_1, fails, and so on

The issue is that the intel_try() function does not know the history of the swaps between rsp_0, rsp1.

maintainer
#7 lore raw
Quoted 20 lines · Stephane Eranian
Quoted 9 lines · Peter Zijlstra
Quoted 7 lines · Stephane Eranian

There is a major issue as it stands, though. You can
get into an infinite loop bouncing between RSP_0 and RSP_1 in case there is no solution in the group, i.e., you have 3 values for the extra MSR. I think you need to count the number of times you've called intel_try_alt_er() with success or maintain some sort of bitmask of possible alternate choices and when you exhaust that, you simply fail.

Quoted 3 lines

That should be sorted by the compare with the initial idx value, no? Once its back where it started out it'll bail.

Nope.

Take:
- ev1=rsp_0:0x1001
- ev2=rsp_0:0x1002
- ev3=rsp_1:0x1008

Quoted 7 lines

ev1-> rsp_0
ev2-> rsp_0, conflict, then try yields rsp_1 -> ok
ev3 -> rsp_1, conflict, then rsp_0, but fails, try again -> rsp_1, fails, and so on

The issue is that the intel_try() function does not know the history of the swaps between rsp_0, rsp1.

But it does, we pass the initial reg->idx in, and return false when that matches the new idx, so in your example, ev3 will do:

     rsp_1 -> conflict,

try rsp_0 -> conflict,
try rsp_1 -> bail, return emptyconstraint

author
#8 lore raw
Quoted 29 lines · Peter Zijlstra
Quoted 20 lines · Stephane Eranian
Quoted 9 lines · Peter Zijlstra
Quoted 7 lines · Stephane Eranian

There is a major issue as it stands, though. You can
get into an infinite loop bouncing between RSP_0 and RSP_1 in case there is no solution in the group, i.e., you have 3 values for the extra MSR. I think you need to count the number of times you've called intel_try_alt_er() with success or maintain some sort of bitmask of possible alternate choices and when you exhaust that, you simply fail.

Quoted 3 lines

That should be sorted by the compare with the initial idx value, no? Once its back where it started out it'll bail.

Nope.

Take:
  - ev1=rsp_0:0x1001
  - ev2=rsp_0:0x1002
  - ev3=rsp_1:0x1008

Quoted 7 lines

ev1-> rsp_0
ev2-> rsp_0, conflict, then try yields rsp_1 -> ok
ev3 -> rsp_1, conflict, then rsp_0, but fails, try again -> rsp_1, fails, and so on

The issue is that the intel_try() function does not know the history of the swaps between rsp_0, rsp1.

Quoted 8 lines

But it does, we pass the initial reg->idx in, and return false when that matches the new idx, so in your example, ev3 will do:

    rsp_1 -> conflict,
 try rsp_0 -> conflict,
 try rsp_1 -> bail, return emptyconstraint

Ok, my fault. That's because I tweaked your patch a bit and I did not keep track of idx. I guess the logic is not obvious. I think you should rename idx to orig_idx.

Quoted 1 line
maintainer
#9 lore raw
Quoted 2 lines · Stephane Eranian

Ok, my fault. That's because I tweaked your patch a bit and I did not keep track of idx.

AH! Ok, at least its not my brain that's broken, was starting to doubt my sanity there for a moment ;-)

Quoted 2 lines

I guess the logic is not obvious.
I think you should rename idx to orig_idx.

Indeed, that would clarify things.

Thanks!

maintainer
#10 lore raw
In reply to Stephane Eranian
Quoted 4 lines · Stephane Eranian
Quoted 1 line
Quoted 1 line

event->hw.config &= ~X86_RAW_EVENT_MASK;

Not quite, you want INTEL_ARCH_EVENT_MASK instead
because you only want to modify umask+event code.

Do EDGE,INV,CMASK actually make a difference for the OFFCORE event?

author
#11 lore raw
Quoted 8 lines · Peter Zijlstra
Quoted 4 lines · Stephane Eranian
Quoted 1 line
Quoted 1 line

event->hw.config &= ~X86_RAW_EVENT_MASK;

Not quite, you want INTEL_ARCH_EVENT_MASK instead
because you only want to modify umask+event code.

Do EDGE,INV,CMASK actually make a difference for the OFFCORE event?

The effect of those filters is independent of the event.

maintainer
#12 lore raw
Quoted 10 lines · Stephane Eranian
Quoted 8 lines · Peter Zijlstra
Quoted 4 lines · Stephane Eranian
Quoted 1 line
Quoted 1 line

event->hw.config &= ~X86_RAW_EVENT_MASK;

Not quite, you want INTEL_ARCH_EVENT_MASK instead
because you only want to modify umask+event code.

Do EDGE,INV,CMASK actually make a difference for the OFFCORE event?

The effect of those filters is independent of the event.

OK.

#13 lore raw

[tip:perf/core] perf, intel: Try alternative OFFCORE encodings

In reply to Peter Zijlstra
Commit-ID:  b79e8941fb9af07d810da91b4e29da2bba331b6e
Gitweb:     http://git.kernel.org/tip/b79e8941fb9af07d810da91b4e29da2bba331b6e
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Mon, 23 May 2011 11:08:15 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 1 Jul 2011 11:06:37 +0200

perf, intel: Try alternative OFFCORE encodings

Since the OFFCORE registers are fully symmetric, try the other one when the specified one is already in use.

Signed-off-by Peter Zijlstra <[email protected]>
Link http://lkml.kernel.org/r/1306141897.18455.8.camel@twins open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/kernel/cpu/perf_event.c       |    5 +++-
 arch/x86/kernel/cpu/perf_event_intel.c |   44 ++++++++++++++++++++++++++-----
 2 files changed, 41 insertions(+), 8 deletions(-)
arch/x86/kernel/cpu/perf_event.c +4 -1
index 583f311..c53d433 100644
@@ -327,9 +327,12 @@ struct x86_pmu {
 	 * Extra registers for events
 	 */
 	struct extra_reg *extra_regs;
-	bool regs_no_ht_sharing;
+	unsigned int er_flags;
 };
 
+#define ERF_NO_HT_SHARING	1
+#define ERF_HAS_RSP_1		2
+
 static struct x86_pmu x86_pmu __read_mostly;
 
 static DEFINE_PER_CPU(struct cpu_hw_events, cpu_hw_events) = {
arch/x86/kernel/cpu/perf_event_intel.c +37 -7

96 lines not on this page · raw message