[PATCH] drm: Give the DRM device's anon_inode a unique lockclass for its mmap_rswem

[Daniel Vetter]

On Mon, Dec 11, 2017 at 11:39 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> Teach lockdep to track the device's internal mmapping separately
> from the generic lockclass over all other inodes. Since this is device
> private we wish to allow a different locking hierarchy than is typified
> by the requirement for the mmap_rwsem being the outermost lock for
> handling pagefaults. By giving the internal mmap_rwsem a distinct
> lockclass, lockdep can identify it and learn/enforce its distinct locking
> requirements.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104209
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>

I think both the commit message and comment are a bit too fluffy - the
critical bit is that we're biting ourselves on gtt mmaps from
usersptr, and that's explicitly not allowed exactly because it would
deadlock.

I'm also not sure it's a good idea to implement this in generic code,
since this is a very i915 specific issue, and other drivers (who might
be a lot less sloppy here) will now no longer get reports about this
deadlock.

Aside from that I'm not really sure why you think the bugzilla link is
a false positive: The mapping->rwsem is the one for the gtt in both
cases I think.
-Daniel

> ---
>  drivers/gpu/drm/drm_drv.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 9acc1e157813..21ad06c3d684 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -393,6 +393,7 @@ static struct file_system_type drm_fs_type = {
>
>  static struct inode *drm_fs_inode_new(void)
>  {
> +       static struct lock_class_key lockclass;
>         struct inode *inode;
>         int r;
>
> @@ -403,8 +404,22 @@ static struct inode *drm_fs_inode_new(void)
>         }
>
>         inode = alloc_anon_inode(drm_fs_mnt->mnt_sb);
> -       if (IS_ERR(inode))
> +       if (IS_ERR(inode)) {
>                 simple_release_fs(&drm_fs_mnt, &drm_fs_cnt);
> +               return inode;
> +       }
> +
> +       /*
> +        * Teach lockdep to track the device's internal mmapping separately
> +        * from all other inodes. Since this is device private we wish to
> +        * allow a different locking hierarchy than is typified by the
> +        * requirement for the mmap_rwsem being the outermost lock for
> +        * handling pagefaults. By giving the internal mmap_rwsem a distinct
> +        * lockclass, lockdep can identify it and thereby learn and enforce its
> +        * distinct locking requirements.
> +        */
> +       lockdep_set_class_and_name(&inode->i_mapping->i_mmap_rwsem,
> +                                  &lockclass, "drm_fs_inode");
>
>         return inode;
>  }
> --
> 2.15.1
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch