Messages in this thread | | | Date | Tue, 5 Jun 2012 23:26:18 +0200 | Subject | Re: [PATCH] perf, x86: Fix Intel shared extra MSR allocation | From | Stephane Eranian <> |
| |
On Tue, Jun 5, 2012 at 3:56 PM, Peter Zijlstra <peterz@infradead.org> wrote: > On Tue, 2012-06-05 at 15:30 +0200, Peter Zijlstra wrote: > >> @@ -1157,14 +1163,14 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc, >> struct event_constraint *c = &emptyconstraint; >> struct er_account *era; >> unsigned long flags; >> - int orig_idx = reg->idx; >> + int idx = reg->idx; >> >> /* already allocated shared msr */ >> if (reg->alloc) >> return NULL; /* call x86_get_event_constraint() */ > > I'm afraid that needs to be: > > /* > * reg->alloc can be set due to existing state, so for fake cpuc > * we need to ignore this, otherwise we might fail to allocate > * proper fake state for this extra reg constraint. Also see > * the comment below. > */ > if (reg->alloc && !cpuc->is_fake) > return NULL; /* call x86_get_event_constraints() */ > >> Yes.
>> again: >> - era = &cpuc->shared_regs->regs[reg->idx]; >> + era = &cpuc->shared_regs->regs[idx]; >> /* >> * we use spin_lock_irqsave() to avoid lockdep issues when >> * passing a fake cpuc >> @@ -1173,6 +1179,29 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc, >> >> if (!atomic_read(&era->ref) || era->config == reg->config) { >> >> + /* >> + * If its a fake cpuc -- as per validate_{group,event}() we >> + * shouldn't touch event state and we can avoid doing so >> + * since both will only call get_event_constraints() once >> + * on each event, this avoids the need for reg->alloc. >> + * >> + * Not doing the ER fixup will only result in era->reg being >> + * wrong, but since we won't actually try and program hardware >> + * this isn't a problem either. >> + */ >> + if (!cpuc->is_fake) { >> + if (idx != reg->idx) >> + intel_fixup_er(event, idx); >> + >> + /* >> + * x86_schedule_events() can call get_event_constraints() >> + * multiple times on events in the case of incremental >> + * scheduling(). reg->alloc ensures we only do the ER >> + * allocation once. >> + */ >> + reg->alloc = 1; >> + } >> + >> /* lock in msr value */ >> era->config = reg->config; >> era->reg = reg->reg; >
| |