On 9 May 2026, at 4:06, David Hildenbrand (Arm) wrote:
> On 4/29/26 17:35, Zi Yan wrote:
>> Change the requirement to a file system with large folio support and the
>> supported order needs to include PMD_ORDER.
>>
>> Also add tests of opening a file with read write permission and populating
>> folios with writes. Reuse the XFS image from split_huge_page_test.
>>
>> Signed-off-by: Zi Yan <[email protected]>
>> ---
>> tools/testing/selftests/mm/khugepaged.c | 131 +++++++++++++++-------
>> tools/testing/selftests/mm/run_vmtests.sh | 12 +-
>> 2 files changed, 102 insertions(+), 41 deletions(-)
>>
>> diff --git a/tools/testing/selftests/mm/khugepaged.c
>> b/tools/testing/selftests/mm/khugepaged.c
>> index a6bb9d50363d2..80b913185c643 100644
>> --- a/tools/testing/selftests/mm/khugepaged.c
>> +++ b/tools/testing/selftests/mm/khugepaged.c
>> @@ -49,7 +49,8 @@ struct mem_ops {
>> const char *name;
>> };
>>
>> -static struct mem_ops *file_ops;
>> +static struct mem_ops *read_only_file_ops;
>> +static struct mem_ops *read_write_file_ops;
>> static struct mem_ops *anon_ops;
>> static struct mem_ops *shmem_ops;
>>
>> @@ -112,7 +113,8 @@ static void restore_settings(int sig)
>> static void save_settings(void)
>> {
>> printf("Save THP and khugepaged settings...");
>> - if (file_ops && finfo.type == VMA_FILE)
>> + if ((read_only_file_ops || read_write_file_ops) &&
>> + finfo.type == VMA_FILE)
>> thp_set_read_ahead_path(finfo.dev_queue_read_ahead_path);
>> thp_save_settings();
>>
>> @@ -364,11 +366,14 @@ static bool anon_check_huge(void *addr, int nr_hpages)
>> return check_huge_anon(addr, nr_hpages, hpage_pmd_size);
>> }
>>
>> -static void *file_setup_area(int nr_hpages)
>> +static void *file_setup_area_common(int nr_hpages, bool read_only)
>> {
>> int fd;
>> void *p;
>> unsigned long size;
>> + int open_opt = read_only ? O_RDONLY : O_RDWR;
>> + int mmap_prot = read_only ? PROT_READ : (PROT_READ | PROT_WRITE);
>> + int mmap_opt = read_only ? MAP_PRIVATE : MAP_SHARED;
>
> Can all be const and read better at the very top of the list.
>
> Why do we go from private->shared depending on "read_only" parameter? I'd
> assume
> we'd want to test MAP_SHARED for both (read-only + read-write)?
The original code used MAP_PRIVATE, but I agree that MAP_SHARED works.
Will change it.
>
>
>>
>> unlink(finfo.path); /* Cleanup from previous failed tests */
>> printf("Creating %s for collapse%s...", finfo.path,
>> @@ -399,14 +404,15 @@ static void *file_setup_area(int nr_hpages)
>> munmap(p, size);
>> success("OK");
>>
>> - printf("Opening %s read only for collapse...", finfo.path);
>> - finfo.fd = open(finfo.path, O_RDONLY, 777);
>> + printf("Opening %s %s for collapse...", finfo.path,
>> + read_only ? "read only" : "read-write");
>
> "read-only" ?
>
>> + finfo.fd = open(finfo.path, open_opt, 777);
>> if (finfo.fd < 0) {
>> perror("open()");
>> exit(EXIT_FAILURE);
>> }
>> - p = mmap(BASE_ADDR, size, PROT_READ,
>> - MAP_PRIVATE, finfo.fd, 0);
>> + p = mmap(BASE_ADDR, size, mmap_prot,
>> + mmap_opt, finfo.fd, 0);
>
> While at it, can fit that into a single line.
>
>> if (p == MAP_FAILED || p != BASE_ADDR) {
>> perror("mmap()");
>> exit(EXIT_FAILURE);
>> @@ -418,6 +424,16 @@ static void *file_setup_area(int nr_hpages)
>> return p;
>> }
>>
>> +static void *file_setup_read_only_area(int nr_hpages)
>> +{
>> + return file_setup_area_common(nr_hpages, /* read_only= */ true);
>> +}
>> +
>> +static void *file_setup_read_write_area(int nr_hpages)
>> +{
>> + return file_setup_area_common(nr_hpages, /* read_only= */ false);
>> +}
>> +
>> static void file_cleanup_area(void *p, unsigned long size)
>> {
>> munmap(p, size);
>> @@ -425,14 +441,25 @@ static void file_cleanup_area(void *p, unsigned long
>> size)
>> unlink(finfo.path);
>> }
>>
>> -static void file_fault(void *p, unsigned long start, unsigned long end)
>> +static void file_fault_common(void *p, unsigned long start, unsigned long
>> end,
>> + int madv_ops)
>> {
>> - if (madvise(((char *)p) + start, end - start, MADV_POPULATE_READ)) {
>> + if (madvise(((char *)p) + start, end - start, madv_ops)) {
>> perror("madvise(MADV_POPULATE_READ");
>> exit(EXIT_FAILURE);
>> }
>> }
>>
>> +static void file_fault_read(void *p, unsigned long start, unsigned long end)
>> +{
>> + file_fault_common(p, start, end, MADV_POPULATE_READ);
>
> Do we really want file_fault_common()? I'd say, just inline it. In particular
> avoids checking the madv_ops to figure out the error message ...
>
>> +}
>> +
>> +static void file_fault_write(void *p, unsigned long start, unsigned long
>> end)
>> +{
>> + file_fault_common(p, start, end, MADV_POPULATE_WRITE);
>> +}
>> +
>> static bool file_check_huge(void *addr, int nr_hpages)
>> {
>> switch (finfo.type) {
>> @@ -488,10 +515,18 @@ static struct mem_ops __anon_ops = {
>> .name = "anon",
>> };
>
> [...]
>
>> };
>>
>> +static bool is_tmpfs(struct mem_ops *ops)
>> +{
>> + return (ops == &__read_only_file_ops ||
>> + ops == &__read_write_file_ops) &&
>
> Can't this fit into two lines?
>
>> + finfo.type == VMA_SHMEM;
>> +}
>> +
>> +static bool is_anon(struct mem_ops *ops)
>> +{
>> + return ops == &__anon_ops;
>> +}
>> +
>> static void __madvise_collapse(const char *msg, char *p, int nr_hpages,
>> struct mem_ops *ops, bool expect)
>> {
>
>> -
>> static void alloc_at_fault(void)
>> {
>> struct thp_settings settings = *thp_current_settings();
>> @@ -1097,8 +1142,8 @@ static void usage(void)
>> fprintf(stderr, "\t<context>\t: [all|khugepaged|madvise]\n");
>> fprintf(stderr, "\t<mem_type>\t: [all|anon|file|shmem]\n");
>> fprintf(stderr, "\n\t\"file,all\" mem_type requires [dir] argument\n");
>> - fprintf(stderr, "\n\t\"file,all\" mem_type requires kernel built
>> with\n");
>> - fprintf(stderr, "\tCONFIG_READ_ONLY_THP_FOR_FS=y\n");
>> + fprintf(stderr, "\n\t\"file,all\" mem_type requires a file system\n");
>> + fprintf(stderr, "\twith large folio support (order >= PMD order)\n");
>
> "with PMD-sized large folio support"?
>
>> fprintf(stderr, "\n\tif [dir] is a (sub)directory of a tmpfs mount,
>> tmpfs must be\n");
>> fprintf(stderr, "\tmounted with huge=advise option for khugepaged tests
>> to work\n");
>> fprintf(stderr, "\n\tSupported Options:\n");
>> @@ -1154,20 +1199,22 @@ static void parse_test_type(int argc, char **argv)
>> usage();
Ack on all above.
Best Regards,
Yan, Zi
