perf_events: fix validation of events using an extra reg (v2)
Subsystems (2)
PATCH 2/3 perf_events: fix validation of events using an extra reg (v2)
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.
---
arch/x86/kernel/cpu/perf_event.c +48 -14
@@ -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; }
arch/x86/kernel/cpu/perf_event_intel.c +14 -4
@@ -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; }
Peter Zijlstra
maintainerQuoted 2 lines · Stephane Eranian
The
is_fakefield is necessary to avoid a lockdep issue having to do with interrupt masking andera->lock.
Why not simply disable IRQs in both validate_* functions?
Quoted 6 lines · Peter Zijlstra
Quoted 2 lines · Stephane Eranian
The
is_fakefield is necessary to avoid a lockdep issue having to do with interrupt masking andera->lock.Why not simply disable IRQs in both validate_* functions?
Could do that too if you think that's cleaner.
Peter Zijlstra
maintainerQuoted 8 lines · Stephane Eranian
Quoted 6 lines · Peter Zijlstra
Quoted 2 lines · Stephane Eranian
The
is_fakefield is necessary to avoid a lockdep issue having to do with interrupt masking andera->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.
Quoted 6 lines · Peter Zijlstra
Quoted 5 lines · Stephane Eranian
Quoted 4 lines · Peter Zijlstra
Quoted 2 lines · Stephane Eranian
The
is_fakefield is necessary to avoid a lockdep issue having to do with interrupt masking andera->lock.
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines
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.