Hi Krzysztof,
...
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> index 9d454d0b46f2..ccb00cd5e750 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> @@ -38,6 +38,8 @@ struct tile {
> unsigned int swizzle;
> };
>
> +bool skip_vma = true;
> +
I don't see any need for a global variable here, please refactor
to avoid it.
> static u64 swizzle_bit(unsigned int bit, u64 offset)
> {
> return (offset & BIT_ULL(bit)) >> (bit - 6);
...
> int i915_gem_mman_live_selftests(struct drm_i915_private *i915)
> {
> int ret;
> - bool unuse_mm = false;
> static const struct i915_subtest tests[] = {
> SUBTEST(igt_partial_tiling),
> SUBTEST(igt_smoke_tiling),
> @@ -1860,14 +1867,41 @@ int i915_gem_mman_live_selftests(struct
> drm_i915_private *i915)
> };
>
> if (!current->mm) {
so that if current->mm is true we skip valid tests?
> - kthread_use_mm(current->active_mm);
> - unuse_mm = true;
> + int timeout = 10;
10?
> + /**
> + * We want to use current->active_mm, which corresponds to the
> + * address space of a userspace process that was last handled by
> + * scheduler. It is possible that this memory is in the middle
> + * of cleanup indicated by mm_users == 0, in which case kernel
> + * waits until the process is finished to release its mm_struct.
> + * Borrowing that memory at that point is unsafe, because it may
> + * be freed at any point and taking additional references to it
> + * will not change the cleanup behavior.
> + * Wait for a bit in hopes that another process is taken by
> + * scheduler and has reliable memory for us to map into.
> + */
> + while (timeout--) {
> + if (mmget_not_zero(current->active_mm)) {
> + kthread_use_mm(current->active_mm);
> + skip_vma = false;
> + break;
> + }
> +
> + msleep(1000);
1000?
> + }
> }
>
> ret = i915_live_subtests(tests, i915);
>
> - if (unuse_mm)
> - kthread_unuse_mm(current->active_mm);
> + if (!skip_vma) {
> + /**
> + * The scheduler was working while the test executed,
> + * so current->active_mm might have changed. Use the old
> + * reference to the address space stored in current->mm.
> + */
> + mmput_async(current->mm);
> + kthread_unuse_mm(current->mm);
> + }
>
> return ret;
Overall I preferred the previous version. What's wrong to just
politically get the mm and move forward with the test?
Andi
