Messages in this thread Patch in this message | | | Subject | Re: [PATCH] perf, x86: Fix Intel shared extra MSR allocation | From | Peter Zijlstra <> | Date | Tue, 05 Jun 2012 15:56:26 +0200 |
| |
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() */
> > 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;
| |