* [PATCH 2/3] perf_events: fix validation of events using an extra reg (v2)
@ 2011-05-20 14:37 Stephane Eranian
2011-05-20 15:05 ` Peter Zijlstra
0 siblings, 1 reply; 5+ messages in thread
From: Stephane Eranian @ 2011-05-20 14:37 UTC (permalink / raw)
To: linux-kernel; +Cc: mingo, peterz, andi, ming.m.lin
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.
The is_fake field is necessary to avoid a lockdep issue having
to do with interrupt masking and era->lock.
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..3918927 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -139,6 +139,7 @@ struct cpu_hw_events {
struct perf_event *event_list[X86_PMC_IDX_MAX]; /* in enabled order */
unsigned int group_flag;
+ bool is_fake; /* fake cpuc for validation purposes */
/*
* Intel DebugStore bits
@@ -1682,6 +1683,42 @@ 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);
+
+ cpuc->is_fake = true;
+
+ /* 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 +1729,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 +1741,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 +1761,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..974eaf2 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -1029,12 +1029,21 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
struct er_account *era;
/* 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 do not lock in case we come here via validate_group(), i.e.,
+ * fake cpuc, otherwise we trigger a lockdep issue. Locking is not
+ * necessary with the fakecpu, as we are simply trying to validate
+ * co-scheduling within a group.
+ *
+ * Note that locking is done even when the register is not shared
+ * between CPUs but there will never be contention.
+ */
+ if (!cpuc->is_fake)
+ raw_spin_lock(&era->lock);
if (!atomic_read(&era->ref) || era->config == reg->config) {
@@ -1058,7 +1067,8 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
*/
c = &unconstrained;
}
- raw_spin_unlock(&era->lock);
+ if (!cpuc->is_fake)
+ raw_spin_unlock(&era->lock);
return c;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] perf_events: fix validation of events using an extra reg (v2)
2011-05-20 14:37 [PATCH 2/3] perf_events: fix validation of events using an extra reg (v2) Stephane Eranian
@ 2011-05-20 15:05 ` Peter Zijlstra
2011-05-20 16:45 ` Stephane Eranian
0 siblings, 1 reply; 5+ messages in thread
From: Peter Zijlstra @ 2011-05-20 15:05 UTC (permalink / raw)
To: Stephane Eranian; +Cc: linux-kernel, mingo, andi, ming.m.lin
On Fri, 2011-05-20 at 16:37 +0200, Stephane Eranian wrote:
> The is_fake field is necessary to avoid a lockdep issue having
> to do with interrupt masking and era->lock.
Why not simply disable IRQs in both validate_* functions?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] perf_events: fix validation of events using an extra reg (v2)
2011-05-20 15:05 ` Peter Zijlstra
@ 2011-05-20 16:45 ` Stephane Eranian
2011-05-23 7:47 ` Peter Zijlstra
0 siblings, 1 reply; 5+ messages in thread
From: Stephane Eranian @ 2011-05-20 16:45 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: linux-kernel, mingo, andi, ming.m.lin
On Fri, May 20, 2011 at 5:05 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, 2011-05-20 at 16:37 +0200, Stephane Eranian wrote:
>> The is_fake field is necessary to avoid a lockdep issue having
>> to do with interrupt masking and era->lock.
>
> Why not simply disable IRQs in both validate_* functions?
>
Could do that too if you think that's cleaner.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] perf_events: fix validation of events using an extra reg (v2)
2011-05-20 16:45 ` Stephane Eranian
@ 2011-05-23 7:47 ` Peter Zijlstra
2011-05-23 9:15 ` Stephane Eranian
0 siblings, 1 reply; 5+ messages in thread
From: Peter Zijlstra @ 2011-05-23 7:47 UTC (permalink / raw)
To: Stephane Eranian; +Cc: linux-kernel, mingo, andi, ming.m.lin
On Fri, 2011-05-20 at 18:45 +0200, Stephane Eranian wrote:
> On Fri, May 20, 2011 at 5:05 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Fri, 2011-05-20 at 16:37 +0200, Stephane Eranian wrote:
> >> The is_fake field is necessary to avoid a lockdep issue having
> >> to do with interrupt masking and era->lock.
> >
> > Why not simply disable IRQs in both validate_* functions?
> >
> Could do that too if you think that's cleaner.
Its saner than making that code path conditional, but an alternative
solution is to give the fake thing a different lock class.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] perf_events: fix validation of events using an extra reg (v2)
2011-05-23 7:47 ` Peter Zijlstra
@ 2011-05-23 9:15 ` Stephane Eranian
0 siblings, 0 replies; 5+ messages in thread
From: Stephane Eranian @ 2011-05-23 9:15 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: LKML, mingo, Andi Kleen, Lin Ming
On Mon, May 23, 2011 at 9:47 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, 2011-05-20 at 18:45 +0200, Stephane Eranian wrote:
>> On Fri, May 20, 2011 at 5:05 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Fri, 2011-05-20 at 16:37 +0200, Stephane Eranian wrote:
>> >> The is_fake field is necessary to avoid a lockdep issue having
>> >> to do with interrupt masking and era->lock.
>> >
>> > Why not simply disable IRQs in both validate_* functions?
>> >
>> Could do that too if you think that's cleaner.
>
> Its saner than making that code path conditional, but an alternative
> solution is to give the fake thing a different lock class.
>
Looks like using irqsave()/irqrestore() would be easier. Let me respin
the patch series for this.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-05-23 9:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-20 14:37 [PATCH 2/3] perf_events: fix validation of events using an extra reg (v2) Stephane Eranian
2011-05-20 15:05 ` Peter Zijlstra
2011-05-20 16:45 ` Stephane Eranian
2011-05-23 7:47 ` Peter Zijlstra
2011-05-23 9:15 ` Stephane Eranian
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.