Re: [PATCH v3 3/7] selftest/mm: Fix ksm_funtional_test failures

[Donet Tom <donettom@linux.ibm.com>]

On 8/4/25 2:41 PM, Wei Yang wrote:
> On Tue, Jul 29, 2025 at 11:03:59AM +0530, Aboorva Devarajan wrote:
>> From: Donet Tom <donettom@linux.ibm.com>
>>
>> This patch fixed 2 issues.
>>
>> 1) After fork() in test_prctl_fork, the child process uses the file
>> descriptors from the parent process to read ksm_stat and
>> ksm_merging_pages. This results in incorrect values being read (parent
>> process ksm_stat and ksm_merging_pages will be read in child), causing
>> the test to fail.
>>
>> This patch calls init_global_file_handles() in the child process to
>> ensure that the current process's file descriptors are used to read
>> ksm_stat and ksm_merging_pages.
>>
>> 2) All tests currently call ksm_merge to trigger page merging.
>> To ensure the system remains in a consistent state for subsequent
>> tests, it is better to call ksm_unmerge during the test cleanup phase.
>>
>> In the test_prctl_fork test, after a fork(), reading ksm_merging_pages
>> in the child process returns a non-zero value because a previous test
>> performed a merge, and the child's memory state is inherited from the
>> parent.
>>
>> Although the child process calls ksm_unmerge, the ksm_merging_pages
>> counter in the parent is reset to zero, while the child's counter
>> remains unchanged. This discrepancy causes the test to fail.
>>
>> To avoid this issue, each test should call ksm_unmerge during cleanup
>> to ensure the counter is reset and the system is in a clean state for
>> subsequent tests.
>>
>> execv argument is an array of pointers to null-terminated strings.
>> In this patch we also added NULL in the execv argument.
>>
>> Fixes: 6c47de3be3a0 ("selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec")
>> Co-developed-by: Aboorva Devarajan <aboorvad@linux.ibm.com>
>> Signed-off-by: Aboorva Devarajan <aboorvad@linux.ibm.com>
>> Signed-off-by: Donet Tom <donettom@linux.ibm.com>
>> ---
>> tools/testing/selftests/mm/ksm_functional_tests.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
>> index d8bd1911dfc0..996dc6645570 100644
>> --- a/tools/testing/selftests/mm/ksm_functional_tests.c
>> +++ b/tools/testing/selftests/mm/ksm_functional_tests.c
>> @@ -46,6 +46,8 @@ static int ksm_use_zero_pages_fd;
>> static int pagemap_fd;
>> static size_t pagesize;
>>
>> +static void init_global_file_handles(void);
>> +
>> static bool range_maps_duplicates(char *addr, unsigned long size)
>> {
>> 	unsigned long offs_a, offs_b, pfn_a, pfn_b;
>> @@ -274,6 +276,7 @@ static void test_unmerge(void)
>> 	ksft_test_result(!range_maps_duplicates(map, size),
>> 			 "Pages were unmerged\n");
>> unmap:
>> +	ksm_unmerge();
> In __mmap_and_merge_range(), we call ksm_unmerge(). Why this one not help?
>
> Not very familiar with ksm stuff. Would you mind giving more on how this fix
> the failure you see?


The issue I was facing here was test_prctl_fork was failing.

# [RUN] test_prctl_fork
# Still pages merged
#

This issue occurred because the previous test performed a merge, causing
the value of /proc/self/ksm_merging_pages to reflect the number of
deduplicated pages. After that, a fork() was called. Post-fork, the 
child process
inherited the parent's ksm_merging_pages value.

Then, the child process invoked __mmap_and_merge_range(), which resulted
in unmerging the pages and resetting the value. However, since the 
parent process
had performed the merge, its ksm_merging_pages value also got reset to 0.
Meanwhile, the child process had not performed any merge itself, so the 
inherited
value remained unchanged. That’s why get_my_merging_page() in the child was
returning a non-zero value.

Initially, I fixed the issue by calling ksm_unmerge() before the fork(), 
and that
resolved the problem. Later, I decided it would be cleaner to move the
ksm_unmerge() call to the test cleanup phase.


>
>> 	munmap(map, size);
>> }
>>
>> @@ -338,6 +341,7 @@ static void test_unmerge_zero_pages(void)
>> 	ksft_test_result(!range_maps_duplicates(map, size),
>> 			"KSM zero pages were unmerged\n");
>> unmap:
>> +	ksm_unmerge();
>> 	munmap(map, size);
>> }
>>
>> @@ -366,6 +370,7 @@ static void test_unmerge_discarded(void)
>> 	ksft_test_result(!range_maps_duplicates(map, size),
>> 			 "Pages were unmerged\n");
>> unmap:
>> +	ksm_unmerge();
>> 	munmap(map, size);
>> }
>>
>> @@ -452,6 +457,7 @@ static void test_unmerge_uffd_wp(void)
>> close_uffd:
>> 	close(uffd);
>> unmap:
>> +	ksm_unmerge();
>> 	munmap(map, size);
>> }
>> #endif
>> @@ -515,6 +521,7 @@ static int test_child_ksm(void)
>> 	else if (map == MAP_MERGE_SKIP)
>> 		return -3;
>>
>> +	ksm_unmerge();
>> 	munmap(map, size);
>> 	return 0;
>> }
>> @@ -548,6 +555,7 @@ static void test_prctl_fork(void)
>>
>> 	child_pid = fork();
>> 	if (!child_pid) {
>> +		init_global_file_handles();
> Would this leave fd in parent as orphan?
>
>> 		exit(test_child_ksm());
>> 	} else if (child_pid < 0) {
>> 		ksft_test_result_fail("fork() failed\n");
>> @@ -595,7 +603,7 @@ static void test_prctl_fork_exec(void)
>> 		return;
>> 	} else if (child_pid == 0) {
>> 		char *prg_name = "./ksm_functional_tests";
>> -		char *argv_for_program[] = { prg_name, FORK_EXEC_CHILD_PRG_NAME };
>> +		char *argv_for_program[] = { prg_name, FORK_EXEC_CHILD_PRG_NAME, NULL };
>>
>> 		execv(prg_name, argv_for_program);
>> 		return;
>> @@ -644,6 +652,7 @@ static void test_prctl_unmerge(void)
>> 	ksft_test_result(!range_maps_duplicates(map, size),
>> 			 "Pages were unmerged\n");
>> unmap:
>> +	ksm_unmerge();
>> 	munmap(map, size);
>> }
>>
>> @@ -677,6 +686,7 @@ static void test_prot_none(void)
>> 	ksft_test_result(!range_maps_duplicates(map, size),
>> 			 "Pages were unmerged\n");
>> unmap:
>> +	ksm_unmerge();
>> 	munmap(map, size);
>> }
>>
>> -- 
>> 2.47.1
>>