| Message ID | 1444894601-5200-10-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Pretty soon only some drivers will need dev->struct_mutex in their > gem_free_object callbacks. Hence it's really important to make sure > everything still keeps getting this right. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > include/drm/drm_gem.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 7a592d7e398b..5b3754864fb0 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -142,6 +142,8 @@ drm_gem_object_reference(struct drm_gem_object *obj) > static inline void > drm_gem_object_unreference(struct drm_gem_object *obj) > { > + WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); > + This doesn't work. mutex_is_locked() is not context-aware, so it does not check whether the holder is the current thread or someone else. You *have* to use lockdep if you want negative lock checks. In other words, if some other thread holds the mutex in parallel to this being called, you will trigger the WARN_ON. Thanks David > if (obj != NULL) > kref_put(&obj->refcount, drm_gem_object_free); > } > -- > 2.5.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Oct 15, 2015 at 10:36:08AM +0200, David Herrmann wrote: > Hi > > On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Pretty soon only some drivers will need dev->struct_mutex in their > > gem_free_object callbacks. Hence it's really important to make sure > > everything still keeps getting this right. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > include/drm/drm_gem.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > > index 7a592d7e398b..5b3754864fb0 100644 > > --- a/include/drm/drm_gem.h > > +++ b/include/drm/drm_gem.h > > @@ -142,6 +142,8 @@ drm_gem_object_reference(struct drm_gem_object *obj) > > static inline void > > drm_gem_object_unreference(struct drm_gem_object *obj) > > { > > + WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); > > + > > This doesn't work. mutex_is_locked() is not context-aware, so it does > not check whether the holder is the current thread or someone else. > You *have* to use lockdep if you want negative lock checks. > > In other words, if some other thread holds the mutex in parallel to > this being called, you will trigger the WARN_ON. It's the other way round: If another thread holds the lock, but the caller doesn't, then we won't WARN: It's a false negative, not a false positive. And since lockdep is a serious hog and no one runs it I prefer this way round than the "perfect" lockdep_assert_hold. Not that in the unlocked variant we can't afford this mistake, so there we do rely upon the perfect lockdep locking tracking and use might_lock. -Daniel
Hi On Thu, Oct 15, 2015 at 10:55 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Oct 15, 2015 at 10:36:08AM +0200, David Herrmann wrote: >> Hi >> >> On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> > Pretty soon only some drivers will need dev->struct_mutex in their >> > gem_free_object callbacks. Hence it's really important to make sure >> > everything still keeps getting this right. >> > >> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> > --- >> > include/drm/drm_gem.h | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h >> > index 7a592d7e398b..5b3754864fb0 100644 >> > --- a/include/drm/drm_gem.h >> > +++ b/include/drm/drm_gem.h >> > @@ -142,6 +142,8 @@ drm_gem_object_reference(struct drm_gem_object *obj) >> > static inline void >> > drm_gem_object_unreference(struct drm_gem_object *obj) >> > { >> > + WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); >> > + >> >> This doesn't work. mutex_is_locked() is not context-aware, so it does >> not check whether the holder is the current thread or someone else. >> You *have* to use lockdep if you want negative lock checks. >> >> In other words, if some other thread holds the mutex in parallel to >> this being called, you will trigger the WARN_ON. > > It's the other way round: If another thread holds the lock, but the caller > doesn't, then we won't WARN: It's a false negative, not a false positive. > > And since lockdep is a serious hog and no one runs it I prefer this way > round than the "perfect" lockdep_assert_hold. > > Not that in the unlocked variant we can't afford this mistake, so there we > do rely upon the perfect lockdep locking tracking and use might_lock. Gnah, you're right! I read this as assert, not WARN_ON.. *sigh* Sorry for the noise. David
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 7a592d7e398b..5b3754864fb0 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -142,6 +142,8 @@ drm_gem_object_reference(struct drm_gem_object *obj) static inline void drm_gem_object_unreference(struct drm_gem_object *obj) { + WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); + if (obj != NULL) kref_put(&obj->refcount, drm_gem_object_free); }
Pretty soon only some drivers will need dev->struct_mutex in their gem_free_object callbacks. Hence it's really important to make sure everything still keeps getting this right. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- include/drm/drm_gem.h | 2 ++ 1 file changed, 2 insertions(+)