Skip to contentSkip to conversation

perf_events: fix validation of events using an extra reg (v4)

PATCH In review Stephane Eranian 2 participants 7 messages Jun 6, 2011 – Jul 1, 2011

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

Stephane Eranian

author
#0 lore raw

PATCH 2/3 perf_events: fix validation of events using an extra reg (v4)

The validate_group() function needs to validate events with extra shared regs. Within an event group, only events with the same value for the extra reg can co-exist. This was not checked by validate_group() because it was missing the shared_regs logic.

This patch changes the allocation of the fake cpuc used for validation to also point to a fake shared_regs structure such that group events be properly testing.

It modifies __intel_shared_reg_get_constraints() to use spin_lock_irqsave() to avoid lockdep issues.

The fourth version fixes a compilation problem when Intel processors are not supported (CONFIG_CPU_SUP_INTEL). Thanks to Peter Zijsltra for reporting the bug and suggesting the fix.

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

--

arch/x86/kernel/cpu/perf_event.c +45 -14
index 0f8d3ff..ed6af75 100644
@@ -1682,6 +1682,40 @@ static int x86_pmu_commit_txn(struct pmu *pmu)
 	perf_pmu_enable(pmu);
 	return 0;
 }
+/*
+ * a fake_cpuc is used to validate event groups. Due to
+ * the extra reg logic, we need to also allocate a fake
+ * per_core and per_cpu structure. Otherwise, group events
+ * using extra reg may conflict without the kernel being
+ * able to catch this when the last event gets added to
+ * the group.
+ */
+static void free_fake_cpuc(struct cpu_hw_events *cpuc)
+{
+	kfree(cpuc->shared_regs);
+	kfree(cpuc);
+}
+
+static struct cpu_hw_events *allocate_fake_cpuc(void)
+{
+	struct cpu_hw_events *cpuc;
+	int cpu = smp_processor_id();
+
+	cpuc = kzalloc(sizeof(*cpuc), GFP_KERNEL);
+	if (!cpuc)
+		return ERR_PTR(-ENOMEM);
+
+	/* only needed, if we have extra_regs */
+	if (x86_pmu.extra_regs) {
+		cpuc->shared_regs = allocate_shared_regs(cpu);
+		if (!cpuc->shared_regs)
+			goto error;
+	}
+	return cpuc;
+error:
+	free_fake_cpuc(cpuc);
+	return ERR_PTR(-ENOMEM);
+}
 
 /*
  * validate that we can schedule this event

@@ -1692,9 +1726,9 @@ static int validate_event(struct perf_event *event)
 	struct event_constraint *c;
 	int ret = 0;
 
-	fake_cpuc = kmalloc(sizeof(*fake_cpuc), GFP_KERNEL | __GFP_ZERO);
-	if (!fake_cpuc)
-		return -ENOMEM;
+	fake_cpuc = allocate_fake_cpuc();
+	if (IS_ERR(fake_cpuc))
+		return PTR_ERR(fake_cpuc);
 
 	c = x86_pmu.get_event_constraints(fake_cpuc, event);
 

@@ -1704,7 +1738,7 @@ static int validate_event(struct perf_event *event)
 	if (x86_pmu.put_event_constraints)
 		x86_pmu.put_event_constraints(fake_cpuc, event);
 
-	kfree(fake_cpuc);
+	free_fake_cpuc(fake_cpuc);
 
 	return ret;
 }

@@ -1724,35 +1758,32 @@ static int validate_group(struct perf_event *event)
 {
 	struct perf_event *leader = event->group_leader;
 	struct cpu_hw_events *fake_cpuc;
-	int ret, n;
+	int ret = -ENOSPC, n;
 
-	ret = -ENOMEM;
-	fake_cpuc = kmalloc(sizeof(*fake_cpuc), GFP_KERNEL | __GFP_ZERO);
-	if (!fake_cpuc)
-		goto out;
+	fake_cpuc = allocate_fake_cpuc();
+	if (IS_ERR(fake_cpuc))
+		return PTR_ERR(fake_cpuc);
 	/*
 	 * the event is not yet connected with its
 	 * siblings therefore we must first collect
 	 * existing siblings, then add the new event
 	 * before we can simulate the scheduling
 	 */
