Re: [PATCH v2] perf,x86: Fix shared register mutual exclusionenforcement

[Peter Zijlstra]

On Mon, Jun 24, 2013 at 10:01:26AM +0200, Stephane Eranian wrote:

> You are missing the error path in schedule_events():
> 
>  if (!assign || num) {
> 
>                 for (i = 0; i < n; i++) {
>                         if (x86_pmu.put_event_constraints)
>                                 x86_pmu.put_event_constraints(cpuc,
> cpuc->event_list[i]);
>                 }
> 
>         }
> 
> That one wipes out on get() even on events that were correctly
> schedule in the previous
> invocation. So here group2 fails, but it should not release the
> constraints from group1.

What I was saying:

 schedule(group1)
   get_event_constraints() +1
   no error path, no puts

 schedule(group2)
   get_event_constraints() +1
   *fail*
     put_event_constraints() -1

This leaves the constraints of group1 with a net +1 'ref' count and thus
if we were to treat the get/put as such, the put wouldn't be the last
and thus shouldn't release resources.

> > Only once these events pass through x86_pmu_del() will they get a final
> > put and the 'ref' count will drop to 0.
> >
> > Now the problem seems to be the get/put things don't actually count
> > properly.
> >
> > However, if we look at __intel_shared_reg_{get,put}_constraints() there
> > is a refcount in there; namely era->ref; however we don't appear to
> > clear reg->alloc based on it.
> >
> The era->ref is not used to ref count the number of successful attempts
> at scheduling. It is used to count the number of CPU sharing the resource.
> So it goes from 0, 1, to 2. You can invoke schedule_events() many more
> times. The reg->alloc is a bypass, to avoid checking the shared reg
> again and again if it succeeded once.

Oh right, I knew I was missing something..  :/

> For a while I thought I could leverage the era->ref to account the get/put.
> But it does not work. Because the of the put().

Crud, right you are. 

Also, I don't think we could even use them as I outlined; suppose it
would have worked; then we'd have:

  schedule(group1)
    get_event_constraints() +1

  schedule(group2)
    get_event_constraints() +1

And we'd be stuck with a ref of 2, the put at x86_pmu_del() would never
be sufficient to drop them back to 0 again.

A well, your patch does indeed make it work so I'll grab that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/