Re: [Intel-gfx] [PATCH] drm: Don't complain too much about struct_mutex.

[Daniel Vetter]

On Sat, Jul 15, 2017 at 12:02:47PM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2017-07-15 10:53:28)
> > For modern drivers the DRM core doesn't use struct_mutex at all, which
> > means it's defacto a driver-private lock. But since we still need it
> > for legacy drivers we can't initialize it in drivers, which means all
> > the different instances share one lockdep key. Despite that they might
> > be placed in totally different places in the locking hierarchy.
> > 
> > This results in a lot of bogus lockdep splats when running stuff on
> > systems with multiple gpus. Partially remedy the situation by only
> > doing might_lock checks on drivers that do use struct_mutex still for
> > gem locking.
> > 
> > A more complete solution would be to do the mutex_init in the drm core
> > only for legacy drivers, plus add it to each modern driver that still
> > needs it, which would also give each its own lockdep key. Trying to do
> > that dynamically doesn't work, because lockdep requires it's keys to
> > be statically allocated.
> > 
> > Cc: Hans de Goede 
> > Cc: Ben Skeggs 
> > Cc: Jiri Slaby 
> > Cc: Peter Zijlstra 
> > Cc: Ingo Molnar 
> > Signed-off-by: Daniel Vetter 
> 
> But fwiw, the patch isn't wrong and fixes a false positive, so
> Reviewed-by: Chris Wilson 
> 
> >  drivers/gpu/drm/drm_gem.c | 8 +---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 8dc11064253d..9663a79dd363 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -826,13 +826,15 @@ drm_gem_object_put_unlocked(struct drm_gem_object 
> > *obj)
> > return;
> >  
> > dev = obj->dev;
> > -   might_lock(>struct_mutex);
> >  
> > if (dev->driver->gem_free_object_unlocked)
> 
> coding-style nit, if one branch needs {} they all need {}.
> (The real reason why I didn't want to move might_lock around in this
> function ;)

Fixed while applying, thanks for your review.
-Daniel
> 
> > kref_put(>refcount, drm_gem_object_free);
> > -   else if (kref_put_mutex(>refcount, drm_gem_object_free,
> > +   else {
> > +   might_lock(>struct_mutex);
> > +   if (kref_put_mutex(>refcount, drm_gem_object_free,
> > >struct_mutex))
> > -   mutex_unlock(>struct_mutex);
> > +   mutex_unlock(>struct_mutex);
> > +   }
> >  }
> >  EXPORT_SYMBOL(drm_gem_object_put_unlocked);
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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

Re: [Intel-gfx] [PATCH] drm: Don't complain too much about struct_mutex.

[Peter Zijlstra]

On Mon, Jul 17, 2017 at 05:06:42PM +0200, Daniel Vetter wrote:
> On Mon, Jul 17, 2017 at 11:35 AM, Peter Zijlstra  wrote:
> > On Sat, Jul 15, 2017 at 11:53:28AM +0200, Daniel Vetter wrote:
> >> A more complete solution would be to do the mutex_init in the drm core
> >> only for legacy drivers, plus add it to each modern driver that still
> >> needs it, which would also give each its own lockdep key. Trying to do
> >> that dynamically doesn't work, because lockdep requires it's keys to
> >> be statically allocated.
> >
> > Would something like the below work? Its not pretty, but would give each
> > user of drm_dev_init() its own key.
> 
> We're very close to getting rid of struct_mutex, neither nouveau.ko
> nor i915.ko use it, and only about 4 drivers really still need it (out
> of about 30+). I think the minimal patch is enough, and then not
> initializing the mutex to prevent more users croping up again in the
> future.

Whatever you want of course. I was just going by that one paragraphy I
quoted :-)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: Don't complain too much about struct_mutex.

[Daniel Vetter]

On Mon, Jul 17, 2017 at 11:35 AM, Peter Zijlstra  wrote:
> On Sat, Jul 15, 2017 at 11:53:28AM +0200, Daniel Vetter wrote:
>> A more complete solution would be to do the mutex_init in the drm core
>> only for legacy drivers, plus add it to each modern driver that still
>> needs it, which would also give each its own lockdep key. Trying to do
>> that dynamically doesn't work, because lockdep requires it's keys to
>> be statically allocated.
>
> Would something like the below work? Its not pretty, but would give each
> user of drm_dev_init() its own key.

We're very close to getting rid of struct_mutex, neither nouveau.ko
nor i915.ko use it, and only about 4 drivers really still need it (out
of about 30+). I think the minimal patch is enough, and then not
initializing the mutex to prevent more users croping up again in the
future.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: Don't complain too much about struct_mutex.

[Peter Zijlstra]

On Sat, Jul 15, 2017 at 11:53:28AM +0200, Daniel Vetter wrote:
> A more complete solution would be to do the mutex_init in the drm core
> only for legacy drivers, plus add it to each modern driver that still
> needs it, which would also give each its own lockdep key. Trying to do
> that dynamically doesn't work, because lockdep requires it's keys to
> be statically allocated.

Would something like the below work? Its not pretty, but would give each
user of drm_dev_init() its own key.




diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 37b8ad3e30d8..06cc32012728 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -478,9 +478,10 @@ static void drm_fs_inode_free(struct inode *inode)
  * RETURNS:
  * 0 on success, or error code on failure.
  */