-	ret = -ENOSPC;
 	n = collect_events(fake_cpuc, leader, true);
 	if (n < 0)
-		goto out_free;
+		goto out;
 
 	fake_cpuc->n_events = n;
 	n = collect_events(fake_cpuc, event, false);
 	if (n < 0)
-		goto out_free;
+		goto out;
 
 	fake_cpuc->n_events = n;
 
 	ret = x86_pmu.schedule_events(fake_cpuc, n, NULL);
 
-out_free:
-	kfree(fake_cpuc);
 out:
+	free_fake_cpuc(fake_cpuc);
 	return ret;
 }
arch/x86/kernel/cpu/perf_event_intel.c +12 -4
index 2d40f33..69afaad 100644
@@ -1027,14 +1027,18 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
 {
 	struct event_constraint *c = &emptyconstraint;
 	struct er_account *era;
+	unsigned long flags;
 
 	/* already allocated shared msr */
-	if (reg->alloc || !cpuc->shared_regs)
+	if (reg->alloc)
 		return &unconstrained;
 
 	era = &cpuc->shared_regs->regs[reg->idx];
-
-	raw_spin_lock(&era->lock);
+	/*
+	 * we use spin_lock_irqsave() to avoid lockdep issues when
+	 * passing a fake cpuc
+	 */
+	raw_spin_lock_irqsave(&era->lock, flags);
 
 	if (!atomic_read(&era->ref) || era->config == reg->config) {
 

@@ -1058,7 +1062,7 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
 		 */
 		c = &unconstrained;
 	}
-	raw_spin_unlock(&era->lock);
+	raw_spin_unlock_irqrestore(&era->lock, flags);
 
 	return c;
 }

@@ -1524,4 +1528,8 @@ static int intel_pmu_init(void)
 	return 0;
 }
 
+static struct intel_shared_regs *allocate_shared_regs(int cpu)
+{
+	return NULL;
+}
 #endif /* CONFIG_CPU_SUP_INTEL */
maintainer
#1 lore raw

arch/x86/kernel/cpu/perf_event.c static int x86_pmu_commit_txn(struct pmu *pmu) · line 1702

Quoted 4 lines · Stephane Eranian
arch/x86/kernel/cpu/perf_event.c +4 -0
+static struct cpu_hw_events *allocate_fake_cpuc(void)
+{
+       struct cpu_hw_events *cpuc;
+       int cpu = smp_processor_id();

That's a boo-boo, clearly we are in a preemptible context here (see the GFP_KERNEL allocation on the next line), so using smp_processor_id() isn't valid.

Now since all that allocate_shared_regs() does with it is pick a NUMA node, we should probably use raw_smp_processor_id() and leave it at that, right?

Quoted 15 lines
arch/x86/kernel/cpu/perf_event.c +15 -0
+       cpuc = kzalloc(sizeof(*cpuc), GFP_KERNEL);
+       if (!cpuc)
+               return ERR_PTR(-ENOMEM);
+
+       /* only needed, if we have extra_regs */
+       if (x86_pmu.extra_regs) {
+               cpuc->shared_regs = allocate_shared_regs(cpu);
+               if (!cpuc->shared_regs)
+                       goto error;
+       }
+       return cpuc;
+error:
+       free_fake_cpuc(cpuc);
+       return ERR_PTR(-ENOMEM);
+}
author
#2 lore raw
Quoted 10 lines · Peter Zijlstra

arch/x86/kernel/cpu/perf_event.c static int x86_pmu_commit_txn(struct pmu *pmu) · line 1702

