[PATCH] drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference

[PATCH] drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference

[Chris Wilson]

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.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0a06f9120b5a..ce54e985d91b 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ 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) {
-- 
2.8.1

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

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

[Daniel Vetter]

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.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>

Applied to drm-misc.
-Daniel

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0a06f9120b5a..ce54e985d91b 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ 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) {
> -- 
> 2.8.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Ro.CI.BAT: success for drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference

[Patchwork]

== Series Details ==

Series: drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference
URL   : https://patchwork.freedesktop.org/series/9029/
State : success

== Summary ==

Series 9029v1 drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference
http://patchwork.freedesktop.org/api/1.0/series/9029/revisions/1/mbox


fi-skl-i5-6260u  total:225  pass:198  dwarn:0   dfail:0   fail:2   skip:25 
fi-skl-i7-6700k  total:225  pass:186  dwarn:0   dfail:0   fail:2   skip:37 
fi-snb-i7-2600   total:225  pass:172  dwarn:0   dfail:0   fail:2   skip:51 
ro-ivb-i7-3770   total:224  pass:180  dwarn:0   dfail:0   fail:0   skip:44 
fi-hsw-i7-4770k failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1261/

9201a72 drm-intel-nightly: 2016y-06m-21d-21h-26m-35s UTC integration manifest
26bb3f3 drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference

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

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

[Peter Zijlstra]

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.

> +++ 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) {
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Paul E. McKenney]

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