Re: [PATCH v4] selftests/mm: add folio_split() and filemap_get_entry() race test

[Zi Yan]

On 23 Mar 2026, at 8:48, David Hildenbrand (Arm) wrote:

> On 3/20/26 15:22, Zi Yan wrote:
>> The added folio_split_race_test is a modified C port of the race condition
>> test from [1]. The test creates shmem huge pages, where the main thread
>> punches holes in the shmem to cause folio_split() in the kernel and
>> a set of 16 threads reads the shmem to cause filemap_get_entry() in the
>> kernel. filemap_get_entry() reads the folio and xarray split by
>> folio_split() locklessly. The original test[2] is written in rust and uses
>> memfd (shmem backed). This C port uses shmem directly and use a single
>> process.
>>
>> Note: the initial rust to C conversion is done by Cursor.
>>
>> Link: https://lore.kernel.org/all/CAKNNEtw5_kZomhkugedKMPOG-sxs5Q5OLumWJdiWXv+C9Yct0w@xxxxxxxxxxxxxx/ [1]
>> Link: https://github.com/dfinity/thp-madv-remove-test [2]
>> Signed-off-by: Bas van Dijk <bas@xxxxxxxxxxx>
>> Signed-off-by: Adam Bratschi-Kaye <adam.bratschikaye@xxxxxxxxxxx>
>
> You are likely missing two Co-developed-by.
>
> See Documentation/process/submitting-patches.rst on how to handle such
> SOBs.

Will add them.

