perf_events: update extra shared registers management (v2)
Subsystems (2)
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
---
Peter Zijlstra
maintainerHow 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.
--- 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
@@ -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
Peter Zijlstra
maintainerarch/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];
:-)
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.
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.
Peter Zijlstra
maintainerQuoted 7 lines · Stephane Eranian
There is a major issue as it stands, though. You can
get into an infinite loop bouncing betweenRSP_0andRSP_1in 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 calledintel_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.
Peter Zijlstra
maintainerQuoted 4 lines · Stephane Eranian
Quoted 1 line
Quoted 1 line
event->hw.config&= ~X86_RAW_EVENT_MASK;Not quite, you want
INTEL_ARCH_EVENT_MASKinstead
because you only want to modify umask+event code.
Do EDGE,INV,CMASK actually make a difference for the OFFCORE event?
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_MASKinstead
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.
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 betweenRSP_0andRSP_1in 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 calledintel_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.
Peter Zijlstra
maintainerQuoted 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 betweenRSP_0andRSP_1in 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 calledintel_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 yieldsrsp_1-> ok
ev3 ->rsp_1, conflict, thenrsp_0, but fails, try again ->rsp_1, fails, and so onThe issue is that the
intel_try()function does not know the history of the swaps betweenrsp_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
Peter Zijlstra
maintainerQuoted 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_MASKinstead
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.
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 betweenRSP_0andRSP_1in 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 calledintel_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:0x1008Quoted 7 lines
ev1->
rsp_0
ev2->rsp_0, conflict, then try yieldsrsp_1-> ok
ev3 ->rsp_1, conflict, thenrsp_0, but fails, try again ->rsp_1, fails, and so onThe issue is that the
intel_try()function does not know the history of the swaps betweenrsp_0, rsp1.
Quoted 8 lines
But it does, we pass the initial
reg->idxin, and return false when that matches the new idx, so in your example, ev3 will do:
rsp_1-> conflict,
tryrsp_0-> conflict,
tryrsp_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
Peter Zijlstra
maintainerQuoted 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 toorig_idx.
Indeed, that would clarify things.
Thanks!
[tip:perf/core] perf, intel: Try alternative OFFCORE encodings
In reply to Peter ZijlstraCommit-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.
--- 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
@@ -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