-int drm_dev_init(struct drm_device *dev,
-struct drm_driver *driver,
-struct device *parent)
+int __drm_dev_init(struct drm_device *dev,
+  struct drm_driver *driver,
+  struct device *parent,
+  struct lock_class_key *key)
 {
int ret;
 
@@ -496,7 +497,7 @@ int drm_dev_init(struct drm_device *dev,
 
spin_lock_init(>buf_lock);
spin_lock_init(>event_lock);
-   mutex_init(>struct_mutex);
+   __mutex_init(>struct_mutex, ">struct_mutex", key);
mutex_init(>filelist_mutex);
mutex_init(>ctxlist_mutex);
mutex_init(>master_mutex);
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 53b98321df9b..cda11e97024e 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -531,9 +531,17 @@ void drm_printk(const char *level, unsigned int category,
const char *format, ...);
 extern unsigned int drm_debug;
 
-int drm_dev_init(struct drm_device *dev,
-struct drm_driver *driver,
-struct device *parent);
+int __drm_dev_init(struct drm_device *dev,
+  struct drm_driver *driver,
+  struct device *parent,
+  struct lock_class_key *key);
+
+#define drm_dev_init(_dev, _driver, _parent)   \
+({ \
+   static struct lock_class_key __key; \
+   __drm_dev_init((_dev), (_driver), (_parent), &__key);   \
+})
+
 void drm_dev_fini(struct drm_device *dev);
 
 struct drm_device *drm_dev_alloc(struct drm_driver *driver,
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: Don't complain too much about struct_mutex.

[Chris Wilson]

Quoting Daniel Vetter (2017-07-15 10:53:28)
> For modern drivers the DRM core doesn't use struct_mutex at all, which
> means it's defacto a driver-private lock. But since we still need it
> for legacy drivers we can't initialize it in drivers, which means all
> the different instances share one lockdep key. Despite that they might
> be placed in totally different places in the locking hierarchy.
> 
> This results in a lot of bogus lockdep splats when running stuff on
> systems with multiple gpus. Partially remedy the situation by only
> doing might_lock checks on drivers that do use struct_mutex still for
> gem locking.
> 
> A more complete solution would be to do the mutex_init in the drm core
> only for legacy drivers, plus add it to each modern driver that still
> needs it, which would also give each its own lockdep key. Trying to do
> that dynamically doesn't work, because lockdep requires it's keys to
> be statically allocated.
> 
> Cc: Hans de Goede 
> Cc: Ben Skeggs 
> Cc: Jiri Slaby 
> Cc: Peter Zijlstra 
> Cc: Ingo Molnar 
> Signed-off-by: Daniel Vetter 

But fwiw, the patch isn't wrong and fixes a false positive, so
Reviewed-by: Chris Wilson 

>  drivers/gpu/drm/drm_gem.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 8dc11064253d..9663a79dd363 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -826,13 +826,15 @@ drm_gem_object_put_unlocked(struct drm_gem_object *obj)
> return;
>  
> dev = obj->dev;
> -   might_lock(>struct_mutex);
>  
> if (dev->driver->gem_free_object_unlocked)

coding-style nit, if one branch needs {} they all need {}.
(The real reason why I didn't want to move might_lock around in this
function ;)

> kref_put(>refcount, drm_gem_object_free);
> -   else if (kref_put_mutex(>refcount, drm_gem_object_free,
> +   else {
> +   might_lock(>struct_mutex);
> +   if (kref_put_mutex(>refcount, drm_gem_object_free,
> >struct_mutex))
> -   mutex_unlock(>struct_mutex);
> +   mutex_unlock(>struct_mutex);
> +   }
>  }
>  EXPORT_SYMBOL(drm_gem_object_put_unlocked);
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: Don't complain too much about struct_mutex.

[Chris Wilson]

Quoting Daniel Vetter (2017-07-15 10:53:28)
> For modern drivers the DRM core doesn't use struct_mutex at all, which
> means it's defacto a driver-private lock. But since we still need it
> for legacy drivers we can't initialize it in drivers, which means all
> the different instances share one lockdep key. Despite that they might
> be placed in totally different places in the locking hierarchy.
> 
> This results in a lot of bogus lockdep splats when running stuff on
> systems with multiple gpus. Partially remedy the situation by only
> doing might_lock checks on drivers that do use struct_mutex still for
> gem locking.
> 
> A more complete solution would be to do the mutex_init in the drm core
> only for legacy drivers, plus add it to each modern driver that still
> needs it, which would also give each its own lockdep key. Trying to do
> that dynamically doesn't work, because lockdep requires it's keys to
> be statically allocated.

I placed it in drm_driver which is static to get around that, did you
see the patch adding drm_driver_class? I didn't choose that path since
this might_lock clearly belongs to kref_put_mutex...

Similarly might_lock can also include might_sleep

+#define lock_is_mutex(lock) \
+   __builtin_types_compatible_p(typeof(*lock), struct mutex)
+
 # define might_lock(lock)  \
 do {   \
+   might_sleep_if(lock_is_mutex(lock));


> Cc: Hans de Goede 
> Cc: Ben Skeggs 
> Cc: Jiri Slaby 
> Cc: Peter Zijlstra 
> Cc: Ingo Molnar 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_gem.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 8dc11064253d..9663a79dd363 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -826,13 +826,15 @@ drm_gem_object_put_unlocked(struct drm_gem_object *obj)
> return;
>  
> dev = obj->dev;
> -   might_lock(>struct_mutex);
>  
> if (dev->driver->gem_free_object_unlocked)
> kref_put(>refcount, drm_gem_object_free);
> -   else if (kref_put_mutex(>refcount, drm_gem_object_free,
> +   else {
> +   might_lock(>struct_mutex);
> +   if (kref_put_mutex(>refcount, drm_gem_object_free,
> >struct_mutex))
> -   mutex_unlock(>struct_mutex);
> +   mutex_unlock(>struct_mutex);
> +   }
>  }
>  EXPORT_SYMBOL(drm_gem_object_put_unlocked);
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx