[PATCH v2 1/4] perf-events: Add support for supplementary event registers

Lin Ming

author

2011-03-01 16:48 UTC #0 lore raw

PATCH v2 1/4 perf-events: Add support for supplementary event registers

Change logs against Andi's original version:
- Extends perf_event_attr:config to config{,1,2} (Peter Zijlstra)
- Fixed a major event scheduling issue. There cannot be a ref++ on an event that has already done ref++ once and without calling put_constraint() in between. (Stephane Eranian)
- Use thread_cpumask for percore allocation. (Lin Ming)
- Use MSR names in the extra reg lists. (Lin Ming)

Intel Nehalem/Westmere have a special OFFCORE_RESPONSE event that can be used to monitor any offcore accesses from a core. This is a very useful event for various tunings, and it's also needed to implement the generic LLC-* events correctly.

Unfortunately this event requires programming a mask in a separate register. And worse this separate register is per core, not per CPU thread.

This patch adds:
- Teaches perf_events that OFFCORE_RESPONSE needs extra parameters. The extra parameters are passed by user space in the perf_event_attr::config1 field.
- Add support to the Intel perf_event core to schedule per core resources. This adds fairly generic infrastructure that can be also used for other per core resources.
The basic code has is patterned after the similar AMD northbridge constraints code.

Thanks to Stephane Eranian who pointed out some problems in the original version and suggested improvements.

Cc [email protected]

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

Signed-off-by Lin Ming <[email protected]>

---
 arch/x86/include/asm/msr-index.h       |    3 +
 arch/x86/kernel/cpu/perf_event.c       |   64 ++++++++++
 arch/x86/kernel/cpu/perf_event_intel.c |  201 ++++++++++++++++++++++++++++++++
 include/linux/perf_event.h             |   13 ++-
 4 files changed, 279 insertions(+), 2 deletions(-)

arch/x86/include/asm/msr-index.h +3 -0

index 4d0dfa0..d25e74c 100644
@@ -47,6 +47,9 @@
 #define MSR_IA32_MCG_STATUS		0x0000017a
 #define MSR_IA32_MCG_CTL		0x0000017b
 
+#define MSR_OFFCORE_RSP_0		0x000001a6
+#define MSR_OFFCORE_RSP_1		0x000001a7
+
 #define MSR_IA32_PEBS_ENABLE		0x000003f1
 #define MSR_IA32_DS_AREA		0x00000600
 #define MSR_IA32_PERF_CAPABILITIES	0x00000345

arch/x86/kernel/cpu/perf_event.c +64 -0

106 lines not on this page · raw message

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

271 lines not on this page · raw message

include/linux/perf_event.h +11 -2

index 8ceb5a6..48d966a 100644
@@ -225,8 +225,14 @@ struct perf_event_attr {
 	};
 
 	__u32			bp_type;
-	__u64			bp_addr;
-	__u64			bp_len;
+	union {
+		__u64		bp_addr;
+		__u64		config1; /* extension of config0 */
+	};
+	union {
+		__u64		bp_len;
+		__u64		config2; /* extension of config1 */
+	};
 };
 
 /*

@@ -541,6 +547,9 @@ struct hw_perf_event {
 			unsigned long	event_base;
 			int		idx;
 			int		last_cpu;
+			unsigned int	extra_reg;
+			u64		extra_config;
+			int		extra_alloc;
 		};
 		struct { /* software */
 			struct hrtimer	hrtimer;

Signature · 1 line

1.7.3

Stephane Eranian

2011-03-01 17:09 UTC #1 lore raw

Lin,

See comments below:

Quoted 14 lines · Lin Ming

From: Andi Kleen <[email protected]>
static struct event_constraint *

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

