Re: [PATCH v3 2/3] selftest/mm: ksm_functional_tests: refactor mmap_and_merge_range()

[David Hildenbrand <david@redhat.com>]

On 27.03.24 07:09, Jinjiang Tu wrote:
> In order to extend test_prctl_fork() and test_prctl_fork_exec() to make
> sure that deduplication really happens, mmap_and_merge_range() needs to be
> refactored.
> 
> Firstly, mmap_and_merge_range() will be called with no need to call enable
> KSM by madvise or prctl. So, switch the 'bool use_prctl' parameter to enum
> ksm_merge_mode.
> 
> Secondly, mmap_and_merge_range() will be called in child process in the
> two testcases, it isn't appropriate to call ksft_test_result_{fail, skip},
> because the global variables ksft_{fail, skip} aren't consistent with the
> parent process. Thus, convert calls of ksft_test_result_{fail, skip} to
> ksft_print_msg() and return differrent error according to the two cases.
> The callers of mmap_and_merge_range() then call ksft_test_result_{fail,
> skip} according to the return value. Introduce mmap_and_merge_err_print()
> helper to make it easier.
> 
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
> ---

[...]

>   
> @@ -209,14 +228,28 @@ static char *mmap_and_merge_range(char val, unsigned long size, int prot,
>   	 * accounted differently (depending on kernel support).
>   	 */
>   	if (val && !get_my_merging_pages()) {
> -		ksft_test_result_fail("No pages got merged\n");
> +		ksft_print_msg("No pages got merged\n");
>   		goto unmap;
>   	}
>   
>   	return map;
>   unmap:
>   	munmap(map, size);
> -	return MAP_FAILED;
> +	return err_map;
> +}
> +
> +static inline int mmap_and_merge_err_print(char *map)
> +{
> +	int ret = -1;
> +
> +	if (map == MAP_MERGE_FAIL)
> +		ksft_test_result_fail("Merging memory failed");
> +	else if (map == MAP_MERGE_FAIL)

MAP_MERGE_SKIP

> +		ksft_test_result_skip("Merging memory skiped");

"skipped"

> +	else
> +		ret = 0;
> +
> +	return ret;
>   }
>   
>   static void test_unmerge(void)
> @@ -226,8 +259,8 @@ static void test_unmerge(void)
>   
>   	ksft_print_msg("[RUN] %s\n", __func__);
>   
> -	map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, false);
> -	if (map == MAP_FAILED)
> +	map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, KSM_MERGE_MADVISE);
> +	if (mmap_and_merge_err_print(map))

I like the concept.

The following would be cleaner and result in less churn:

1) Rename mmap_and_merge_range() to "__mmap_and_merge_range()"

2) Introduce a new mmap_and_merge_range()

static inline char *mmap_and_merge_range(char val, unsigned long size,
		int prot, enum ksm_merge_mode mode)
{
	char *map = __mmap_and_merge_range(val, size, prot, mode);	
	char *ret = MAP_FAILED;

	if (map == MAP_MERGE_FAIL)
		ksft_test_result_fail("Merging memory failed");
	else if (map == MAP_MERGE_SKIP)
		ksft_test_result_skip("Merging memory skipped");
	else
		ret = map;

	return ret;
}

3) Use __mmap_and_merge_range() in patch #3.

No need to adjust any existing callers of mmap_and_merge_range().

-- 
Cheers,

David / dhildenb