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@xxxxxxxxx>
> ---
>
> 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
