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

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, May 13, 2010 at 11:25:56AM -0700, Paul E. McKenney wrote:
> This commit guesses at the perf_cpu_context locking design and deploys
> an rcu_dereference_check() accordingly.  The design appears to require
> that a given CPU be accessing its own per_cpu_context or that it be
> traversing under RCU protection.
> 
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> 
>  perf_event.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index a4fa381..002791c 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -4074,7 +4074,9 @@ find_swevent_head(struct perf_cpu_context *ctx, u64 type, u32 event_id)
>  
>  	hash = swevent_hash(type, event_id);
>  
> -	hlist = rcu_dereference(ctx->swevent_hlist);
> +	hlist = rcu_dereference_check(ctx->swevent_hlist,
> +				      rcu_read_lock_held() ||
> +				      ctx == &__get_cpu_var(perf_cpu_context));
>  	if (!hlist)
>  		return NULL;
>  


Hmm, that's not exactly that. It will always be the ctx of this cpu
but not always under rcu read lock. I mean touching the current cpu
ctx is not inherently safe.

In fact we have two paths:

perf_swevent_enable() gets the hlist and if it is called it means
that this hlist is not supposed to be NULL. If it is, it's a bug.

If we have created a software event, the hlist has been allocated
and perf_swevent_enable() is called later to activate this event.
May be I shouldn't use rcu_dereference() here but a simple dereference.
And the hlist can't be freed under us at this time so we don't need
rcu_read_lock().

OTOH, do_perf_sw_event() can be called anytime so it need this
rcu_read_lock().


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));

or:


hlist = ctx->swevent_hlist;