Quoted 4 lines · Stephane Eranian
arch/x86/kernel/cpu/perf_event.c +4 -0
+static struct cpu_hw_events *allocate_fake_cpuc(void)
+{
+       struct cpu_hw_events *cpuc;
+       int cpu = smp_processor_id();

That's a boo-boo, clearly we are in a preemptible context here (see the GFP_KERNEL allocation on the next line), so using smp_processor_id() isn't valid.

Good point. I missed that.

Quoted 4 lines

Now since all that allocate_shared_regs() does with it is pick a NUMA node, we should probably use raw_smp_processor_id() and leave it at that, right?

Yeah, for what we do with fake_cpuc, it does not really matter where it comes from. This is not on any critical path. The simplest allocator will do it.

Quoted 17 lines
Quoted 15 lines

+       cpuc = kzalloc(sizeof(*cpuc), GFP_KERNEL);
+       if (!cpuc)
+               return ERR_PTR(-ENOMEM);

arch/x86/kernel/cpu/perf_event.c +12 -0
+
+       /* only needed, if we have extra_regs */
+       if (x86_pmu.extra_regs) {
+               cpuc->shared_regs = allocate_shared_regs(cpu);
+               if (!cpuc->shared_regs)
+                       goto error;
+       }
+       return cpuc;
+error:
+       free_fake_cpuc(cpuc);
+       return ERR_PTR(-ENOMEM);
+}
maintainer
#3 lore raw
Quoted 11 lines · Stephane Eranian
Quoted 10 lines · Peter Zijlstra

arch/x86/kernel/cpu/perf_event.c static int x86_pmu_commit_txn(struct pmu *pmu) · line 1702

