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

[David Hildenbrand <david@redhat.com>]

On 16.07.25 10:27, Wei Yang wrote:
> There are some general ksm operations could be used by other related
> test case. Put them into vm_util for common use.
> 
> This is a preparation patch for later use.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Cc: Rik van Riel <riel@surriel.com>
> Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Harry Yoo <harry.yoo@oracle.com>
> 
> ---
> v1: open/close fd in function itself instead of pass as parameter
> ---
>   .../selftests/mm/ksm_functional_tests.c       |  95 ++-------------
>   tools/testing/selftests/mm/vm_util.c          | 111 ++++++++++++++++++
>   tools/testing/selftests/mm/vm_util.h          |   7 ++
>   3 files changed, 126 insertions(+), 87 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
> index 6da5c3340e10..0b151f3a29dc 100644
> --- a/tools/testing/selftests/mm/ksm_functional_tests.c
> +++ b/tools/testing/selftests/mm/ksm_functional_tests.c
> @@ -38,11 +38,6 @@ enum ksm_merge_mode {
>   };
>   
>   static int mem_fd;
> -static int ksm_fd;
> -static int ksm_full_scans_fd;
> -static int proc_self_ksm_stat_fd;
> -static int proc_self_ksm_merging_pages_fd;
> -static int ksm_use_zero_pages_fd;
>   static int pagemap_fd;
>   static size_t pagesize;
>   
> @@ -73,62 +68,6 @@ static bool range_maps_duplicates(char *addr, unsigned long size)
>   	return false;
>   }
>   
> -static long get_my_ksm_zero_pages(void)
> -{
> -	char buf[200];
> -	char *substr_ksm_zero;
> -	size_t value_pos;
> -	ssize_t read_size;
> -	unsigned long my_ksm_zero_pages;
> -
> -	if (proc_self_ksm_stat_fd < 0)
> -		return 0;
> -
> -	read_size = pread(proc_self_ksm_stat_fd, buf, sizeof(buf) - 1, 0);
> -	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;
> -}
> -
> -static long get_my_merging_pages(void)
> -{
> -	char buf[10];
> -	ssize_t ret;
> -
> -	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);
> -	if (ret <= 0)
> -		return -errno;
> -	buf[ret] = 0;
> -
> -	return strtol(buf, NULL, 10);
> -}
> -
> -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?

>   {
>   	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()?

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

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;
}

> -	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.


>   	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",
> -						O_RDONLY);
> -	ksm_use_zero_pages_fd = open("/sys/kernel/mm/ksm/use_zero_pages", O_RDWR);
>   }
>   
>   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)

> +{
> +	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;

> +
> +	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



-- 
Cheers,

David / dhildenb