>
>> Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>
>> ---
>> From V3:
>> 1. fixed for loop stepping issue
>> 2. used PRIu64 instead of %zu for uint64_t.
>>
>> From V2:
>> 1. simplied the program by removing fork.
>>
>> From V1:
>> 1. added prctl(PR_SET_PDEATHSIG, SIGTERM) to avoid child looping
>>    forever.
>> 2. removed page_idx % PUNCH_INTERVAL >= 0, since it is a nop. Added a
>>    comment.
>> 3. added a child process status check to prevent parent looping forever
>>    and record that as a failure.
>> 4. used ksft_exit_skip() instead of ksft_finished() when the program is
>>    not running as root.
>> 5. restored THP settings properly when the program exits abnormally.
>>  tools/testing/selftests/mm/Makefile           |   1 +
>>  .../selftests/mm/folio_split_race_test.c      | 293 ++++++++++++++++++
>>  tools/testing/selftests/mm/run_vmtests.sh     |   2 +
>>  3 files changed, 296 insertions(+)
>>  create mode 100644 tools/testing/selftests/mm/folio_split_race_test.c
>>
>> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
>> index 7a5de4e9bf520..cd24596cdd27e 100644
>> --- a/tools/testing/selftests/mm/Makefile
>> +++ b/tools/testing/selftests/mm/Makefile
>> @@ -105,6 +105,7 @@ TEST_GEN_FILES += droppable
>>  TEST_GEN_FILES += guard-regions
>>  TEST_GEN_FILES += merge
>>  TEST_GEN_FILES += rmap
>> +TEST_GEN_FILES += folio_split_race_test
>>
>>  ifneq ($(ARCH),arm64)
>>  TEST_GEN_FILES += soft-dirty
>> diff --git a/tools/testing/selftests/mm/folio_split_race_test.c b/tools/testing/selftests/mm/folio_split_race_test.c
>> new file mode 100644
>> index 0000000000000..c264cc625a7cb
>> --- /dev/null
>> +++ b/tools/testing/selftests/mm/folio_split_race_test.c
>> @@ -0,0 +1,293 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * The test creates shmem PMD huge pages, fills all pages with known patterns,
>> + * then continuously verifies non-punched pages with 16 threads. Meanwhile, the
>> + * main thread punches holes via MADV_REMOVE on the shmem.
>> + *
>> + * It tests the race condition between folio_split() and filemap_get_entry(),
>> + * where the hole punches on shmem lead to folio_split() and reading the shmem
>> + * lead to filemap_get_entry().
>> + */
>> +
>> +#define _GNU_SOURCE
>> +#include <errno.h>
>> +#include <inttypes.h>
>> +#include <linux/mman.h>
>> +#include <pthread.h>
>> +#include <stdatomic.h>
>> +#include <stdbool.h>
>> +#include <stdint.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <sys/mman.h>
>> +#include <signal.h>
>> +#include <unistd.h>
>> +#include "vm_util.h"
>> +#include "kselftest.h"
>> +#include "thp_settings.h"
>
>
> [...] some comment son the main() part :)
>
>> +int main(void)
>> +{
>> +	struct thp_settings current_settings;
>> +	bool failed = false;
>> +	size_t failures;
>> +	size_t iter;
>
> Why are iterations a "size_t" ? Similarly for "failures". Just use int / unsigned long?

Will change them.

>
>> +
>> +	ksft_print_header();
>> +
>> +	if (!thp_is_enabled())
>> +		ksft_exit_skip("Transparent Hugepages not available\n");
>
> Is checking thp_is_enabled() the right thing to do when you perform your own setup below either way?
>
> I think you should just use thp_available(). Then, configure THP accordingly below?

OK, will use thp_available().

>
>> +
>> +	if (geteuid() != 0)
>> +		ksft_exit_skip("Please run the test as root\n");
>> +
>> +	thp_save_settings();
>> +	/* make sure thp settings are restored */
>> +	if (atexit(thp_settings_cleanup) != 0)
>> +		ksft_exit_fail_msg("atexit failed\n");
>> +
>> +	signal(SIGINT, thp_cleanup_handler);
>> +	signal(SIGTERM, thp_cleanup_handler);
>> +
>> +	thp_read_settings(&current_settings);
>> +	current_settings.shmem_enabled = SHMEM_ADVISE;
>> +	thp_write_settings(&current_settings);
>> +
>> +	ksft_set_plan(1);
>> +
>> +	page_size = getpagesize();
>> +	pmd_pagesize = read_pmd_pagesize();
>
> I wonder whether we should check for 0 here and skip the test (older kernels?).

OK, will take care of that.

>
>> +
>> +	ksft_print_msg("folio split race test\n");
>> +	ksft_print_msg("===================================================\n");
>> +	ksft_print_msg("Shmem size:       %" PRIu64 " MiB\n", FILE_SIZE / 1024 / 1024);
>> +	ksft_print_msg("Total pages:     %" PRIu64 "\n", TOTAL_PAGES);
>> +	ksft_print_msg("Child readers:   %d\n", NUM_READER_THREADS);
>> +	ksft_print_msg("Punching every %dth to %dth page\n", PUNCH_INTERVAL,
>> +		       PUNCH_INTERVAL + PUNCH_SIZE_FACTOR);
>> +	ksft_print_msg("Iterations:      %d\n", NUM_ITERATIONS);
>
>
> I don't think printing static test information is that helpful.
> Do we need all that at all?

To provide some information on what this test is doing? I am OK with
removing them, but one will need to check the source code to get an idea.

>
>
>> +
>> +	for (iter = 1; iter <= NUM_ITERATIONS; iter++) {
>
> Why not start at 0? You know, to confuse less people :)
>
> 	for (iter = 0; iter < NUM_ITERATIONS; iter++) {

Will change it.

>
>> +		failures = run_iteration();
>
> "corrupted_pages" ?
>
>> +		if (failures > 0) {
>> +			failed = true;
>
> Do you really need that variable?
>
>> +			ksft_print_msg(
>> +				"FAILED on iteration %zu: %zu pages corrupted by MADV_REMOVE!\n",
>> +				iter, failures);
>
> Can that simply be printed below?
>
> Like
>
> if (iter < NUM_ITERATIONS) {
> 	ksft_test_result_fail("Test failed on iterations %zu: %zu pages ...\n",
> 			      iter + 1, corrupted_pages);
> } else {
> 	ksft_test_result_pass ...
> }
>

Sure, will simplify it. Thanks for the feedback.

>> +			break;
>> +		}
>> +	}
>> +
>> +	if (failed) {
>> +		ksft_test_result_fail("Test failed\n");
>> +		ksft_exit_fail();
>> +	} else {
>> +		ksft_test_result_pass("All %d iterations passed\n",
>> +				      NUM_ITERATIONS);
>> +		ksft_exit_pass();
>> +	}
>> +
>> +	return 0;
> -- 
> Cheers,
>
> David


Best Regards,
Yan, Zi