Messages in this thread | | | Date | Mon, 24 Jun 2013 17:48:20 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH v2] perf,x86: Fix shared register mutual exclusion enforcement |
| |
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.
| |