Re: [PATCH v2 01/11] perf,x86: Fix event/group validation

[Stephane Eranian <eranian@google.com>]

On Tue, May 26, 2015 at 5:16 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, May 26, 2015 at 04:46:07AM -0700, Stephane Eranian wrote:
>> On Tue, May 26, 2015 at 3:12 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Tue, May 26, 2015 at 02:24:38AM -0700, Stephane Eranian wrote:
>> >> On Fri, May 22, 2015 at 6:40 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> >> > On Fri, May 22, 2015 at 03:29:06PM +0200, Peter Zijlstra wrote:
>> >> >> @@ -788,9 +788,9 @@ int x86_schedule_events(struct cpu_hw_ev
>> >> >>               x86_pmu.start_scheduling(cpuc);
>> >> >>
>> >> >>       for (i = 0, wmin = X86_PMC_IDX_MAX, wmax = 0; i < n; i++) {
>> >> >> +             cpuc->event_constraint[i] = NULL;
>> >> >
>
>> But where is the code that says: skip reinstalling the constraint
>> in intel_get_event_constraints() because there is already a (stale)
>> one? I don't see where that is.
>
> IIRC the problem was that the copy from c2 into c1:
>
>         if (c1 && (c1->flags & PERF_X86_EVENT_DYNAMIC)) {
>                 bitmap_copy(c1->idxmsk, c2->idxmsk, X86_PMC_IDX_MAX);
>                 c1->weight = c2->weight;
>                 c2 = c1;
>         }
>
> is incomplete. For instance, flags is not copied, and some code down the
> line might check that and get wrong flags.
>
Ok, now I remember this code. It has to do with incremental scheduling.
Suppose E1, E2, E3 events where E1 is exclusive. The first call is
for scheduling E1. It gets to get_event_constraint() which "allocates" a
dynamic constraint. The second call  tries to schedule E1, E2. But the
second time for E1, you already have the dynamic constraint allocated, so
this code is reusing the constraint storage and just updates the bitmask
and weight.

Now, that the storage is not actually dynamic (kmalloc'd), but taken from a
fixed size array in cpuc, I believe we can simplify this and "re-allocate"
the constraint for each incremental call to intel_get_event_constraints().
Do you agree?


> I'm not entirely sure I saw misbehaviour, but I figured I'd better close
> that hole and rule out this is contributing to fail.
>