Re: [PATCH v9 12/14] x86: perf: intel_pt: Intel PT PMU driver

[Alexander Shishkin <alexander.shishkin@linux.intel.com>]

On 29 January 2015 at 17:20, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Jan 29, 2015 at 05:03:21PM +0200, Alexander Shishkin wrote:
>> > We're already holding ctx->mutex, this should have made lockdep scream.
>>
>> As I mentioned offline, cpuctx->ctx.mutex is set to a lockdep class of
>> its own, so lockdep doesn't see this. It is, of course, still a
>> problem.
>
> Right; I don't think we currently use that annotation of
> cpuctx->ctx.mutex but I had indeed overlooked that.
>
> Per perf_ctx_lock() the nesting order is:
>
>  cpuctx
>    ctx
>
> And here we would have done the inverse. Still I'm not entirely sure it
> would've resulted in a deadlock, we typically don't take multiple
> ctx->mutex locks, and where we do its either between different context
> on the same CPU (eg, moving a software event to the hardware lists), or
> between the same context on different CPUs (perf_pmu_migrate_context),
> or between inherited contexts (put_event, in child->parent nesting).
>
> None of those cases seem to overlap with this order.

Indeed.

> However,
>
>> But as you pointed out, if we grab the exclusive_cnt for per-task
>> cpu!=-1 events as well, we don't need to look into other contexts here
>> at all.
>
> its irrelevant because we can avoid the entire ordeal ;-)

Exactly. :)

>> > --- a/kernel/events/core.c
>> > +++ b/kernel/events/core.c
>> > @@ -3487,6 +3487,9 @@ static void __free_event(struct perf_eve
>> >         if (event->destroy)
>> >                 event->destroy(event);
>> >
>> > +       if (event->pmu && event->ctx)
>> > +               exclusive_event_release(event);
>>
>> It looks like event can be already removed from its context at this
>> point, so event->ctx will be NULL and the counter would leak or am I
>> missing something?
>
> Yeah, event->ctx is _magic_ ;-)
>
> event->ctx is never NULL, although it can change. Much fun because of
> that; see: lkml.kernel.org/r/20150123125159.696530128@infradead.org if
> you like to torture yourself a wee bit.

Yeah, I already did, having noticed mutex_lock_double() in the context
of your diff. And I'll probably re-read it a few more times. :)

>> I used the event->attach_state & PERF_ATTACH_TASK to see if it's a
>> per-task counter, which seems reliable even though it might need
>> documenting.
>
> Yes, I suppose that is indeed better, I had feared we clear the
> ATTACH_TASK state on remove_from_context() like we do all the other
> ATTACH states, but we do not.

Yep. It probably needs documenting though, in case somebody decides to
change this in future.

>> > +static bool exclusive_event_ok(struct perf_event *event,
>> > +                              struct perf_event_context *ctx)
>>
>> Then, maybe exclusive_event_installable() is a better name, because
>> this one only deals with the current context; cpu-wide vs per-task
>> case is taken care of in perf_event_alloc()/__free_event().
>
> OK.
>
>> > +       /*
>> > +        * exclusive_cnt <0: cpu
>> > +        *               >0: tsk
>> > +        */
>> > +       if (ctx->task) {
>> > +               if (!atomic_inc_unless_negative(&pmu->exclusive_cnt))
>> > +                       return false;
>> > +       } else {
>> > +               if (!atomic_dec_unless_positive(&pmu->exclusive_cnt))
>> > +                       return false;
>> > +       }
>>
>> So I would like to keep this bit in perf_event_alloc() path and the
>> reverse in __free_event(),
>
> Fair enough; exclusive_event_init() ?

Ok.

>
>> > +
>> > +       mutex_lock(&ctx->lock);
>> > +       ret = __exclusive_event_ok(event, ctx);
>> > +       mutex_unlock(&ctx->lock);
>
> And as you pointed out on IRC, this needs to be in the same section as
> install_in_context() otherwise we have a race.
>
>> > +       if (!ret) {
>> > +               if (ctx->task)
>> > +                       atomic_dec(&pmu->exclusive_cnt);
>> > +               else
>> > +                       atomic_inc(&pmu->exclusive_cnt);
>> > +       }
>>
>> in which case we don't need to undo the counter here, because it will
>> still go through __free_event() in the error path.
>
> OK; exclusive_event_destroy() ?

Yep.

Cheers,
--
Alex