[PATCH] i915/selftest/igt_mmap: let mmap tests run in kthread

[Andi Shyti]

Hi Mikolaj,

On Tue, Feb 25, 2025 at 11:41:56AM +0100, Mikolaj Wasiak wrote:
> When driver is loaded on system with numa nodes it might be run in kthread.

nit: please, don't forget articles:

*the* driver
*the* system

:-)

> This makes it impossible to use current->mm in selftests which currently
> creates null pointer exception.
> This patch allows selftest to use current->mm by using active_mm.

What is the failure you are getting? I'm not sure this might be
the right solution, but it might make sense, though... if there
is a valid reason.

It looks a bit drastic to me that we want to force current->mm to
point to the active_mm without a real reason, just to avoid a
NULL pointer dereference.

I would first ask myself why are we getting a NULL pointer
dereference? Why do we need to use current->mm? Which memory's
should current->mm point to? Is it a kernel thread or a user
thread? Why are we peaking inside current->mm?

Remember that for kernel threads is normally fine to have
current->mm equal to NULL.

> Signed-off-by: Mikolaj Wasiak <mikolaj.wasiak at intel.com>
> ---
>
> This patch is mostly damage control. By using active_mm we expose our
> test to foreign memory mapping, which sometimes makes the test fail.
> That is still better than just having null pointer exception in driver
> code.

If we are hiding a bug with another bug then this patch is not
OK, even if the second bug is less serious. So, please, don't do
it.

>  .../drm/i915/gem/selftests/i915_gem_mman.c    | 61 ++++++++++++++-----
>  drivers/gpu/drm/i915/selftests/igt_mmap.c     | 19 ++++++
>  drivers/gpu/drm/i915/selftests/igt_mmap.h     |  3 +
>  3 files changed, 67 insertions(+), 16 deletions(-)

...

>  int i915_gem_mman_live_selftests(struct drm_i915_private *i915)
> @@ -1847,6 +1877,5 @@ int i915_gem_mman_live_selftests(struct drm_i915_private *i915)
>               SUBTEST(igt_mmap_revoke),
>               SUBTEST(igt_mmap_gpu),
>       };
> -

This change is out of context.

>       return i915_live_subtests(tests, i915);
>  }
> diff --git a/drivers/gpu/drm/i915/selftests/igt_mmap.c b/drivers/gpu/drm/i915/selftests/igt_mmap.c
> index e920a461bd36..5c63622879a2 100644
> --- a/drivers/gpu/drm/i915/selftests/igt_mmap.c
> +++ b/drivers/gpu/drm/i915/selftests/igt_mmap.c
> @@ -50,3 +50,22 @@ unsigned long igt_mmap_offset(struct drm_i915_private *i915,
>       fput(file);
>       return addr;
>  }
> +
> +

you have two blank lines here.

> +int igt_mmap_enable_current(void)
> +{
> +     if (current->flags & PF_KTHREAD) {
> +             if (!current->active_mm) {
> +                     pr_info("Couldn't get userspace mm in kthread\n");
> +                     return -ENODATA;

if we get here, then something really bad has happened: we have a
task without memory. Most probably, if this was true, we wouldn't
even reach this point. I might have used a GEM_BUG_ON() here.

> +             }
> +             kthread_use_mm(current->active_mm);
> +     }
> +     return 0;
> +}
> +
> +void igt_mmap_disable_current(void)
> +{
> +     if (current->flags & PF_KTHREAD)
> +             kthread_unuse_mm(current->active_mm);
> +}
> diff --git a/drivers/gpu/drm/i915/selftests/igt_mmap.h b/drivers/gpu/drm/i915/selftests/igt_mmap.h
> index acbe34d81a6d..58582396bdd7 100644
> --- a/drivers/gpu/drm/i915/selftests/igt_mmap.h
> +++ b/drivers/gpu/drm/i915/selftests/igt_mmap.h
> @@ -18,4 +18,7 @@ unsigned long igt_mmap_offset(struct drm_i915_private *i915,
>                             unsigned long prot,
>                             unsigned long flags);
>
> +int igt_mmap_enable_current(void);
> +void igt_mmap_disable_current(void);

Please, don't make it a library call, it's just used by a few
functions in a file. No need to expose them.

Thanks,
Andi