+intel_percore_constraints(struct cpu_hw_events *cpuc, struct perf_event *event)
+{
+       struct hw_perf_event *hwc = &event->hw;
+       unsigned int e = hwc->config & ARCH_PERFMON_EVENTSEL_EVENT;
+       struct event_constraint *c;
+       struct intel_percore *pc;
+       struct er_account *era;
+       int i;
+       int free_slot;
+       int found;
+
+       if (!x86_pmu.percore_constraints || hwc->extra_alloc)

arch/x86/kernel/cpu/perf_event_intel.c intel_bts_constraints(struct perf_event *event) · line 866

Quoted 10 lines

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

+               return NULL;
+
+       for (c = x86_pmu.percore_constraints; c->cmask; c++) {
+               if (e != c->code)
+                       continue;
+
+               /*
+                * Allocate resource per core.
+                */
+               c = NULL;

Not sure I understand this c = NULL, given you hardcoded return NULL outside of the
loop. I forgot to mention this earlier.

Quoted 49 lines

+ pc = cpuc->per_core;
+ if (!pc)
+ break;
+ c = &emptyconstraint;
+ raw_spin_lock(&pc->lock);
+ free_slot = -1;
+ found = 0;
+ for (i = 0; i < MAX_EXTRA_REGS; i++) {
+ era = &pc->regs[i];
+ if (era->ref > 0 && hwc->extra_reg == era->extra_reg) {
+ /* Allow sharing same config */
+ if (hwc->extra_config == era->extra_config) {
+ era->ref++;
+ cpuc->percore_used = 1;
+ hwc->extra_alloc = 1;
+ c = NULL;
+ }
+ /* else conflict */
+ found = 1;
+ break;
+ } else if (era->ref == 0 && free_slot == -1)
+ free_slot = i;
+ }
+ if (!found && free_slot != -1) {
+ era = &pc->regs[free_slot];
+ era->ref = 1;
+ era->extra_reg = hwc->extra_reg;
+ era->extra_config = hwc->extra_config;
+ cpuc->percore_used = 1;
+ hwc->extra_alloc = 1;
+ c = NULL;
+ }
+ raw_spin_unlock(&pc->lock);
+ return c;
+ }
arch/x86/kernel/cpu/perf_event_intel.c +4 -0
+
+       return NULL;
+}
+
include/linux/perf_event.h +0 -2
index 8ceb5a6..48d966a 100644
@@ -225,8 +225,14 @@ struct perf_event_attr {
        };
 
        __u32                   bp_type;
-       __u64                   bp_addr;
-       __u64                   bp_len;

include/linux/perf_event.h struct perf_event_attr { · line 237

Quoted 10 lines

include/linux/perf_event.h +8 -0

+       union {
+               __u64           bp_addr;
+               __u64           config1; /* extension of config0 */
+       };
+       union {
+               __u64           bp_len;
+               __u64           config2; /* extension of config1 */
+       };
  };

I don't see where those config0 or config1 are coming from. ��{.n�+��+%��ݶ��w��{.n�+��{��G��{ay�ʇڙ�,j��f��h��z_�(�階�ݢj"��m��G��?��&��~��iO��z��v�^�m��?�I�

Lin Ming

author

2011-03-02 00:37 UTC #2 lore raw

Quoted 25 lines · Stephane Eranian

Lin,

See comments below:

Quoted 20 lines · Lin Ming

From: Andi Kleen <[email protected]>
static struct event_constraint *

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

+intel_percore_constraints(struct cpu_hw_events *cpuc, struct perf_event *event)
+{
+       struct hw_perf_event *hwc = &event->hw;
+       unsigned int e = hwc->config & ARCH_PERFMON_EVENTSEL_EVENT;
+       struct event_constraint *c;
+       struct intel_percore *pc;
+       struct er_account *era;
+       int i;
+       int free_slot;
+       int found;
+
+       if (!x86_pmu.percore_constraints || hwc->extra_alloc)
+               return NULL;
+
+       for (c = x86_pmu.percore_constraints; c->cmask; c++) {
+               if (e != c->code)
+                       continue;
+

Quoted 8 lines

arch/x86/kernel/cpu/perf_event_intel.c intel_bts_constraints(struct perf_event *event) · line 866
Quoted 4 lines
arch/x86/kernel/cpu/perf_event_intel.c +4 -0
+               /*
+                * Allocate resource per core.
+                */
+               c = NULL;
Not sure I understand this c = NULL, given you hardcoded return NULL outside of the
loop. I forgot to mention this earlier.

Right, that is redundant.
Remove it.

Quoted 47 lines

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

+               pc = cpuc->per_core;
+               if (!pc)
+                       break;
+               c = &emptyconstraint;
+               raw_spin_lock(&pc->lock);
+               free_slot = -1;
+               found = 0;
+               for (i = 0; i < MAX_EXTRA_REGS; i++) {
+                       era = &pc->regs[i];
+                       if (era->ref > 0 && hwc->extra_reg == era->extra_reg) {
+                               /* Allow sharing same config */
+                               if (hwc->extra_config == era->extra_config) {
+                                       era->ref++;
+                                       cpuc->percore_used = 1;
+                                       hwc->extra_alloc = 1;
+                                       c = NULL;
+                               }
+                               /* else conflict */
+                               found = 1;
+                               break;
+                       } else if (era->ref == 0 && free_slot == -1)
+                               free_slot = i;
+               }
+               if (!found && free_slot != -1) {
+                       era = &pc->regs[free_slot];
+                       era->ref = 1;
+                       era->extra_reg = hwc->extra_reg;
+                       era->extra_config = hwc->extra_config;
+                       cpuc->percore_used = 1;
+                       hwc->extra_alloc = 1;
+                       c = NULL;
+               }
+               raw_spin_unlock(&pc->lock);
+               return c;
+       }
+
+       return NULL;
+}
+

