| Message ID | 20200122162651.1111-1-janusz.krzysztofik@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,i-g-t] tests/gem_userptr_blits: Enhance invalid mapping exercise | expand |
On 22/01/20 08:26, Janusz Krzysztofik wrote: > Working with a userptr GEM object backed by any type of mapping to > another GEM object, not only GTT mapping currently examined bu the > test, may cause a currently unavoidable lockdep splat inside the i915 > driver. Then, such operations are expected to fail in advance to > prevent from that badness to happen. > > Extend the scope of the test by adding subtests which exercise other, > non-GTT mapping types. Moreover, also succeed if a failure happens > as soon as a userptr object is created on top of an invalid mapping. > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> > --- > tests/i915/gem_userptr_blits.c | 87 +++++++++++++++++++++++----------- > 1 file changed, 59 insertions(+), 28 deletions(-) > > diff --git a/tests/i915/gem_userptr_blits.c b/tests/i915/gem_userptr_blits.c > index a8d3783f..69e5bd1f 100644 > --- a/tests/i915/gem_userptr_blits.c > +++ b/tests/i915/gem_userptr_blits.c > @@ -60,6 +60,7 @@ > > #include "drm.h" > #include "i915_drm.h" > +#include "i915/gem_mman.h" > > #include "intel_bufmgr.h" > > @@ -577,11 +578,11 @@ static int test_invalid_null_pointer(int fd) > return 0; > } > > -static int test_invalid_gtt_mapping(int fd) > +static int test_invalid_mapping(int fd, uint64_t flags) > { > - struct drm_i915_gem_mmap_gtt arg; > + struct drm_i915_gem_mmap_offset arg; > uint32_t handle; > - char *gtt, *map; > + char *offset, *map; > > /* Anonymous mapping to find a hole */ > map = mmap(NULL, sizeof(linear) + 2 * PAGE_SIZE, > @@ -602,39 +603,46 @@ static int test_invalid_gtt_mapping(int fd) > igt_assert_eq(copy(fd, handle, handle), 0); > gem_close(fd, handle); > > - /* GTT mapping */ > + /* object mapping */ > memset(&arg, 0, sizeof(arg)); > arg.handle = create_bo(fd, 0); > - do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &arg); > - gtt = mmap(map + PAGE_SIZE, sizeof(linear), > - PROT_READ | PROT_WRITE, > - MAP_SHARED | MAP_FIXED, > - fd, arg.offset); > - igt_assert(gtt == map + PAGE_SIZE); > + arg.flags = flags; > + do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_OFFSET, &arg); > + offset = mmap(map + PAGE_SIZE, sizeof(linear), PROT_READ | PROT_WRITE, > + MAP_SHARED | MAP_FIXED, fd, arg.offset); > + igt_assert(offset == map + PAGE_SIZE); > gem_close(fd, arg.handle); > - igt_assert(((unsigned long)gtt & (PAGE_SIZE - 1)) == 0); > + igt_assert(((unsigned long)offset & (PAGE_SIZE - 1)) == 0); > igt_assert((sizeof(linear) & (PAGE_SIZE - 1)) == 0); > > - gem_userptr(fd, gtt, sizeof(linear), 0, userptr_flags, &handle); > - igt_assert_eq(copy(fd, handle, handle), -EFAULT); > - gem_close(fd, handle); > + if (!__gem_userptr(fd, offset, sizeof(linear), 0, userptr_flags, > + &handle)) { Not sure I follow why you converted this to an if. Do we expect the userptr IOCTL not to work? Could you add a small comment. Antonio > + igt_assert_eq(copy(fd, handle, handle), -EFAULT); > + gem_close(fd, handle); > + } > > - gem_userptr(fd, gtt, PAGE_SIZE, 0, userptr_flags, &handle); > - igt_assert_eq(copy(fd, handle, handle), -EFAULT); > - gem_close(fd, handle); > + if (!__gem_userptr(fd, offset, PAGE_SIZE, 0, userptr_flags, &handle)) { > + igt_assert_eq(copy(fd, handle, handle), -EFAULT); > + gem_close(fd, handle); > + } > > - gem_userptr(fd, gtt + sizeof(linear) - PAGE_SIZE, PAGE_SIZE, 0, userptr_flags, &handle); > - igt_assert_eq(copy(fd, handle, handle), -EFAULT); > - gem_close(fd, handle); > + if (!__gem_userptr(fd, offset + sizeof(linear) - PAGE_SIZE, PAGE_SIZE, > + 0, userptr_flags, &handle)) { > + igt_assert_eq(copy(fd, handle, handle), -EFAULT); > + gem_close(fd, handle); > + } > > /* boundaries */ > - gem_userptr(fd, map, 2*PAGE_SIZE, 0, userptr_flags, &handle); > - igt_assert_eq(copy(fd, handle, handle), -EFAULT); > - gem_close(fd, handle); > + if (!__gem_userptr(fd, map, 2 * PAGE_SIZE, 0, userptr_flags, &handle)) { > + igt_assert_eq(copy(fd, handle, handle), -EFAULT); > + gem_close(fd, handle); > + } > > - gem_userptr(fd, map + sizeof(linear), 2*PAGE_SIZE, 0, userptr_flags, &handle); > - igt_assert_eq(copy(fd, handle, handle), -EFAULT); > - gem_close(fd, handle); > + if (!__gem_userptr(fd, map + sizeof(linear), 2 * PAGE_SIZE, 0, > + userptr_flags, &handle)) { > + igt_assert_eq(copy(fd, handle, handle), -EFAULT); > + gem_close(fd, handle); > + } > > munmap(map, sizeof(linear) + 2*PAGE_SIZE); > > @@ -2009,8 +2017,31 @@ igt_main_args("c:", NULL, help_str, opt_handler, NULL) > igt_subtest("invalid-null-pointer") > test_invalid_null_pointer(fd); > > - igt_subtest("invalid-gtt-mapping") > - test_invalid_gtt_mapping(fd); > + igt_describe("Verify userptr on top of GTT mapping to GEM object will fail"); > + igt_subtest("invalid-gtt-mapping") { > + gem_require_mappable_ggtt(fd); > + test_invalid_mapping(fd, I915_MMAP_OFFSET_GTT); > + } > + igt_subtest_group { > + igt_fixture > + igt_require(gem_has_mmap_offset(fd)); > + > + igt_describe("Verify userptr on top of CPU mapping to GEM object will fail"); > + igt_subtest("invalid-wb-mapping") > + test_invalid_mapping(fd, I915_MMAP_OFFSET_WB); > + > + igt_subtest_group { > + igt_fixture > + igt_require(gem_mmap_offset__has_wc(fd)); > + > + igt_describe("Verify userptr on top of coherent mapping to GEM object will fail"); > + igt_subtest("invalid-wc-mapping") > + test_invalid_mapping(fd, I915_MMAP_OFFSET_WC); > + igt_describe("Verify userptr on top of uncached mapping to GEM object will fail"); > + igt_subtest("invalid-uc-mapping") > + test_invalid_mapping(fd, I915_MMAP_OFFSET_UC); > + } > + } > > igt_subtest("forked-access") > test_forked_access(fd); >
Hi Antonio, On Tuesday, January 28, 2020 1:59:00 AM CET Antonio Argenziano wrote: > > On 22/01/20 08:26, Janusz Krzysztofik wrote: > > Working with a userptr GEM object backed by any type of mapping to > > another GEM object, not only GTT mapping currently examined bu the > > test, may cause a currently unavoidable lockdep splat inside the i915 > > driver. Then, such operations are expected to fail in advance to > > prevent from that badness to happen. > > > > Extend the scope of the test by adding subtests which exercise other, > > non-GTT mapping types. Moreover, also succeed if a failure happens > > as soon as a userptr object is created on top of an invalid mapping. > > > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> > > --- > > tests/i915/gem_userptr_blits.c | 87 +++++++++++++++++++++++----------- > > 1 file changed, 59 insertions(+), 28 deletions(-) > > > > diff --git a/tests/i915/gem_userptr_blits.c b/tests/i915/gem_userptr_blits.c > > index a8d3783f..69e5bd1f 100644 > > --- a/tests/i915/gem_userptr_blits.c > > +++ b/tests/i915/gem_userptr_blits.c > > @@ -60,6 +60,7 @@ > > > > #include "drm.h" > > #include "i915_drm.h" > > +#include "i915/gem_mman.h" > > > > #include "intel_bufmgr.h" > > > > @@ -577,11 +578,11 @@ static int test_invalid_null_pointer(int fd) > > return 0; > > } > > > > -static int test_invalid_gtt_mapping(int fd) > > +static int test_invalid_mapping(int fd, uint64_t flags) > > { > > - struct drm_i915_gem_mmap_gtt arg; > > + struct drm_i915_gem_mmap_offset arg; > > uint32_t handle; > > - char *gtt, *map; > > + char *offset, *map; > > > > /* Anonymous mapping to find a hole */ > > map = mmap(NULL, sizeof(linear) + 2 * PAGE_SIZE, > > @@ -602,39 +603,46 @@ static int test_invalid_gtt_mapping(int fd) > > igt_assert_eq(copy(fd, handle, handle), 0); > > gem_close(fd, handle); > > > > - /* GTT mapping */ > > + /* object mapping */ > > memset(&arg, 0, sizeof(arg)); > > arg.handle = create_bo(fd, 0); > > - do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &arg); > > - gtt = mmap(map + PAGE_SIZE, sizeof(linear), > > - PROT_READ | PROT_WRITE, > > - MAP_SHARED | MAP_FIXED, > > - fd, arg.offset); > > - igt_assert(gtt == map + PAGE_SIZE); > > + arg.flags = flags; > > + do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_OFFSET, &arg); > > + offset = mmap(map + PAGE_SIZE, sizeof(linear), PROT_READ | PROT_WRITE, > > + MAP_SHARED | MAP_FIXED, fd, arg.offset); > > + igt_assert(offset == map + PAGE_SIZE); > > gem_close(fd, arg.handle); > > - igt_assert(((unsigned long)gtt & (PAGE_SIZE - 1)) == 0); > > + igt_assert(((unsigned long)offset & (PAGE_SIZE - 1)) == 0); > > igt_assert((sizeof(linear) & (PAGE_SIZE - 1)) == 0); > > > > - gem_userptr(fd, gtt, sizeof(linear), 0, userptr_flags, &handle); > > - igt_assert_eq(copy(fd, handle, handle), -EFAULT); > > - gem_close(fd, handle); > > + if (!__gem_userptr(fd, offset, sizeof(linear), 0, userptr_flags, > > + &handle)) { > > Not sure I follow why you converted this to an if. Do we expect the > userptr IOCTL not to work? Could you add a small comment. Please have a look at Chris' response to CI report [1] for more information on what is expected. Thanks, Janusz [1] https://lists.freedesktop.org/archives/igt-dev/2020-January/019515.html > > Antonio > > > + igt_assert_eq(copy(fd, handle, handle), -EFAULT); > > + gem_close(fd, handle); > > + } > > > > - gem_userptr(fd, gtt, PAGE_SIZE, 0, userptr_flags, &handle); > > - igt_assert_eq(copy(fd, handle, handle), -EFAULT); > > - gem_close(fd, handle); > > + if (!__gem_userptr(fd, offset, PAGE_SIZE, 0, userptr_flags, &handle)) { > > + igt_assert_eq(copy(fd, handle, handle), -EFAULT); > > + gem_close(fd, handle); > > + } > > > > - gem_userptr(fd, gtt + sizeof(linear) - PAGE_SIZE, PAGE_SIZE, 0, userptr_flags, &handle); > > - igt_assert_eq(copy(fd, handle, handle), -EFAULT); > > - gem_close(fd, handle); > > + if (!__gem_userptr(fd, offset + sizeof(linear) - PAGE_SIZE, PAGE_SIZE, > > + 0, userptr_flags, &handle)) { > > + igt_assert_eq(copy(fd, handle, handle), -EFAULT); > > + gem_close(fd, handle); > > + } > > > > /* boundaries */ > > - gem_userptr(fd, map, 2*PAGE_SIZE, 0, userptr_flags, &handle); > > - igt_assert_eq(copy(fd, handle, handle), -EFAULT); > > - gem_close(fd, handle); > > + if (!__gem_userptr(fd, map, 2 * PAGE_SIZE, 0, userptr_flags, &handle)) { > > + igt_assert_eq(copy(fd, handle, handle), -EFAULT); > > + gem_close(fd, handle); > > + } > > > > - gem_userptr(fd, map + sizeof(linear), 2*PAGE_SIZE, 0, userptr_flags, &handle); > > - igt_assert_eq(copy(fd, handle, handle), -EFAULT); > > - gem_close(fd, handle); > > + if (!__gem_userptr(fd, map + sizeof(linear), 2 * PAGE_SIZE, 0, > > + userptr_flags, &handle)) { > > + igt_assert_eq(copy(fd, handle, handle), -EFAULT); > > + gem_close(fd, handle); > > + } > > > > munmap(map, sizeof(linear) + 2*PAGE_SIZE); > > > > @@ -2009,8 +2017,31 @@ igt_main_args("c:", NULL, help_str, opt_handler, NULL) > > igt_subtest("invalid-null-pointer") > > test_invalid_null_pointer(fd); > > > > - igt_subtest("invalid-gtt-mapping") > > - test_invalid_gtt_mapping(fd); > > + igt_describe("Verify userptr on top of GTT mapping to GEM object will fail"); > > + igt_subtest("invalid-gtt-mapping") { > > + gem_require_mappable_ggtt(fd); > > + test_invalid_mapping(fd, I915_MMAP_OFFSET_GTT); > > + } > > + igt_subtest_group { > > + igt_fixture > > + igt_require(gem_has_mmap_offset(fd)); > > + > > + igt_describe("Verify userptr on top of CPU mapping to GEM object will fail"); > > + igt_subtest("invalid-wb-mapping") > > + test_invalid_mapping(fd, I915_MMAP_OFFSET_WB); > > + > > + igt_subtest_group { > > + igt_fixture > > + igt_require(gem_mmap_offset__has_wc(fd)); > > + > > + igt_describe("Verify userptr on top of coherent mapping to GEM object will fail"); > > + igt_subtest("invalid-wc-mapping") > > + test_invalid_mapping(fd, I915_MMAP_OFFSET_WC); > > + igt_describe("Verify userptr on top of uncached mapping to GEM object will fail"); > > + igt_subtest("invalid-uc-mapping") > > + test_invalid_mapping(fd, I915_MMAP_OFFSET_UC); > > + } > > + } > > > > igt_subtest("forked-access") > > test_forked_access(fd); > > >
(adding intel-gfx to the loop) Hi Chris, On Wednesday, January 29, 2020 12:24:39 PM CET Chris Wilson wrote: > Quoting Patchwork (2020-01-23 20:22:25) > > == Series Details == > > > > Series: tests/gem_userptr_blits: Enhance invalid mapping exercise > > URL : https://patchwork.freedesktop.org/series/72411/ > > State : success > > > > == Summary == > > > > CI Bug Log - changes from CI_DRM_7794_full -> IGTPW_3969_full > > ==================================================== > > > > Summary > > ------- > > > > **SUCCESS** > > > > No regressions found. > > > > External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3969/ index.html > > > > New tests > > --------- > > > > New tests have been introduced between CI_DRM_7794_full and IGTPW_3969_full: > > > > ### New IGT tests (3) ### > > > > * igt@gem_userptr_blits@invalid-uc-mapping: > > - Statuses : 7 pass(s) > > - Exec time: [0.01, 0.02] s > > > > * igt@gem_userptr_blits@invalid-wb-mapping: > > - Statuses : 7 pass(s) > > - Exec time: [0.01, 0.02] s > > > > * igt@gem_userptr_blits@invalid-wc-mapping: > > - Statuses : 7 pass(s) > > - Exec time: [0.01, 0.02] s > > Ok, we need a bit more work to trigger the lockdep loop. We need a > cancel_userptr of the same object that is faulted via mmap-offset. Does that mean it would be helpful if we could trigger that lockdep loop form gem_userptr_blits@invalid-*-mapping so we should focus on that? My confusion also resulted partially from me not being able to find anything like cancel_userptr in the current code. Anyway, I've been trying to find out a solution. While working on that, I found there was a cancel_userptr() function but it had been dropped by commit 484d9a844d0d ("drm/i915/userptr: Avoid struct_mutex recursion for mmu_invalidate_range_start"). Other than that, it was not clear for me if what you tried to say was we needed that cancel_userptr for triggering the lockdep loop, or we needed it for having the issue resolved. Anyway, taking into account that cancel_userptr() is no longer available, could you please give me another hint before I spend more time possibly going in a wrong direction? Thanks, Janusz > > We're basically looking for the equivalent of this > > <4> [175.968441] ====================================================== > <4> [175.968444] WARNING: possible circular locking dependency detected > <4> [175.968447] 5.5.0-CI-CI_DRM_7828+ #1 Tainted: G U > <4> [175.968449] ------------------------------------------------------ > <4> [175.968452] gem_userptr_bli/1564 is trying to acquire lock: > <4> [175.968455] ffff888208f49580 (&vm->mutex){+.+.}, at: i915_vma_unbind+0xae/0x110 [i915] > <4> [175.968527] > but task is already holding lock: > <4> [175.968529] ffffffff82664d40 (mmu_notifier_invalidate_range_start){+. +.}, at: unmap_vmas+0x0/0x150 > <4> [175.968535] > which lock already depends on the new lock. > <4> [175.968538] > the existing dependency chain (in reverse order) is: > <4> [175.968541] > -> #2 (mmu_notifier_invalidate_range_start){+.+.}: > <4> [175.968546] page_mkclean_one+0xda/0x210 > <4> [175.968548] rmap_walk_file+0xff/0x260 > <4> [175.968551] page_mkclean+0x9f/0xb0 > <4> [175.968555] clear_page_dirty_for_io+0xa2/0x2f0 > <4> [175.968559] mpage_submit_page+0x1a/0x70 > <4> [175.968561] mpage_process_page_bufs+0xe7/0x110 > <4> [175.968564] mpage_prepare_extent_to_map+0x1d2/0x2b0 > <4> [175.968568] ext4_writepages+0x5ba/0x12b0 > <4> [175.968571] do_writepages+0x46/0xe0 > <4> [175.968573] __filemap_fdatawrite_range+0xc6/0x100 > <4> [175.968576] file_write_and_wait_range+0x3c/0x90 > <4> [175.968579] ext4_sync_file+0x1a4/0x540 > <4> [175.968582] do_fsync+0x33/0x60 > <4> [175.968584] __x64_sys_fsync+0xb/0x10 > <4> [175.968587] do_syscall_64+0x4f/0x220 > <4> [175.968591] entry_SYSCALL_64_after_hwframe+0x49/0xbe > <4> [175.968594] > -> #1 (&mapping->i_mmap_rwsem){++++}: > <4> [175.968599] down_write+0x33/0x70 > <4> [175.968601] unmap_mapping_pages+0x48/0x130 > <4> [175.968671] i915_vma_revoke_mmap.part.37+0x66/0x190 [i915] > <4> [175.968715] fence_update+0xfd/0x2d0 [i915] > <4> [175.968759] __i915_vma_unbind+0x1eb/0x530 [i915] > <4> [175.968803] i915_vma_release+0x101/0x220 [i915] > <4> [175.968843] __i915_gem_free_objects+0x113/0x530 [i915] > <4> [175.968886] i915_gem_create_ioctl+0x12/0x40 [i915] > <4> [175.968890] drm_ioctl_kernel+0xad/0xf0 > <4> [175.968893] drm_ioctl+0x2e1/0x390 > <4> [175.968896] do_vfs_ioctl+0x9c/0x730 > <4> [175.968899] ksys_ioctl+0x35/0x60 > <4> [175.968901] __x64_sys_ioctl+0x11/0x20 > <4> [175.968904] do_syscall_64+0x4f/0x220 > <4> [175.968906] entry_SYSCALL_64_after_hwframe+0x49/0xbe > <4> [175.968909] > -> #0 (&vm->mutex){+.+.}: > <4> [175.968914] __lock_acquire+0x1328/0x15d0 > <4> [175.968916] lock_acquire+0xa7/0x1c0 > <4> [175.968919] __mutex_lock+0x9a/0x9c0 > <4> [175.968962] i915_vma_unbind+0xae/0x110 [i915] > <4> [175.969004] i915_gem_object_unbind+0x1dc/0x400 [i915] > <4> [175.969045] userptr_mn_invalidate_range_start+0xdd/0x190 [i915] > <4> [175.969049] __mmu_notifier_invalidate_range_start+0x148/0x250 > <4> [175.969052] unmap_vmas+0x13e/0x150 > <4> [175.969055] unmap_region+0xa3/0x100 > <4> [175.969057] __do_munmap+0x26d/0x490 > <4> [175.969060] __vm_munmap+0x66/0xc0 > <4> [175.969063] __x64_sys_munmap+0x12/0x20 > <4> [175.969066] do_syscall_64+0x4f/0x220 > <4> [175.969068] entry_SYSCALL_64_after_hwframe+0x49/0xbe > <4> [175.969071] > > cycle but with obj->mm.lock. > -Chris >
Hi Chris, Let me share some of my observations on the userptr(mmap_offset) case. On Wednesday, January 29, 2020 12:24:39 PM CET Chris Wilson wrote: > Quoting Patchwork (2020-01-23 20:22:25) > > == Series Details == > > > > Series: tests/gem_userptr_blits: Enhance invalid mapping exercise > > URL : https://patchwork.freedesktop.org/series/72411/ > > State : success > > > > == Summary == > > > > CI Bug Log - changes from CI_DRM_7794_full -> IGTPW_3969_full > > ==================================================== > > > > Summary > > ------- > > > > **SUCCESS** > > > > No regressions found. > > > > External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3969/index.html > > > > New tests > > --------- > > > > New tests have been introduced between CI_DRM_7794_full and IGTPW_3969_full: > > > > ### New IGT tests (3) ### > > > > * igt@gem_userptr_blits@invalid-uc-mapping: > > - Statuses : 7 pass(s) > > - Exec time: [0.01, 0.02] s > > > > * igt@gem_userptr_blits@invalid-wb-mapping: > > - Statuses : 7 pass(s) > > - Exec time: [0.01, 0.02] s > > > > * igt@gem_userptr_blits@invalid-wc-mapping: > > - Statuses : 7 pass(s) > > - Exec time: [0.01, 0.02] s > > Ok, we need a bit more work to trigger the lockdep loop. We need a > cancel_userptr of the same object that is faulted via mmap-offset. > > We're basically looking for the equivalent of this > > <4> [175.968441] ====================================================== > <4> [175.968444] WARNING: possible circular locking dependency detected > <4> [175.968447] 5.5.0-CI-CI_DRM_7828+ #1 Tainted: G U > <4> [175.968449] ------------------------------------------------------ > <4> [175.968452] gem_userptr_bli/1564 is trying to acquire lock: > <4> [175.968455] ffff888208f49580 (&vm->mutex){+.+.}, at: i915_vma_unbind+0xae/0x110 [i915] > <4> [175.968527] > but task is already holding lock: > <4> [175.968529] ffffffff82664d40 (mmu_notifier_invalidate_range_start){+.+.}, at: unmap_vmas+0x0/0x150 > <4> [175.968535] > which lock already depends on the new lock. > <4> [175.968538] > the existing dependency chain (in reverse order) is: > <4> [175.968541] > -> #2 (mmu_notifier_invalidate_range_start){+.+.}: > <4> [175.968546] page_mkclean_one+0xda/0x210 > <4> [175.968548] rmap_walk_file+0xff/0x260 > <4> [175.968551] page_mkclean+0x9f/0xb0 > <4> [175.968555] clear_page_dirty_for_io+0xa2/0x2f0 > <4> [175.968559] mpage_submit_page+0x1a/0x70 > <4> [175.968561] mpage_process_page_bufs+0xe7/0x110 > <4> [175.968564] mpage_prepare_extent_to_map+0x1d2/0x2b0 > <4> [175.968568] ext4_writepages+0x5ba/0x12b0 > <4> [175.968571] do_writepages+0x46/0xe0 > <4> [175.968573] __filemap_fdatawrite_range+0xc6/0x100 > <4> [175.968576] file_write_and_wait_range+0x3c/0x90 > <4> [175.968579] ext4_sync_file+0x1a4/0x540 > <4> [175.968582] do_fsync+0x33/0x60 > <4> [175.968584] __x64_sys_fsync+0xb/0x10 > <4> [175.968587] do_syscall_64+0x4f/0x220 > <4> [175.968591] entry_SYSCALL_64_after_hwframe+0x49/0xbe > <4> [175.968594] > -> #1 (&mapping->i_mmap_rwsem){++++}: > <4> [175.968599] down_write+0x33/0x70 > <4> [175.968601] unmap_mapping_pages+0x48/0x130 > <4> [175.968671] i915_vma_revoke_mmap.part.37+0x66/0x190 [i915] > <4> [175.968715] fence_update+0xfd/0x2d0 [i915] > <4> [175.968759] __i915_vma_unbind+0x1eb/0x530 [i915] > <4> [175.968803] i915_vma_release+0x101/0x220 [i915] > <4> [175.968843] __i915_gem_free_objects+0x113/0x530 [i915] > <4> [175.968886] i915_gem_create_ioctl+0x12/0x40 [i915] > <4> [175.968890] drm_ioctl_kernel+0xad/0xf0 > <4> [175.968893] drm_ioctl+0x2e1/0x390 > <4> [175.968896] do_vfs_ioctl+0x9c/0x730 > <4> [175.968899] ksys_ioctl+0x35/0x60 > <4> [175.968901] __x64_sys_ioctl+0x11/0x20 > <4> [175.968904] do_syscall_64+0x4f/0x220 > <4> [175.968906] entry_SYSCALL_64_after_hwframe+0x49/0xbe > <4> [175.968909] > -> #0 (&vm->mutex){+.+.}: > <4> [175.968914] __lock_acquire+0x1328/0x15d0 > <4> [175.968916] lock_acquire+0xa7/0x1c0 > <4> [175.968919] __mutex_lock+0x9a/0x9c0 > <4> [175.968962] i915_vma_unbind+0xae/0x110 [i915] > <4> [175.969004] i915_gem_object_unbind+0x1dc/0x400 [i915] > <4> [175.969045] userptr_mn_invalidate_range_start+0xdd/0x190 [i915] AFAICU, for that to happen, not only the userptr MMU notifier would have to be registered, but at least one userptr object would have to be added to the notifier's list of active objects. For a userptr object to be ever added to the notifier's list of active objects, it would have to be created without I915_USERPTR_UNSYNCHRONIZED flag set. As "invalid-*-mapping" (or dynamic "invalid-mmap-offset" since addressing your comments in v3 of my patch) subtests create userptr objects with that flag set, we need "-sync" flavors of those subtests in order to have a chance for the lockdep loop to be exercised. If that observation is not questionable to you, I'm going to address it in my next version of the patch. Now, the only way to activate the MMU notifier for a userptr object is when __i915_gem_userptr_set_active() is called from i915_gem_userptr_get_pages(). That can happen when either all required pages have already been pinned before and are returned by __get_user_pages_fast(), or, if some pages are not yet pinned, when __i915_gem_userptr_get_pages_schedule() likely succeeds (returns -EAGAIN). In the latter case, __i915_gem_userptr_get_pages_worker() work is scheduled. In case of a userptr object backed by our mmap-offset mapping, get_user_pages_remote() called from __i915_gem_userptr_get_pages_worker() fails immediately with -EFAULT on (vm_flags & (VM_IO | VM_PFNMAP)) in mm/gup.c:check_vma_flags(). As a result, the MMU notifier is immediately deactivated for the object. Then indeed, a time window with the MMU notifier being active for the object exists. However, I still can't find out how userspace could trigger the lockdep loop *within* that time window in a reproducible manner. Could you think of a way to do it? On the other hand, if we could postpone activation of the userptr MMU notifier for an object, or otherwise prevent the notifier from doing its job until at least one page is successfully acquired, then a userptr object backed with mmap-offset mapping would never be able to trigger that lockdep loop, I believe. If you find my conclusions not missing the point, I'm going to propose a patch (i915, not IGT). Thanks, Janusz > <4> [175.969049] __mmu_notifier_invalidate_range_start+0x148/0x250 > <4> [175.969052] unmap_vmas+0x13e/0x150 > <4> [175.969055] unmap_region+0xa3/0x100 > <4> [175.969057] __do_munmap+0x26d/0x490 > <4> [175.969060] __vm_munmap+0x66/0xc0 > <4> [175.969063] __x64_sys_munmap+0x12/0x20 > <4> [175.969066] do_syscall_64+0x4f/0x220 > <4> [175.969068] entry_SYSCALL_64_after_hwframe+0x49/0xbe > <4> [175.969071] > > cycle but with obj->mm.lock. > -Chris >
diff --git a/tests/i915/gem_userptr_blits.c b/tests/i915/gem_userptr_blits.c index a8d3783f..69e5bd1f 100644 --- a/tests/i915/gem_userptr_blits.c +++ b/tests/i915/gem_userptr_blits.c @@ -60,6 +60,7 @@ #include "drm.h" #include "i915_drm.h" +#include "i915/gem_mman.h" #include "intel_bufmgr.h" @@ -577,11 +578,11 @@ static int test_invalid_null_pointer(int fd) return 0; } -static int test_invalid_gtt_mapping(int fd) +static int test_invalid_mapping(int fd, uint64_t flags) { - struct drm_i915_gem_mmap_gtt arg; + struct drm_i915_gem_mmap_offset arg; uint32_t handle; - char *gtt, *map; + char *offset, *map; /* Anonymous mapping to find a hole */ map = mmap(NULL, sizeof(linear) + 2 * PAGE_SIZE, @@ -602,39 +603,46 @@ static int test_invalid_gtt_mapping(int fd) igt_assert_eq(copy(fd, handle, handle), 0); gem_close(fd, handle); - /* GTT mapping */ + /* object mapping */ memset(&arg, 0, sizeof(arg)); arg.handle = create_bo(fd, 0); - do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &arg); - gtt = mmap(map + PAGE_SIZE, sizeof(linear), - PROT_READ | PROT_WRITE, - MAP_SHARED | MAP_FIXED, - fd, arg.offset); - igt_assert(gtt == map + PAGE_SIZE); + arg.flags = flags; + do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_OFFSET, &arg); + offset = mmap(map + PAGE_SIZE, sizeof(linear), PROT_READ | PROT_WRITE, + MAP_SHARED | MAP_FIXED, fd, arg.offset); + igt_assert(offset == map + PAGE_SIZE); gem_close(fd, arg.handle); - igt_assert(((unsigned long)gtt & (PAGE_SIZE - 1)) == 0); + igt_assert(((unsigned long)offset & (PAGE_SIZE - 1)) == 0); igt_assert((sizeof(linear) & (PAGE_SIZE - 1)) == 0); - gem_userptr(fd, gtt, sizeof(linear), 0, userptr_flags, &handle); - igt_assert_eq(copy(fd, handle, handle), -EFAULT); - gem_close(fd, handle); + if (!__gem_userptr(fd, offset, sizeof(linear), 0, userptr_flags, + &handle)) { + igt_assert_eq(copy(fd, handle, handle), -EFAULT); + gem_close(fd, handle); + } - gem_userptr(fd, gtt, PAGE_SIZE, 0, userptr_flags, &handle); - igt_assert_eq(copy(fd, handle, handle), -EFAULT); - gem_close(fd, handle); + if (!__gem_userptr(fd, offset, PAGE_SIZE, 0, userptr_flags, &handle)) { + igt_assert_eq(copy(fd, handle, handle), -EFAULT); + gem_close(fd, handle); + } - gem_userptr(fd, gtt + sizeof(linear) - PAGE_SIZE, PAGE_SIZE, 0, userptr_flags, &handle); - igt_assert_eq(copy(fd, handle, handle), -EFAULT); - gem_close(fd, handle); + if (!__gem_userptr(fd, offset + sizeof(linear) - PAGE_SIZE, PAGE_SIZE, + 0, userptr_flags, &handle)) { + igt_assert_eq(copy(fd, handle, handle), -EFAULT); + gem_close(fd, handle); + } /* boundaries */ - gem_userptr(fd, map, 2*PAGE_SIZE, 0, userptr_flags, &handle); - igt_assert_eq(copy(fd, handle, handle), -EFAULT); - gem_close(fd, handle); + if (!__gem_userptr(fd, map, 2 * PAGE_SIZE, 0, userptr_flags, &handle)) { + igt_assert_eq(copy(fd, handle, handle), -EFAULT); + gem_close(fd, handle); + } - gem_userptr(fd, map + sizeof(linear), 2*PAGE_SIZE, 0, userptr_flags, &handle); - igt_assert_eq(copy(fd, handle, handle), -EFAULT); - gem_close(fd, handle); + if (!__gem_userptr(fd, map + sizeof(linear), 2 * PAGE_SIZE, 0, + userptr_flags, &handle)) { + igt_assert_eq(copy(fd, handle, handle), -EFAULT); + gem_close(fd, handle); + } munmap(map, sizeof(linear) + 2*PAGE_SIZE); @@ -2009,8 +2017,31 @@ igt_main_args("c:", NULL, help_str, opt_handler, NULL) igt_subtest("invalid-null-pointer") test_invalid_null_pointer(fd); - igt_subtest("invalid-gtt-mapping") - test_invalid_gtt_mapping(fd); + igt_describe("Verify userptr on top of GTT mapping to GEM object will fail"); + igt_subtest("invalid-gtt-mapping") { + gem_require_mappable_ggtt(fd); + test_invalid_mapping(fd, I915_MMAP_OFFSET_GTT); + } + igt_subtest_group { + igt_fixture + igt_require(gem_has_mmap_offset(fd)); + + igt_describe("Verify userptr on top of CPU mapping to GEM object will fail"); + igt_subtest("invalid-wb-mapping") + test_invalid_mapping(fd, I915_MMAP_OFFSET_WB); + + igt_subtest_group { + igt_fixture + igt_require(gem_mmap_offset__has_wc(fd)); + + igt_describe("Verify userptr on top of coherent mapping to GEM object will fail"); + igt_subtest("invalid-wc-mapping") + test_invalid_mapping(fd, I915_MMAP_OFFSET_WC); + igt_describe("Verify userptr on top of uncached mapping to GEM object will fail"); + igt_subtest("invalid-uc-mapping") + test_invalid_mapping(fd, I915_MMAP_OFFSET_UC); + } + } igt_subtest("forked-access") test_forked_access(fd);
Working with a userptr GEM object backed by any type of mapping to another GEM object, not only GTT mapping currently examined bu the test, may cause a currently unavoidable lockdep splat inside the i915 driver. Then, such operations are expected to fail in advance to prevent from that badness to happen. Extend the scope of the test by adding subtests which exercise other, non-GTT mapping types. Moreover, also succeed if a failure happens as soon as a userptr object is created on top of an invalid mapping. Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> --- tests/i915/gem_userptr_blits.c | 87 +++++++++++++++++++++++----------- 1 file changed, 59 insertions(+), 28 deletions(-)