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

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

[Chris Wilson]

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@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for drm: Give the DRM device's anon_inode a unique lockclass for its mmap_rswem

[Patchwork]

== Series Details ==

Series: drm: Give the DRM device's anon_inode a unique lockclass for its mmap_rswem
URL   : https://patchwork.freedesktop.org/series/35171/
State : success

== Summary ==

Series 35171v1 drm: Give the DRM device's anon_inode a unique lockclass for its mmap_rswem
https://patchwork.freedesktop.org/api/1.0/series/35171/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> DMESG-WARN (fi-elk-e7500) fdo#103989 +1
Test gem_exec_fence:
        Subgroup nb-await-default:
                dmesg-fail -> PASS       (fi-pnv-d510)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                fail       -> PASS       (fi-gdg-551) fdo#102618
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> FAIL       (fi-hsw-4770) fdo#103375
                pass       -> DMESG-WARN (fi-kbl-r) fdo#104172 +1
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#102618 https://bugs.freedesktop.org/show_bug.cgi?id=102618
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#104172 https://bugs.freedesktop.org/show_bug.cgi?id=104172
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:440s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:450s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:384s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:523s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:284s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:505s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:515s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:493s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:482s
fi-elk-e7500     total:224  pass:163  dwarn:15  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:178  dwarn:1   dfail:0   fail:1   skip:108 time:271s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:539s
fi-hsw-4770      total:288  pass:260  dwarn:0   dfail:0   fail:1   skip:27  time:353s
fi-hsw-4770r     total:288  pass:224  dwarn:0   dfail:0   fail:0   skip:64  time:262s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:395s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-ivb-3770      total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:449s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:490s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:531s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:476s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:532s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:604s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:548s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:562s
fi-skl-6700k     total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:527s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:503s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:451s
fi-snb-2520m     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:555s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:423s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:604s
fi-glk-dsi       total:288  pass:152  dwarn:1   dfail:4   fail:0   skip:131 time:294s

6c0bf00e417c7d37409f87be775ff98c76505350 drm-tip: 2017y-12m-10d-18h-32m-45s UTC integration manifest
a05561a3319e drm: Give the DRM device's anon_inode a unique lockclass for its mmap_rswem

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7462/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm: Give the DRM device's anon_inode a unique lockclass for its mmap_rswem

[Patchwork]

== Series Details ==

Series: drm: Give the DRM device's anon_inode a unique lockclass for its mmap_rswem
URL   : https://patchwork.freedesktop.org/series/35171/
State : success

== Summary ==

Test kms_cursor_crc:
        Subgroup cursor-128x128-onscreen:
                skip       -> PASS       (shard-snb)
        Subgroup cursor-64x64-suspend:
                pass       -> INCOMPLETE (shard-hsw) fdo#103540
Test kms_plane:
        Subgroup plane-panning-top-left-pipe-b-planes:
                skip       -> PASS       (shard-snb)
Test kms_chv_cursor_fail:
        Subgroup pipe-a-256x256-top-edge:
                skip       -> PASS       (shard-snb)
Test kms_atomic:
        Subgroup atomic_invalid_params:
                skip       -> PASS       (shard-snb)
Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> INCOMPLETE (shard-hsw) fdo#102707

fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707

shard-hsw        total:2616 pass:1490 dwarn:1   dfail:0   fail:10  skip:1113 time:8801s
shard-snb        total:2692 pass:1310 dwarn:1   dfail:0   fail:11  skip:1370 time:8107s
Blacklisted hosts:
shard-apl        total:2692 pass:1686 dwarn:2   dfail:0   fail:23  skip:981 time:13919s
shard-kbl        total:2692 pass:1808 dwarn:1   dfail:0   fail:23  skip:859 time:10994s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7462/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chris Wilson]

Quoting Daniel Vetter (2017-12-11 17:20:32)
> On Mon, Dec 11, 2017 at 11:39 AM, Chris Wilson <chris@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@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@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.

I was thinking that in a more general sense manipulating of the
vma_manager's inode is independent of the processes's mappings. As such
we do not want to tie the two together and force them to conform to the
same rules, because the core mapping semaphore will be held on entry to
driver code, but the internal mapping will be used from within driver
code.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Mon, Dec 11, 2017 at 6:27 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Daniel Vetter (2017-12-11 17:20:32)
>> On Mon, Dec 11, 2017 at 11:39 AM, Chris Wilson <chris@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@chris-wilson.co.uk>
>> > Cc: Daniel Vetter <daniel.vetter@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.
>
> I was thinking that in a more general sense manipulating of the
> vma_manager's inode is independent of the processes's mappings. As such
> we do not want to tie the two together and force them to conform to the
> same rules, because the core mapping semaphore will be held on entry to
> driver code, but the internal mapping will be used from within driver
> code.

I think they're the same locks really. Maybe I'm missing something,
but I thought the mapping->rwsem we get on mmap/fault is exactly the
one we want/need to use for zap_pte.

Looking at the bugzilla trace I think the deadlock happens when the
i915_gem_userptr_mn_invalidate_range_start callback calls
flush_workqueue for a range that is not itself not allowed to be
userptr-mapped. But because it does that, we end up in a deadlock. I
think if the userptr callback would checkthe range it gets against all
the userptr mappings, we'd avoid this deadlock: userptr is not allowed
to map a gtt range, which means this should avoid calling
flush_workqueue while holding our drm mapping->rwsem.

So there seems to be a real deadlock, at least in my current understanding.

Of course if we'd fix that deadlock we'd still have lockdep
complaining, but maybe the deadlock fix also gets rid of the lockdep
splat (but that would be more rework than just making the flush_work
conditional).
-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