include/linux/perf_event.h +0 -0

index 8ceb5a6..48d966a 100644
@@ -225,8 +225,14 @@ struct perf_event_attr {
       };

Quoted 14 lines

include/linux/perf_event.h struct perf_event_attr { · line 237

Quoted 13 lines

include/linux/perf_event.h +8 -2

       __u32                   bp_type;
-       __u64                   bp_addr;
-       __u64                   bp_len;
+       union {
+               __u64           bp_addr;
+               __u64           config1; /* extension of config0 */
+       };
+       union {
+               __u64           bp_len;
+               __u64           config2; /* extension of config1 */
+       };
 };

I don't see where those config0 or config1 are coming from.

config0 means perf_event_attr::config.

Peter suggested to extend the attr config space.
http://marc.info/?l=linux-kernel&m=129838808907525&w=2

Stephane Eranian

2011-03-02 07:48 UTC #3 lore raw

Quoted 100 lines · Lin Ming

Quoted 25 lines · Stephane Eranian
Lin,
See comments below:
Quoted 20 lines · Lin Ming
From: Andi Kleen <[email protected]>
static struct event_constraint *
arch/x86/kernel/cpu/perf_event_intel.c +18 -0
+intel_percore_constraints(struct cpu_hw_events *cpuc, struct perf_event *event)
+{
+       struct hw_perf_event *hwc = &event->hw;
+       unsigned int e = hwc->config & ARCH_PERFMON_EVENTSEL_EVENT;
+       struct event_constraint *c;
+       struct intel_percore *pc;
+       struct er_account *era;
+       int i;
+       int free_slot;
+       int found;
+
+       if (!x86_pmu.percore_constraints || hwc->extra_alloc)
+               return NULL;
+
+       for (c = x86_pmu.percore_constraints; c->cmask; c++) {
+               if (e != c->code)
+                       continue;
+
Quoted 8 lines
Quoted 4 lines
+ /*
+ * Allocate resource per core.
+ */
+ c = NULL;
Not sure I understand this c = NULL, given you hardcoded return NULL outside of the
loop. I forgot to mention this earlier.
Right, that is redundant.
Remove it.
Quoted 47 lines
Quoted 47 lines
+ pc = cpuc->per_core;
+ if (!pc)
+ break;
+ c = &emptyconstraint;
+ raw_spin_lock(&pc->lock);
+ free_slot = -1;
+ found = 0;
+ for (i = 0; i < MAX_EXTRA_REGS; i++) {
+ era = &pc->regs[i];
+ if (era->ref > 0 && hwc->extra_reg == era->extra_reg) {
+ /* Allow sharing same config */
+ if (hwc->extra_config == era->extra_config) {
+ era->ref++;
+ cpuc->percore_used = 1;
+ hwc->extra_alloc = 1;
+ c = NULL;
+ }
+ /* else conflict */
+ found = 1;
+ break;
+ } else if (era->ref == 0 && free_slot == -1)
+ free_slot = i;
+ }
+ if (!found && free_slot != -1) {
+ era = &pc->regs[free_slot];
+ era->ref = 1;
+ era->extra_reg = hwc->extra_reg;
+ era->extra_config = hwc->extra_config;
+ cpuc->percore_used = 1;
+ hwc->extra_alloc = 1;
+ c = NULL;
+ }
+ raw_spin_unlock(&pc->lock);
+ return c;
+ }
arch/x86/kernel/cpu/perf_event_intel.c +4 -0
+
+       return NULL;
+}
+
include/linux/perf_event.h +0 -0
index 8ceb5a6..48d966a 100644
@@ -225,8 +225,14 @@ struct perf_event_attr {
        };
 
