Re: [PATCH RFC] perf: fix find_swevent_head() RCU lockdep splat

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, May 13, 2010 at 12:46:05PM -0700, Paul E. McKenney wrote:
> On Thu, May 13, 2010 at 09:03:28PM +0200, Frederic Weisbecker wrote:
> > On the perf_swevent_enable() path, what prevents the hlist to be
> > freed under us is the ctx->lock. Because we won't ever remove
> > an event from its context list outside this lock, and we might only
> > release the hlist after a software event gets removed from its
> > context list.
> > 
> > So either we do this:
> > 
> > hlist = rcu_dereference_check(ctx->swevent_hlist,
> >                               rcu_read_lock_held() ||
> >                               raw_spin_lock_is_held(&ctx->lock));
> 
> Something very similar to the above was in fact my first guess, but as
> Ingo can attest, this results in build errors.  The problem is that
> perf_cpu_context does not have a ->lock field.  Hmmm...  But it does
> contain a struct perf_event_context (ctx) and a pointer to another
> (task_ctx).  So, would one of the following work?
> 
> list = rcu_dereference_check(ctx->swevent_hlist,
> 			     rcu_read_lock_held() ||
> 			     lockdep_is_held(&ctx->ctx.lock));
> 
> list = rcu_dereference_check(ctx->swevent_hlist,
> 			     rcu_read_lock_held() ||
> 			     lockdep_is_held(&ctx->task_ctx->lock));



Ah right. Oh, and it depends where the current event to be enabled
come from: a task context or a cpu context, it can be one of these
two locks. The following check wouldn't be 100 % reliable:

	lockdep_is_held(&ctx->ctx.lock) || lockdep_is_held(&ctx->task_ctx->lock))

because it has a small hole: the cpu context lock can be validated while
the current event needs the task one, or opposite. But at least it narrows
down.

Yet we could get the event in the perf_swevent_enable() and do:

	event && lockdep_is_held(event->ctx->lock)

(do_perf_sw_event() can pass NULL as the event, because it doesn't have
any yet).

That has a drawback though: find_swevent_head() would require one more
parameter, for something used in a fastpath.

That's sad but the only solution I can find.