Patchwork drm: Don't complain too much about struct_mutex.

login
register
mail settings
Submitter Daniel Vetter
Date July 15, 2017, 9:53 a.m.
Message ID <20170715095328.25671-1-daniel.vetter@ffwll.ch>
Download mbox | patch
Permalink /patch/9842249/
State New
Headers show

Comments

Daniel Vetter - July 15, 2017, 9:53 a.m.
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 <hdegoede@redhat.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Jiri Slaby <jirislaby@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_gem.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Chris Wilson - July 15, 2017, 9:59 a.m.
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 <hdegoede@redhat.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Jiri Slaby <jirislaby@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  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(&dev->struct_mutex);
>  
>         if (dev->driver->gem_free_object_unlocked)
>                 kref_put(&obj->refcount, drm_gem_object_free);
> -       else if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
> +       else {
> +               might_lock(&dev->struct_mutex);
> +               if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
>                                 &dev->struct_mutex))
> -               mutex_unlock(&dev->struct_mutex);
> +                       mutex_unlock(&dev->struct_mutex);
> +       }
>  }
>  EXPORT_SYMBOL(drm_gem_object_put_unlocked);
Chris Wilson - July 15, 2017, 11:02 a.m.
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 <hdegoede@redhat.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Jiri Slaby <jirislaby@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

But fwiw, the patch isn't wrong and fixes a false positive, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

>  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(&dev->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(&obj->refcount, drm_gem_object_free);
> -       else if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
> +       else {
> +               might_lock(&dev->struct_mutex);
> +               if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
>                                 &dev->struct_mutex))
> -               mutex_unlock(&dev->struct_mutex);
> +                       mutex_unlock(&dev->struct_mutex);
> +       }
>  }
>  EXPORT_SYMBOL(drm_gem_object_put_unlocked);
Daniel Vetter - July 18, 2017, 7:01 a.m.
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 <hdegoede@redhat.com>
> > Cc: Ben Skeggs <bskeggs@redhat.com>
> > Cc: Jiri Slaby <jirislaby@gmail.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> But fwiw, the patch isn't wrong and fixes a false positive, so
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> >  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(&dev->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(&obj->refcount, drm_gem_object_free);
> > -       else if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
> > +       else {
> > +               might_lock(&dev->struct_mutex);
> > +               if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
> >                                 &dev->struct_mutex))
> > -               mutex_unlock(&dev->struct_mutex);
> > +                       mutex_unlock(&dev->struct_mutex);
> > +       }
> >  }
> >  EXPORT_SYMBOL(drm_gem_object_put_unlocked);
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch

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(&dev->struct_mutex);
 
 	if (dev->driver->gem_free_object_unlocked)
 		kref_put(&obj->refcount, drm_gem_object_free);
-	else if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
+	else {
+		might_lock(&dev->struct_mutex);
+		if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
 				&dev->struct_mutex))
-		mutex_unlock(&dev->struct_mutex);
+			mutex_unlock(&dev->struct_mutex);
+	}
 }
 EXPORT_SYMBOL(drm_gem_object_put_unlocked);