Quoted 14 lines
include/linux/perf_event.h struct perf_event_attr { · line 237
Quoted 13 lines
include/linux/perf_event.h +8 -2
        __u32                   bp_type;
-       __u64                   bp_addr;
-       __u64                   bp_len;
+       union {
+               __u64           bp_addr;
+               __u64           config1; /* extension of config0 */
+       };
+       union {
+               __u64           bp_len;
+               __u64           config2; /* extension of config1 */
+       };
  };
 
I don't see where those config0 or config1 are coming from.

Quoted 2 lines

config0 means perf_event_attr::config.

yes.

Quoted 3 lines

Peter suggested to extend the attr config space.
http://marc.info/?l=linux-kernel&m=129838808907525&w=2

Ok, I get it, it is cascading. that's fine. ��{.n�+��+%��ݶ��w��{.n�+��{��G��{ay�ʇڙ�,j��f��h��z_�(�階�ݢj"��m��G��?��&��~��iO��z��v�^�m��?�I�

Peter Zijlstra

2011-03-02 10:32 UTC #4 lore raw

Quoted 15 lines · Stephane Eranian

Quoted 15 lines

Quoted 14 lines

include/linux/perf_event.h struct perf_event_attr { · line 236

Quoted 13 lines

       __u32                   bp_type;

include/linux/perf_event.h +8 -2

-       __u64                   bp_addr;
-       __u64                   bp_len;
+       union {
+               __u64           bp_addr;
+               __u64           config1; /* extension of config0 */
+       };
+       union {
+               __u64           bp_len;
+               __u64           config2; /* extension of config1 */
+       };
 };

I don't see where those config0 or config1 are coming from.

Quoted 8 lines

Quoted 2 lines
config0 means perf_event_attr::config.
yes.
Quoted 3 lines
Peter suggested to extend the attr config space.
http://marc.info/?l=linux-kernel&m=129838808907525&w=2
Ok, I get it, it is cascading. that's fine.

I initially put config and config0 in a union, but that made compilation fail and fixing that up vs the benefit of actually having a config0, which seemed near 0, led me to drop that part. Forgot to update the comment though ;-)

Stephane Eranian

2011-03-02 12:07 UTC #5 lore raw

Quoted 23 lines · Peter Zijlstra

Quoted 22 lines · Stephane Eranian
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
__u32 bp_type;
- __u64 bp_addr;
- __u64 bp_len;
+ union {
+ __u64 bp_addr;
+ __u64 config1; /* extension of config0 */
+ };
+ union {
+ __u64 bp_len;
+ __u64 config2; /* extension of config1 */
+ };
};
Quoted 4 lines
Quoted 1 line
I don't see where those config0 or config1 are coming from.
config0 means perf_event_attr::config.
yes.
Quoted 3 lines
Peter suggested to extend the attr config space.
http://marc.info/?l=linux-kernel&m=129838808907525&w=2

Quoted 7 lines

Quoted 1 line
Ok, I get it, it is cascading. that's fine.
I initially put config and config0 in a union, but that made compilation fail and fixing that up vs the benefit of actually having a config0, which seemed near 0, led me to drop that part. Forgot to update the comment though ;-)

Ok, but that's not committed yet, isn't it?

Peter Zijlstra

2011-03-02 12:27 UTC #6 lore raw

Quoted 6 lines · Stephane Eranian

Quoted 5 lines
I initially put config and config0 in a union, but that made compilation fail and fixing that up vs the benefit of actually having a config0, which seemed near 0, led me to drop that part. Forgot to update the comment though ;-)
Ok, but that's not committed yet, isn't it?

No I dropped all of that when you spotted a problem with the series.