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

[Paul E. McKenney]

On Thu, May 13, 2010 at 10:59:26PM +0200, Frederic Weisbecker wrote:
> On Thu, May 13, 2010 at 10:48:58PM +0200, Frederic Weisbecker wrote:
> > Paul, does that look good to you?
> > I've only compile tested. But if it's fine for you, I'll test
> > it for real and provide you a sane patch.

That would be very good!!!  I believe that I have already more than proven
my ignorance of the perf_event code.  ;-)

> > This version won't use more parameters than necessary in the
> > off case.
> > 
> > 
> > diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> > index a4fa381..d765e48 100644
> > --- a/kernel/perf_event.c
> > +++ b/kernel/perf_event.c
> > @@ -4066,19 +4066,36 @@ static inline u64 swevent_hash(u64 type, u32 event_id)
> >  	return hash_64(val, SWEVENT_HLIST_BITS);
> >  }
> >  
> > -static struct hlist_head *
> > -find_swevent_head(struct perf_cpu_context *ctx, u64 type, u32 event_id)
> > +static inline struct hlist_head *
> > +__find_swevent_head(struct swevent_hlist *hlist, u64 type, u32 event_id)
> >  {
> >  	u64 hash;
> > -	struct swevent_hlist *hlist;
> 
> 
> 
> +	if (!hlist)
> +		return NULL;
> 
> 
> 
> >  
> >  	hash = swevent_hash(type, event_id);
> >  
> > +	return &hlist->heads[hash];
> > +}
> > +
> > +static inline struct hlist_head *
> > +find_swevent_head_rcu(struct perf_cpu_context *ctx, u64 type, u32 event_id)
> > +{
> > +	struct swevent_hlist *hlist;
> > +
> >  	hlist = rcu_dereference(ctx->swevent_hlist);

This is appropriate if find_swevent_head_rcu() is always invoked in an
RCU read-side critical section.  (Which at first glance does appear to
be the intent, just checking.)

> > -	if (!hlist)
> > -		return NULL;
> >  
> > -	return &hlist->heads[hash];
> > +	return __find_swevent_head(hlist, type, event_id);
> > +}
> > +
> > +static inline struct hlist_head *
> > +find_swevent_head(struct perf_cpu_context *ctx, u64 type,
> > +		  u32 event_id, struct perf_event *event)
> > +{
> > +	struct swevent_hlist *hlist;
> > +
> > +	hlist = rcu_dereference_check(ctx->swevent_hlist,
> > +				      lockdep_is_held(&event->ctx->lock));

This could be invoked with either the event->ctx->lock held or in
an RCU read-side critical section.  If this is always called with
the update-side lock held, you can (but don't need to) instead say:

	hlist = rcu_dereference_protected(ctx->swevent_hlist,
				          lockdep_is_held(&event->ctx->lock));

This is slightly faster, as it drops the volatile casts.  In many cases,
you won't care, but in case this code path needs ultimate performance.

Also I thought it was event->ctx.lock, but at this point I trust your eyes
more than my own.  ;-)

							Thanx, Paul

> > +
> > +	return __find_swevent_head(hlist, type, event_id);
> >  }
> >  
> >  static void do_perf_sw_event(enum perf_type_id type, u32 event_id,
> > @@ -4095,7 +4112,7 @@ static void do_perf_sw_event(enum perf_type_id type, u32 event_id,
> >  
> >  	rcu_read_lock();
> >  
> > -	head = find_swevent_head(cpuctx, type, event_id);
> > +	head = find_swevent_head_rcu(cpuctx, type, event_id);
> >  
> >  	if (!head)
> >  		goto end;
> > @@ -4178,7 +4195,8 @@ static int perf_swevent_enable(struct perf_event *event)
> >  		perf_swevent_set_period(event);
> >  	}
> >  
> > -	head = find_swevent_head(cpuctx, event->attr.type, event->attr.config);
> > +	head = find_swevent_head(cpuctx, event->attr.type,
> > +				 event->attr.config, event);
> >  	if (WARN_ON_ONCE(!head))
> >  		return -EINVAL;
> >  
> > 
> > 
> 
--
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/