Re: [PATCH] perf, x86: Fix Intel shared extra MSR allocation

[Stephane Eranian]

On Tue, Jun 5, 2012 at 3:56 PM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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;
>
N‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ0¶ìh®å’i