| Message ID | 5523A412.2000805@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 07, 2015 at 11:32:02AM +0200, Maarten Lankhorst wrote: > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index b13c5526a73b..7aaf8eddf19c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2146,14 +2146,14 @@ i915_gem_request_unreference(struct drm_i915_gem_request *req) > static inline void > i915_gem_request_unreference__unlocked(struct drm_i915_gem_request *req) > { > - if (req && !atomic_add_unless(&req->ref.refcount, -1, 1)) { > - struct drm_device *dev = req->ring->dev; > + struct drm_device *dev; > + > + if (!req) > + return; > > - mutex_lock(&dev->struct_mutex); > - if (likely(atomic_dec_and_test(&req->ref.refcount))) > - i915_gem_request_free(&req->ref); > + dev = req->ring->dev; > + if (kref_put_mutex(&req->ref, i915_gem_request_free, &dev->struct_mutex)) > mutex_unlock(&dev->struct_mutex); We don't need this conditional unlock here since that's only possible if you have a weak reference somewhere (i.e. using kref_get_unless_zero). If the object only has strong references and you're dropping the last one it can't magically get resurrected somehow. And drm_gem_object_unreference_unlocked wants the same patch I think. -Daniel
Op 07-04-15 om 15:37 schreef Daniel Vetter: > On Tue, Apr 07, 2015 at 11:32:02AM +0200, Maarten Lankhorst wrote: >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index b13c5526a73b..7aaf8eddf19c 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2146,14 +2146,14 @@ i915_gem_request_unreference(struct drm_i915_gem_request *req) >> static inline void >> i915_gem_request_unreference__unlocked(struct drm_i915_gem_request *req) >> { >> - if (req && !atomic_add_unless(&req->ref.refcount, -1, 1)) { >> - struct drm_device *dev = req->ring->dev; >> + struct drm_device *dev; >> + >> + if (!req) >> + return; >> >> - mutex_lock(&dev->struct_mutex); >> - if (likely(atomic_dec_and_test(&req->ref.refcount))) >> - i915_gem_request_free(&req->ref); >> + dev = req->ring->dev; >> + if (kref_put_mutex(&req->ref, i915_gem_request_free, &dev->struct_mutex)) >> mutex_unlock(&dev->struct_mutex); > We don't need this conditional unlock here since that's only possible if > you have a weak reference somewhere (i.e. using kref_get_unless_zero). If > the object only has strong references and you're dropping the last one it > can't magically get resurrected somehow. Because we use the same put call for kref_put and kref_put_mutex we do need to unlock struct_mutex here, kref_put_mutex doesn't release the mutex if true, so either the release call needs to do it or the callee. > And drm_gem_object_unreference_unlocked wants the same patch I think. Indeed it does, but with slightly more lockdep annotation!
On Tue, Apr 07, 2015 at 03:51:44PM +0200, Maarten Lankhorst wrote: > Op 07-04-15 om 15:37 schreef Daniel Vetter: > > On Tue, Apr 07, 2015 at 11:32:02AM +0200, Maarten Lankhorst wrote: > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >> index b13c5526a73b..7aaf8eddf19c 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -2146,14 +2146,14 @@ i915_gem_request_unreference(struct drm_i915_gem_request *req) > >> static inline void > >> i915_gem_request_unreference__unlocked(struct drm_i915_gem_request *req) > >> { > >> - if (req && !atomic_add_unless(&req->ref.refcount, -1, 1)) { > >> - struct drm_device *dev = req->ring->dev; > >> + struct drm_device *dev; > >> + > >> + if (!req) > >> + return; > >> > >> - mutex_lock(&dev->struct_mutex); > >> - if (likely(atomic_dec_and_test(&req->ref.refcount))) > >> - i915_gem_request_free(&req->ref); > >> + dev = req->ring->dev; > >> + if (kref_put_mutex(&req->ref, i915_gem_request_free, &dev->struct_mutex)) > >> mutex_unlock(&dev->struct_mutex); > > We don't need this conditional unlock here since that's only possible if > > you have a weak reference somewhere (i.e. using kref_get_unless_zero). If > > the object only has strong references and you're dropping the last one it > > can't magically get resurrected somehow. > Because we use the same put call for kref_put and kref_put_mutex we do need to unlock struct_mutex here, > kref_put_mutex doesn't release the mutex if true, so either the release call needs to do it or the callee. Indeed I didn't realize that all existing users of kref_put_mutex unlock the mutex in the free callback. So looks all good, so applied it. Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b13c5526a73b..7aaf8eddf19c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2146,14 +2146,14 @@ i915_gem_request_unreference(struct drm_i915_gem_request *req) static inline void i915_gem_request_unreference__unlocked(struct drm_i915_gem_request *req) { - if (req && !atomic_add_unless(&req->ref.refcount, -1, 1)) { - struct drm_device *dev = req->ring->dev; + struct drm_device *dev; + + if (!req) + return; - mutex_lock(&dev->struct_mutex); - if (likely(atomic_dec_and_test(&req->ref.refcount))) - i915_gem_request_free(&req->ref); + dev = req->ring->dev; + if (kref_put_mutex(&req->ref, i915_gem_request_free, &dev->struct_mutex)) mutex_unlock(&dev->struct_mutex); - } } static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> ---