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

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

[Stephane Eranian]

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 <peterz@infradead.org>
Signed-off-by: Stephane Eranian <eranian@google.com>
--

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 0f8d3ff..ed6af75 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -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;
 }
 
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index 2d40f33..69afaad 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -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 */

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

[Peter Zijlstra]

On Mon, 2011-06-06 at 16:57 +0200, Stephane Eranian wrote:
> +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?

> +       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);
> +} 

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

[Stephane Eranian]

On Thu, Jun 9, 2011 at 8:40 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, 2011-06-06 at 16:57 +0200, Stephane Eranian wrote:
>> +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.

> 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.


>> +       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);
>> +}
>
>

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

[Peter Zijlstra]

On Thu, 2011-06-09 at 22:36 +0200, Stephane Eranian wrote:
> On Thu, Jun 9, 2011 at 8:40 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, 2011-06-06 at 16:57 +0200, Stephane Eranian wrote:
> >> +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.

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

> > 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.

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

[Stephane Eranian]

On Thu, Jun 9, 2011 at 10:48 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, 2011-06-09 at 22:36 +0200, Stephane Eranian wrote:
>> On Thu, Jun 9, 2011 at 8:40 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Mon, 2011-06-06 at 16:57 +0200, Stephane Eranian wrote:
>> >> +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.
>
> Yeah, I did too, Ingo found it during testing.
>
>> > 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?

>> 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.
>
>

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

[Peter Zijlstra]

On Tue, 2011-06-21 at 11:54 +0200, Stephane Eranian wrote:
> On Thu, Jun 9, 2011 at 10:48 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Thu, 2011-06-09 at 22:36 +0200, Stephane Eranian wrote:
> >> On Thu, Jun 9, 2011 at 8:40 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> >> > On Mon, 2011-06-06 at 16:57 +0200, Stephane Eranian wrote:
> >> >> +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.
> >
> > Yeah, I did too, Ingo found it during testing.
> >
> >> > 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?

Yeah. Let me shove these patches mingo wards again.

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

[tip-bot for Stephane Eranian]

Commit-ID:  cd8a38d33e2528998998bae70a45ad27e442f114
Gitweb:     http://git.kernel.org/tip/cd8a38d33e2528998998bae70a45ad27e442f114
Author:     Stephane Eranian <eranian@google.com>
AuthorDate: Mon, 6 Jun 2011 16:57:08 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
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 <eranian@google.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20110606145708.GA7279@quad
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/cpu/perf_event.c       |   59 ++++++++++++++++++++++++-------
 arch/x86/kernel/cpu/perf_event_intel.c |   16 ++++++--
 2 files changed, 57 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 019fda7..9a0f55c 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -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;
 }
 
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index 6ad95ba..ac02b83 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -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 */