| Message ID | 1466581572-16608-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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) {
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) { >
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) {
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(-)