Hi David!
On 8/25/26 2:56 PM, David Hildenbrand (Arm) wrote:
>
>> 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.
Makes sense, I can remove this section in the 4th patch (tools/mm: move
gup_test from selftests/mm to tools/mm), since we're moving and renaming
gup_test.c in the same patch.
>
>> 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 <[email protected]>
> 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.
Agreed, I'll remove the DUMP_USER_PAGES_TEST cases from gup.c.
>
> 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).
I think we can take this up separate from this series since it would
extend the scope of the series significantly. Does it work or do you
want me to incorporate it in the same series?
