Re: [PATCH v7 6/6] selftests/mm: add a GUP selftest

["David Hildenbrand (Arm)" <david@kernel.org>]

> diff --git a/Documentation/core-api/pin_user_pages.rst b/Documentation/core-api/pin_user_pages.rst
> index c16ca163b55e..1564b16994ad 100644
> --- a/Documentation/core-api/pin_user_pages.rst
> +++ b/Documentation/core-api/pin_user_pages.rst
> @@ -228,12 +228,18 @@ Unit testing
>  ============
>  This file::
>  
> - tools/testing/selftests/mm/gup_test.c
> + tools/testing/selftests/mm/gup.c
>  
> -has the following new calls to exercise the new pin*() wrapper functions:
> +contains the following test cases to exercise pin_user_pages*():
>  
> -* PIN_FAST_BENCHMARK (./gup_test -a)
> -* PIN_BASIC_TEST (./gup_test -b)
> +* pin_user_pages via PIN_BASIC_TEST
> +* pin_user_pages_fast via PIN_FAST_BENCHMARK
> +* pin_user_pages_longterm via PIN_LONGTERM_BENCHMARK
> +
> +Run with::
> +
> +  make -C tools/testing/selftests/mm
> +  ./tools/testing/selftests/mm/gup
>  

Can we just remove that testing part in that doc completely in an earlier patch?
I don't really see the reason for documenting selftests that way.

In particular, now that it's a proper standalone selftest.

>  You can monitor how many total dma-pinned pages have been acquired and released
>  since the system was booted, via two new /proc/vmstat entries: ::
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ed9a8549ae31..861504fa2e31 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17032,6 +17032,7 @@ F:	mm/gup.c
>  F:	mm/gup_test.c
>  F:	mm/gup_test.h
>  F:	tools/mm/gup_bench.c
> +F:	tools/testing/selftests/mm/gup.c
>  F:	tools/testing/selftests/mm/gup_longterm.c

[...]

> +TEST_F(gup_test, dump_user_pages_with_get)
> +{
> +	run_gup_cmd(_metadata, self, variant, DUMP_USER_PAGES_TEST, 0, 1);
> +}
> +
> +TEST_F(gup_test, dump_user_pages_with_pin)
> +{
> +	run_gup_cmd(_metadata, self, variant, DUMP_USER_PAGES_TEST,
> +		    GUP_TEST_FLAG_DUMP_PAGES_USE_PIN, 1);
> +}

I really don't like DUMP_USER_PAGES_TEST: running the selftests just fills the
kernel log with useless information. And it's not like there is real value to it
beyond what the other tests are already testing.

... or that we would verify automatically what is being dumped makes any sense.

I'd vote to not add selftests that use it.

It was added in

commit f4f9bda418ab8b4dbc5372e9e2a28162f7777154
Author: John Hubbard <jhubbard@nvidia.com>
Date:   Mon Dec 14 19:05:21 2020 -0800

    selftests/vm: gup_test: introduce the dump_pages() sub-test

    For quite a while, I was doing a quick hack to gup_test.c (previously,
    gup_benchmark.c) whenever I wanted to try out my changes to dump_page().
    This makes that hack unnecessary, and instead allows anyone to easily get
    the same coverage from a user space program.  That saves a lot of time
    because you don't have to change the kernel, in order to test different
    pages and options.

    The new sub-test takes advantage of the existing gup_test infrastructure,
    which already provides a simple user space program, some allocated user
    space pages, an ioctl call, pinning of those pages (via either
    get_user_pages or pin_user_pages) and a corresponding kernel-side test
    invocation.  There's not much more required, mainly just a couple of
    inputs from the user.

    In fact, the new test re-uses the existing command line options in order
    to get various helpful combinations (THP or normal, _fast or slow gup, gup
    vs.  pup, and more).


I can see why we might want a dedicated dump_page/dump_folio test that either

(1) Is ran automatically and actually verifies (somehow automatically) what is
being dumped checks out. More tricky.

(2) Is ran manually by a suer that verifies whether what is being dumped checked
out. More feasible.

But as is, for an automated test this doesn't make sense.

Maybe we'd want a dedicated dump_page test tool in tools/mm that would make use
of DUMP_USER_PAGES_TEST. But maybe we also want to remove DUMP_USER_PAGES_TEST
entirely and have a different way to trigger+test this.

Ideally we'd have a proper automated dump_page test that actually checks
expected output (somehow).

-- 
Cheers,

David