Re: [PATCH] perf: Fix intel shared extra msr allocation

["Yan, Zheng" <zheng.z.yan@linux.intel.com>]

On 06/04/2012 09:12 PM, Stephane Eranian wrote:
> On Fri, Jun 1, 2012 at 4:11 PM, Yan, Zheng <zheng.z.yan@linux.intel.com> wrote:
>> On Fri, Jun 1, 2012 at 5:35 PM, Stephane Eranian <eranian@google.com> wrote:
>>> On Fri, Jun 1, 2012 at 5:20 AM, Yan, Zheng <zheng.z.yan@intel.com> wrote:
>>>> From: "Yan, Zheng" <zheng.z.yan@intel.com>
>>>>
>>>> intel_shared_reg_get/put_constraints() can be indirectly called
>>>> by validate_group(). In that case, they should avoid modifying
>>>> the perf_event date structure because the event can be already
>>>> in active state. Otherwise the shared extra msr's reference
>>>> count will be left in inconsistent state.
>>>>
>>> I understand the problem but I am wondering if you actually saw
>>> it in real life. The reason I am asking is because  of the way
>>> validate_group() collects the events and how they are added
>>> to sibling_list. The new event is added at the tail. Thus it will
>>> come last, and will get to __intel_shared_reg_get_constraints()
>>> last, thus I am wondering if it can really modify the programming
>>> on the existing events.
>>
>> The real problem is from __intel_shared_reg_put_constraints(). it set
>> reg->alloc to 0 and decreases fake_cpuc->shared_regs->regs[reg->idx]'s
>> reference count. Later when deleting the event,  put_constraints() will find
>> reg->alloc is 0 and it won't decrease the shared msr's reference count.
>>
>> Run 'perf stat --group -a -C 0 -e LLC-loads -e LLC-stores sleep 1" on
>> Nehalem can trigger the bug.
>>
> And what do you see in this particular example?
The offcore_rsp msr's reference count are left in inconsistent state. It means
we can only use the msr for LLC-loads event after that. Any other events that
require programming the offcore_rsp msr will fail.

> 
> I'd like to see the results via libpfm4 and /perf_examples/syst_count:
> $ sudo ./syst_count -e offcore_response_0:dmnd_data_rd
> -eoffcore_response_0:dmnd_rfo  -p -d 10 -c 0
> 
This does not use the event group feature. do you mean
./syst_count -e offcore_response_0:dmnd_data_rd,offcore_response_0:dmnd_rfo -p -d 10 -c 0.

After apply my patch, the above command fails on Nehalem. Because there is no offcore_rsp1.

Regards
Yan, Zheng