Quoted 4 lines · Stephane Eranian
arch/x86/kernel/cpu/perf_event.c +4 -0
+static struct cpu_hw_events *allocate_fake_cpuc(void)
+{
+       struct cpu_hw_events *cpuc;
+       int cpu = smp_processor_id();

That's a boo-boo, clearly we are in a preemptible context here (see the GFP_KERNEL allocation on the next line), so using smp_processor_id() isn't valid.

Quoted 1 line

Good point. I missed that.

Yeah, I did too, Ingo found it during testing.

Quoted 7 lines
Quoted 4 lines

Now since all that allocate_shared_regs() does with it is pick a NUMA node, we should probably use raw_smp_processor_id() and leave it at that, right?

Yeah, for what we do with fake_cpuc, it does not really matter where it comes from. This is not on any critical path. The simplest allocator will do it.

OK, fixed that up.

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

arch/x86/kernel/cpu/perf_event.c static int x86_pmu_commit_txn(struct pmu *pmu) · line 1702

Quoted 4 lines · Stephane Eranian
arch/x86/kernel/cpu/perf_event.c +4 -0
+static struct cpu_hw_events *allocate_fake_cpuc(void)
+{
+       struct cpu_hw_events *cpuc;
+       int cpu = smp_processor_id();

That's a boo-boo, clearly we are in a preemptible context here (see the GFP_KERNEL allocation on the next line), so using smp_processor_id() isn't valid.

Quoted 8 lines
Quoted 1 line

Good point. I missed that.

Yeah, I did too, Ingo found it during testing.

Quoted 4 lines
Quoted 4 lines

Now since all that allocate_shared_regs() does with it is pick a NUMA node, we should probably use raw_smp_processor_id() and leave it at that, right?

Looked at that some more. It is more subtle than this. allocate_shared_regs() is used in two places:
- allocate_fake_cpuc()
- intel_pmu_cpu_prepare()

In the first case, it does not matter where the cpuc is allocated, it's not on any
critical path. But for the other situation, it'd better be allocated on the cpu node.
But I think that is what we get given it is called during the CPU hotplug prepare
path, so it must be running on the CPU to prepare and thus a kzalloc() should allocate on the right node, right?

Quoted 7 lines
Quoted 3 lines

Yeah, for what we do with fake_cpuc, it does not really matter where it comes from. This is not on any critical path. The simplest allocator will do it.

OK, fixed that up.

maintainer
#5 lore raw
Quoted 26 lines · Stephane Eranian
Quoted 12 lines · Peter Zijlstra
Quoted 11 lines · Stephane Eranian
Quoted 10 lines · Peter Zijlstra

arch/x86/kernel/cpu/perf_event.c static int x86_pmu_commit_txn(struct pmu *pmu) · line 1702

Quoted 4 lines · Stephane Eranian
arch/x86/kernel/cpu/perf_event.c +4 -0
+static struct cpu_hw_events *allocate_fake_cpuc(void)
+{
+       struct cpu_hw_events *cpuc;
+       int cpu = smp_processor_id();

That's a boo-boo, clearly we are in a preemptible context here (see the GFP_KERNEL allocation on the next line), so using smp_processor_id() isn't valid.

Quoted 8 lines
Quoted 1 line

Good point. I missed that.

Yeah, I did too, Ingo found it during testing.

Quoted 4 lines
Quoted 4 lines

Now since all that allocate_shared_regs() does with it is pick a NUMA node, we should probably use raw_smp_processor_id() and leave it at that, right?

Looked at that some more. It is more subtle than this. allocate_shared_regs() is used in two places:
- allocate_fake_cpuc()
- intel_pmu_cpu_prepare()

Quoted 8 lines

In the first case, it does not matter where the cpuc is allocated, it's not on any
critical path. But for the other situation, it'd better be allocated on the cpu node.
But I think that is what we get given it is called during the CPU hotplug prepare
path, so it must be running on the CPU to prepare and thus a kzalloc() should allocate on the right node, right?

Yeah. Let me shove these patches mingo wards again.

author
#6 lore raw

[tip:perf/core] perf_events: Fix validation of events using an extra reg

In reply to Stephane Eranian
Commit-ID:  cd8a38d33e2528998998bae70a45ad27e442f114
Gitweb:     http://git.kernel.org/tip/cd8a38d33e2528998998bae70a45ad27e442f114
Author:     Stephane Eranian <[email protected]>
AuthorDate: Mon, 6 Jun 2011 16:57:08 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 1 Jul 2011 11:06:36 +0200

perf_events: Fix validation of events using an extra reg

The validate_group() function needs to validate events with extra shared regs. Within an event group, only events with the same value for the extra reg can co-exist. This was not checked by validate_group() because it was missing the shared_regs logic.

This patch changes the allocation of the fake cpuc used for validation to also point to a fake shared_regs structure such that group events be properly testing.

It modifies __intel_shared_reg_get_constraints() to use spin_lock_irqsave() to avoid lockdep issues.

Signed-off-by Stephane Eranian <[email protected]>
Signed-off-by Peter Zijlstra <[email protected]>
Link http://lkml.kernel.org/r/20110606145708.GA7279@quad open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/kernel/cpu/perf_event.c       |   59 ++++++++++++++++++++++++-------
 arch/x86/kernel/cpu/perf_event_intel.c |   16 ++++++--
 2 files changed, 57 insertions(+), 18 deletions(-)
arch/x86/kernel/cpu/perf_event.c +45 -14
index 019fda7..9a0f55c 100644
@@ -1689,6 +1689,40 @@ static int x86_pmu_commit_txn(struct pmu *pmu)
 	perf_pmu_enable(pmu);
 	return 0;
 }
+/*
+ * a fake_cpuc is used to validate event groups. Due to
+ * the extra reg logic, we need to also allocate a fake
+ * per_core and per_cpu structure. Otherwise, group events
+ * using extra reg may conflict without the kernel being
+ * able to catch this when the last event gets added to
+ * the group.
+ */
+static void free_fake_cpuc(struct cpu_hw_events *cpuc)
+{
+	kfree(cpuc->shared_regs);
+	kfree(cpuc);
+}
+
+static struct cpu_hw_events *allocate_fake_cpuc(void)
+{
+	struct cpu_hw_events *cpuc;
+	int cpu = raw_smp_processor_id();
+
+	cpuc = kzalloc(sizeof(*cpuc), GFP_KERNEL);
+	if (!cpuc)
+		return ERR_PTR(-ENOMEM);
+
+	/* only needed, if we have extra_regs */
+	if (x86_pmu.extra_regs) {
+		cpuc->shared_regs = allocate_shared_regs(cpu);
+		if (!cpuc->shared_regs)
+			goto error;
+	}
+	return cpuc;
+error:
+	free_fake_cpuc(cpuc);
+	return ERR_PTR(-ENOMEM);
+}
 
 /*
  * validate that we can schedule this event

@@ -1699,9 +1733,9 @@ static int validate_event(struct perf_event *event)
 	struct event_constraint *c;
 	int ret = 0;
 
-	fake_cpuc = kmalloc(sizeof(*fake_cpuc), GFP_KERNEL | __GFP_ZERO);
-	if (!fake_cpuc)
-		return -ENOMEM;
+	fake_cpuc = allocate_fake_cpuc();
+	if (IS_ERR(fake_cpuc))
+		return PTR_ERR(fake_cpuc);
 
 	c = x86_pmu.get_event_constraints(fake_cpuc, event);
 

@@ -1711,7 +1745,7 @@ static int validate_event(struct perf_event *event)
 	if (x86_pmu.put_event_constraints)
 		x86_pmu.put_event_constraints(fake_cpuc, event);
 
-	kfree(fake_cpuc);
+	free_fake_cpuc(fake_cpuc);
 
 	return ret;
 }

@@ -1731,35 +1765,32 @@ static int validate_group(struct perf_event *event)
 {
 	struct perf_event *leader = event->group_leader;
 	struct cpu_hw_events *fake_cpuc;
-	int ret, n;
+	int ret = -ENOSPC, n;
 
-	ret = -ENOMEM;
-	fake_cpuc = kmalloc(sizeof(*fake_cpuc), GFP_KERNEL | __GFP_ZERO);
-	if (!fake_cpuc)
-		goto out;
+	fake_cpuc = allocate_fake_cpuc();
+	if (IS_ERR(fake_cpuc))
+		return PTR_ERR(fake_cpuc);
 	/*
 	 * the event is not yet connected with its
 	 * siblings therefore we must first collect
 	 * existing siblings, then add the new event
 	 * before we can simulate the scheduling
 	 */
-	ret = -ENOSPC;
 	n = collect_events(fake_cpuc, leader, true);
 	if (n < 0)
-		goto out_free;
+		goto out;
 
 	fake_cpuc->n_events = n;
 	n = collect_events(fake_cpuc, event, false);
 	if (n < 0)
-		goto out_free;
+		goto out;
 
 	fake_cpuc->n_events = n;
 
 	ret = x86_pmu.schedule_events(fake_cpuc, n, NULL);
 
-out_free:
-	kfree(fake_cpuc);
 out:
+	free_fake_cpuc(fake_cpuc);
 	return ret;
 }
arch/x86/kernel/cpu/perf_event_intel.c +12 -4
index 6ad95ba..ac02b83 100644
@@ -1027,14 +1027,18 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
 {
 	struct event_constraint *c = &emptyconstraint;
 	struct er_account *era;
+	unsigned long flags;
 
 	/* already allocated shared msr */
-	if (reg->alloc || !cpuc->shared_regs)
+	if (reg->alloc)
 		return &unconstrained;
 
 	era = &cpuc->shared_regs->regs[reg->idx];
-
-	raw_spin_lock(&era->lock);
+	/*
+	 * we use spin_lock_irqsave() to avoid lockdep issues when
+	 * passing a fake cpuc
+	 */
+	raw_spin_lock_irqsave(&era->lock, flags);
 
 	if (!atomic_read(&era->ref) || era->config == reg->config) {
 

@@ -1058,7 +1062,7 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
 		 */
 		c = &unconstrained;
 	}
-	raw_spin_unlock(&era->lock);
+	raw_spin_unlock_irqrestore(&era->lock, flags);
 
 	return c;
 }

@@ -1524,4 +1528,8 @@ static int intel_pmu_init(void)
 	return 0;
 }
 
+static struct intel_shared_regs *allocate_shared_regs(int cpu)
+{
+	return NULL;
+}
 #endif /* CONFIG_CPU_SUP_INTEL */