Re: [RESEND PATCH v7 09/10] selftests/mm/cow: Generalize do_run_with_thp() helper

[Ryan Roberts]

On 24/11/2023 17:48, David Hildenbrand wrote:
> On 22.11.23 17:29, Ryan Roberts wrote:
>> do_run_with_thp() prepares (PMD-sized) THP memory into different states
>> before running tests. With the introduction of small-sized THP, we would
>> like to reuse this logic to also test those smaller THP sizes. So let's
>> add a size parameter which tells the function what size THP it should
>> operate on.
>>
>> A separate commit will utilize this change to add new tests for
>> small-sized THP, where available.
>>
>> Signed-off-by: Ryan Roberts <ryan.roberts@xxxxxxx>
>> ---
>>   tools/testing/selftests/mm/cow.c | 146 +++++++++++++++++--------------
>>   1 file changed, 79 insertions(+), 67 deletions(-)
>>
>> diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c
>> index 7324ce5363c0..d03c453cfd5c 100644
>> --- a/tools/testing/selftests/mm/cow.c
>> +++ b/tools/testing/selftests/mm/cow.c
>> @@ -32,7 +32,7 @@
>>
>>   static size_t pagesize;
>>   static int pagemap_fd;
>> -static size_t thpsize;
>> +static size_t pmdsize;
>>   static int nr_hugetlbsizes;
>>   static size_t hugetlbsizes[10];
>>   static int gup_fd;
>> @@ -734,14 +734,14 @@ enum thp_run {
>>       THP_RUN_PARTIAL_SHARED,
>>   };
>>
>> -static void do_run_with_thp(test_fn fn, enum thp_run thp_run)
>> +static void do_run_with_thp(test_fn fn, enum thp_run thp_run, size_t size)
> 
> Nit: can we still call it "thpsize" in this function? That makes it clearer IMHO
> and avoids most renaming.

Yep no problem. Will fix in next version.

> 
>>   {
>>       char *mem, *mmap_mem, *tmp, *mremap_mem = MAP_FAILED;
>> -    size_t size, mmap_size, mremap_size;
>> +    size_t mmap_size, mremap_size;
>>       int ret;
>>
>> -    /* For alignment purposes, we need twice the thp size. */
>> -    mmap_size = 2 * thpsize;
>> +    /* For alignment purposes, we need twice the requested size. */
>> +    mmap_size = 2 * size;
>>       mmap_mem = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE,
>>               MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>>       if (mmap_mem == MAP_FAILED) {
>> @@ -749,36 +749,40 @@ static void do_run_with_thp(test_fn fn, enum thp_run
>> thp_run)
>>           return;
>>       }
>>
>> -    /* We need a THP-aligned memory area. */
>> -    mem = (char *)(((uintptr_t)mmap_mem + thpsize) & ~(thpsize - 1));
>> +    /* We need to naturally align the memory area. */
>> +    mem = (char *)(((uintptr_t)mmap_mem + size) & ~(size - 1));
>>
>> -    ret = madvise(mem, thpsize, MADV_HUGEPAGE);
>> +    ret = madvise(mem, size, MADV_HUGEPAGE);
>>       if (ret) {
>>           ksft_test_result_fail("MADV_HUGEPAGE failed\n");
>>           goto munmap;
>>       }
>>
>>       /*
>> -     * Try to populate a THP. Touch the first sub-page and test if we get
>> -     * another sub-page populated automatically.
>> +     * Try to populate a THP. Touch the first sub-page and test if
>> +     * we get the last sub-page populated automatically.
>>        */
>>       mem[0] = 0;
>> -    if (!pagemap_is_populated(pagemap_fd, mem + pagesize)) {
>> +    if (!pagemap_is_populated(pagemap_fd, mem + size - pagesize)) {
>>           ksft_test_result_skip("Did not get a THP populated\n");
>>           goto munmap;
>>       }
> 
> Yes! I have a patch lying around here that does that same. :)
> 
> I guess there is no need to set MADV_NOHUGEPAGE on the remainder of the mmap'ed
> are:
> 
> Assume we want a 64KiB thp. We mmap'ed 128KiB. If we get a reasonably aligned
> area, we might populate a 128KiB THP.
> 
> But I assume the MADV_HUGEPAGE will in all configurations properly create a
> separate 64KiB VMA and we'll never get 128 KiB populated. So this should work
> reliably.

Yes agreed. And also, we explicitly only enable a single THP size at a time so
should only allocate a THP of the expected size. Perhaps we should mark the
whole mmap area with MADV_HUGEPAGE since that will serve as a test that we only
get the smaller size we configured?

> 
>> -    memset(mem, 0, thpsize);
>> +    memset(mem, 0, size);
>>
>> -    size = thpsize;
>>       switch (thp_run) {
>>       case THP_RUN_PMD:
>>       case THP_RUN_PMD_SWAPOUT:
>> +        if (size != pmdsize) {
>> +            ksft_test_result_fail("test bug: can't PMD-map size\n");
>> +            goto munmap;
>> +        }
> 
> Maybe rather "assert()" because that's a real BUG in the test?

Yep will do.

> 
> [...]
> 
>> +    pmdsize = read_pmd_pagesize();
>> +    if (pmdsize)
>> +        ksft_print_msg("[INFO] detected PMD-mapped THP size: %zu KiB\n",
> 
> Maybe simply: "detected PMD size". Zes, we read it via the THP interface, but
> that shouldn't matter much.

Err, just want to clarify what you are suggesting. With the current patch you
will see something like:

[INFO] detected PMD-mapped THP size: 2048 KiB
[INFO] detected small-sized THP size: 64 KiB
[INFO] detected small-sized THP size: 128 KiB
...
[INFO] detected small-sized THP size: 1024 KiB


Are you suggesting something like this:

[INFO] detected PMD size: 2048 KiB
[INFO] detected THP size: 64 KiB
[INFO] detected THP size: 128 KiB
...
[INFO] detected THP size: 2048 KiB


>