Re: [PATCH 2/3] selftests/mm: put general ksm operation into vm_util

[Wei Yang <richard.weiyang@gmail.com>]

Thanks for the detailed comment.

On Wed, Jul 16, 2025 at 11:20:09AM +0200, David Hildenbrand wrote:
>On 16.07.25 10:27, Wei Yang wrote:
[...]
>> -static long ksm_get_full_scans(void)
>> -{
>> -	char buf[10];
>> -	ssize_t ret;
>> -
>> -	ret = pread(ksm_full_scans_fd, buf, sizeof(buf) - 1, 0);
>> -	if (ret <= 0)
>> -		return -errno;
>> -	buf[ret] = 0;
>> -
>> -	return strtol(buf, NULL, 10);
>> -}
>> -
>>   static int ksm_merge(void)
>
>Should ksm_merge() get factored out as well?
>

Reasonable, will factor out.

While one thing interesting is if it scan 2 full round, the ksm case in patch
3 would fail sometimes. But I don't see the failure in ksm_functional_test
here.

>>   {
>>   	long start_scans, end_scans;
>> @@ -137,7 +76,7 @@ static int ksm_merge(void)
>>   	start_scans = ksm_get_full_scans();
>>   	if (start_scans < 0)
>>   		return start_scans;
>> -	if (write(ksm_fd, "1", 1) != 1)
>> +	if (ksm_start_and_merge())
>>   		return -errno;
>>   	do {
>>   		end_scans = ksm_get_full_scans();
>> @@ -150,7 +89,7 @@ static int ksm_merge(void)
>>   static int ksm_unmerge(void)
>>   {
>> -	if (write(ksm_fd, "2", 1) != 1)
>> +	if (ksm_stop_and_unmerge())
>>   		return -errno;
>>   	return 0;
>
>What's the reason of gaving ksm_unmerge() and ksm_stop_and_unmerge()?
>

My plan is there are two pairs of helper:

    ksm_merge()  <->  ksm_unmerge()
    ksm_start_and_merge() <-> ksm_stop_and_unmerge()

>Probably we should just use ksm_stop_and_unmerge() and remove ksm_unmerge().
>

Looks reasonable, will remove it.

So how about leave three helpers:

   ksm_merge()
   ksm_start()
   ksm_stop_and_unmerge()

Would this be better?

>See below regarding letting ksm_stop_and_unmerge() and friends return -errno in case of error.
>
>>   }
>> @@ -168,7 +107,7 @@ static char *__mmap_and_merge_range(char val, unsigned long size, int prot,
>>   		return err_map;
>>   	}
>> -	if (get_my_merging_pages() > 0) {
>> +	if (ksm_get_self_merging_pages() > 0) {
>>   		ksft_print_msg("Still pages merged\n");
>>   		return err_map;
>>   	}
>> @@ -227,7 +166,7 @@ static char *__mmap_and_merge_range(char val, unsigned long size, int prot,
>>   	 * Check if anything was merged at all. Ignore the zero page that is
>>   	 * accounted differently (depending on kernel support).
>>   	 */
>> -	if (val && !get_my_merging_pages()) {
>> +	if (val && !ksm_get_self_merging_pages()) {
>>   		ksft_print_msg("No pages got merged\n");
>>   		goto unmap;
>>   	}
>> @@ -286,15 +225,7 @@ static void test_unmerge_zero_pages(void)
>>   	ksft_print_msg("[RUN] %s\n", __func__);
>> -	if (proc_self_ksm_stat_fd < 0) {
>> -		ksft_test_result_skip("open(\"/proc/self/ksm_stat\") failed\n");
>> -		return;
>> -	}
>
>See below: probably we should do a test read so we know the file exists and
>can be read. So we don't get misleading errors later on older kernels.
>
>if (ksm_get_self_zero_pages() < 0) {
>	ksft_test_result_skip("accessing \"/proc/self/ksm_stat\" failed\n");
>	return;
>}

Agree

>
>> -	if (ksm_use_zero_pages_fd < 0) {
>> -		ksft_test_result_skip("open \"/sys/kernel/mm/ksm/use_zero_pages\" failed\n");
>> -		return;
>> -	}
>> -	if (write(ksm_use_zero_pages_fd, "1", 1) != 1) {
>> +	if (ksm_use_zero_pages()) {
>>   		ksft_test_result_skip("write \"/sys/kernel/mm/ksm/use_zero_pages\" failed\n");
>>   		return;
>>   	}
>> @@ -306,7 +237,7 @@ static void test_unmerge_zero_pages(void)
>>   	/* Check if ksm_zero_pages is updated correctly after KSM merging */
>>   	pages_expected = size / pagesize;
>> -	if (pages_expected != get_my_ksm_zero_pages()) {
>> +	if (pages_expected != ksm_get_self_zero_pages()) {
>>   		ksft_test_result_fail("'ksm_zero_pages' updated after merging\n");
>>   		goto unmap;
>>   	}
>> @@ -319,7 +250,7 @@ static void test_unmerge_zero_pages(void)
>>   	/* Check if ksm_zero_pages is updated correctly after unmerging */
>>   	pages_expected /= 2;
>> -	if (pages_expected != get_my_ksm_zero_pages()) {
>> +	if (pages_expected != ksm_get_self_zero_pages()) {
>>   		ksft_test_result_fail("'ksm_zero_pages' updated after unmerging\n");
>>   		goto unmap;
>>   	}
>> @@ -329,7 +260,7 @@ static void test_unmerge_zero_pages(void)
>>   		*((unsigned int *)&map[offs]) = offs;
>>   	/* Now we should have no zeropages remaining. */
>> -	if (get_my_ksm_zero_pages()) {
>> +	if (ksm_get_self_zero_pages()) {
>>   		ksft_test_result_fail("'ksm_zero_pages' updated after write fault\n");
>>   		goto unmap;
>>   	}
>> @@ -685,19 +616,9 @@ static void init_global_file_handles(void)
>>   	mem_fd = open("/proc/self/mem", O_RDWR);
>>   	if (mem_fd < 0)
>>   		ksft_exit_fail_msg("opening /proc/self/mem failed\n");
>> -	ksm_fd = open("/sys/kernel/mm/ksm/run", O_RDWR);
>> -	if (ksm_fd < 0)
>> -		ksft_exit_skip("open(\"/sys/kernel/mm/ksm/run\") failed\n");
>> -	ksm_full_scans_fd = open("/sys/kernel/mm/ksm/full_scans", O_RDONLY);
>> -	if (ksm_full_scans_fd < 0)
>> -		ksft_exit_skip("open(\"/sys/kernel/mm/ksm/full_scans\") failed\n");
>
>For these skip cases, we should probably do a test access.
>
>if (ksm_stop_and_unmerge() < 0)
>	ksft_exit_skip("accessing \"/sys/kernel/mm/ksm/run\\") failed\n");
>if (ksm_get_full_scans() < 0)
>	ksft_exit_skip("accessing \"/sys/kernel/mm/ksm/full_scans\") failed\n");
>
>So we later now that the files actually do exist and can be opened+written.
>

Agree

>
>>   	pagemap_fd = open("/proc/self/pagemap", O_RDONLY);>   	if (pagemap_fd < 0)
>>   		ksft_exit_skip("open(\"/proc/self/pagemap\") failed\n");
>> -	proc_self_ksm_stat_fd = open("/proc/self/ksm_stat", O_RDONLY);
>> -	proc_self_ksm_merging_pages_fd = open("/proc/self/ksm_merging_pages",

And I think we may do similar thing here.

  if (ksm_get_self_merging_pages() < 0)
	ksft_exit_skip("accessing \"/proc/self/ksm_merging_pages\") failed\n");

>> -						O_RDONLY);
>> -	ksm_use_zero_pages_fd = open("/sys/kernel/mm/ksm/use_zero_pages", O_RDWR);

proc_self_ksm_stat_fd and ksm_use_zero_pages_fd  is only used in
test_unmerge_zero_pages(). So we can test and skip only there.

For proc_self_ksm_merging_pages_fd, it is used in all tests except
test_prctl(). Check ahead maybe helpful.

>>   }
>>   int main(int argc, char **argv)
>> diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
>> index 9dafa7669ef9..14973d957c9a 100644
>> --- a/tools/testing/selftests/mm/vm_util.c
>> +++ b/tools/testing/selftests/mm/vm_util.c
>> @@ -555,3 +555,114 @@ bool detect_huge_zeropage(void)
>>   	close(fd);
>>   	return enabled;
>>   }
>> +
>> +long ksm_get_self_zero_pages()
>
>For all these functions without parameters:
>
>(void)
>

Thanks will add it.

>> +{
>> +	int proc_self_ksm_stat_fd;
>> +	char buf[200];
>> +	char *substr_ksm_zero;
>> +	size_t value_pos;
>> +	ssize_t read_size;
>> +	unsigned long my_ksm_zero_pages;
>> +
>> +	proc_self_ksm_stat_fd = open("/proc/self/ksm_stat", O_RDONLY);
>> +	if (proc_self_ksm_stat_fd < 0)
>> +		return 0;
>> +
>> +	read_size = pread(proc_self_ksm_stat_fd, buf, sizeof(buf) - 1, 0);
>> +	close(proc_self_ksm_stat_fd);
>> +	if (read_size < 0)
>> +		return -errno;
>> +
>> +	buf[read_size] = 0;
>> +
>> +	substr_ksm_zero = strstr(buf, "ksm_zero_pages");
>> +	if (!substr_ksm_zero)
>> +		return 0;
>> +
>> +	value_pos = strcspn(substr_ksm_zero, "0123456789");
>> +	my_ksm_zero_pages = strtol(substr_ksm_zero + value_pos, NULL, 10);
>> +
>> +	return my_ksm_zero_pages;
>> +}
>> +
>> +long ksm_get_self_merging_pages()
>> +{
>> +	int proc_self_ksm_merging_pages_fd;
>> +	char buf[10];
>> +	ssize_t ret;
>> +
>> +	proc_self_ksm_merging_pages_fd = open("/proc/self/ksm_merging_pages",
>> +						O_RDONLY);
>> +	if (proc_self_ksm_merging_pages_fd < 0)
>> +		return proc_self_ksm_merging_pages_fd;
>> +
>> +	ret = pread(proc_self_ksm_merging_pages_fd, buf, sizeof(buf) - 1, 0);
>> +	close(proc_self_ksm_merging_pages_fd);
>> +	if (ret <= 0)
>> +		return -errno;
>> +	buf[ret] = 0;
>> +
>> +	return strtol(buf, NULL, 10);
>> +}
>> +
>> +long ksm_get_full_scans()
>> +{
>> +	int ksm_full_scans_fd;
>> +	char buf[10];
>> +	ssize_t ret;
>> +
>> +	ksm_full_scans_fd = open("/sys/kernel/mm/ksm/full_scans", O_RDONLY);
>> +	if (ksm_full_scans_fd < 0)
>> +		return ksm_full_scans_fd;
>> +
>> +	ret = pread(ksm_full_scans_fd, buf, sizeof(buf) - 1, 0);
>> +	close(ksm_full_scans_fd);
>> +	if (ret <= 0)
>> +		return -errno;
>> +	buf[ret] = 0;
>> +
>> +	return strtol(buf, NULL, 10);
>> +}
>> +
>> +int ksm_use_zero_pages()
>> +{
>> +	int ksm_use_zero_pages_fd;
>> +	ssize_t ret;
>> +
>> +	ksm_use_zero_pages_fd = open("/sys/kernel/mm/ksm/use_zero_pages", O_RDWR);
>> +	if (ksm_use_zero_pages_fd < 0)
>> +		return -1;
>
>For all these functions, we should not require the caller to lookup errno but
>instead return it.
>
>	return -errno;
>

Agree on the -errno thing, will change in all places.

>> +
>> +	ret = write(ksm_use_zero_pages_fd, "1", 1);
>> +	close(ksm_use_zero_pages_fd);
>> +	return ret == 1 ? 0 : ret;
>
>return ret == 1 ? 0 : -errno;
>
>> +}
>> +
>> +int ksm_start_and_merge()
>> +{
>> +	int ksm_fd;
>> +	ssize_t ret;
>> +
>> +	ksm_fd = open("/sys/kernel/mm/ksm/run", O_RDWR);
>> +	if (ksm_fd < 0)
>> +		return -1;
>
>Same comments regarding errno.
>
>> +
>> +	ret = write(ksm_fd, "1", 1);
>> +	close(ksm_fd);
>> +	return ret == 1 ?  0 : ret;
>
>double space
>
>> +}
>> +
>> +int ksm_stop_and_unmerge()
>> +{
>> +	int ksm_fd;
>> +	ssize_t ret;
>> +
>> +	ksm_fd = open("/sys/kernel/mm/ksm/run", O_RDWR);
>> +	if (ksm_fd < 0)
>> +		return -1;
>> +
>> +	ret = write(ksm_fd, "2", 1);
>> +	close(ksm_fd);
>> +	return ret == 1 ?  0 : ret;
>
>double space
>

Shame on me :-(

>
>
>-- 
>Cheers,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me