Re: drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Thu, Aug 11, 2016 at 11:44:08AM +0200, Peter Zijlstra wrote:
> On Wed, Jun 22, 2016 at 08:46:12AM +0100, Chris Wilson wrote:
> > We are only documenting that the read is outside of the lock, and do not
> > require strict ordering on the operation. In this case the more relaxed
> > lockless_dereference() will suffice.
> 
> No, no, no... This is 'broken'. lockless_dereference() is _stronger_
> than READ_ONCE(), not weaker.
> 
> lockless_dereference() is a wrapper around smp_read_barrier_depends()
> and is used to form read dependencies. There is no read dependency here,
> therefore using lockless_dereference() is entirely pointless.
> 
> Look at the definition of lockless_dereference(), it does a READ_ONCE()
> and then smp_read_barrier_depends().
> 
> Also, clue is in the name: 'dereference', you don't actually dereference
> the pointer here, only load it.

What Peter said!

If you are just checking the value of an RCU-protected pointer,
that is, one on which you would use rcu_dereference() rather than
lockless_dereference(), rcu_access_pointer() does the job of READ_ONCE()
while keeping sparse happy.

							Thanx, Paul

> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -464,7 +464,7 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
> >  
> >  	/* Sometimes user space wants everything disabled, so don't steal the
> >  	 * display if there's a master. */
> > -	if (READ_ONCE(dev->master))
> > +	if (lockless_dereference(dev->master))
> >  		return false;
> >  
> >  	drm_for_each_crtc(crtc, dev) {
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx