Re: [PATCH v2 2/2] selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec

[David Hildenbrand <david@redhat.com>]

On 22.03.24 07:09, Jinjiang Tu wrote:
> This extends test_prctl_fork() and test_prctl_fork_exec() to make sure
> that deduplication really happens, instead of only test the
> MMF_VM_MERGE_ANY flag is set.
> 
> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
> ---
>   .../selftests/mm/ksm_functional_tests.c       | 79 +++++++++++++++++--
>   1 file changed, 73 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
> index d615767e396b..01999aab2e37 100644
> --- a/tools/testing/selftests/mm/ksm_functional_tests.c
> +++ b/tools/testing/selftests/mm/ksm_functional_tests.c
> @@ -146,6 +146,54 @@ static int ksm_unmerge(void)
>   	return 0;
>   }
>   
> +static int child_test_merge(void)
> +{
> +	const unsigned int size = 2 * MiB;
> +	char *map;
> +	int ret = -1;
> +
> +	/* Stabilize accounting by disabling KSM completely. */
> +	if (ksm_unmerge()) {
> +		ksft_print_msg("Disabling (unmerging) KSM failed\n");
> +		return ret;
> +	}
> +
> +	if (get_my_merging_pages() > 0) {
> +		ksft_print_msg("Still pages merged\n");
> +		return ret;
> +	}
> +
> +	map = mmap(NULL, size, PROT_READ|PROT_WRITE,
> +		   MAP_PRIVATE|MAP_ANON, -1, 0);
> +	if (map == MAP_FAILED) {
> +		ksft_print_msg("mmap() failed\n");
> +		return ret;
> +	}
> +
> +	/* Don't use THP. Ignore if THP are not around on a kernel. */
> +	if (madvise(map, size, MADV_NOHUGEPAGE) && errno != EINVAL) {
> +		ksft_print_msg("MADV_NOHUGEPAGE failed\n");
> +		goto unmap;
> +	}
> +
> +	memset(map, 0x1c, size);
> +
> +	if (ksm_merge()) {
> +		ksft_print_msg("Running KSM failed\n");
> +		goto unmap;
> +	}
> +
> +	if (get_my_merging_pages() <= 0) {
> +		ksft_print_msg("Fail to merge\n");
> +		goto unmap;
> +	}

Looks like all you want is use mmap_and_merge_range(), but neither 
setting the prctl nor madvise().

Two alternatives:

1) switching from "bool use_prctl" to an enum like

enum ksm_merge_mode {
	KSM_MERGE_PRCTL
	KSM_MERGE_MADVISE,
	KSM_MERGE_NONE, /* PRCTL already set */
};

Then, you can simply use mmap_and_merge_range(0x1c, 2 * MiB, 
PROT_READ|PROT_WRITE, KSM_MERGE_NONE);

2) With "bool use_prctl", before doing the prctl(PR_SET_MEMORY_MERGE, 
...), check if it is already enabled.

As we do that already in ksm_fork_exec_child(), and fail if it isn't 
set, that should work.

Then, you can simply use mmap_and_merge_range(0x1c, 2 * MiB, 
PROT_READ|PROT_WRITE, true);

here.

> +
> +	ret = 0;
> +unmap:
> +	munmap(map, size);
> +	return ret;
> +}
> +
>   static char *mmap_and_merge_range(char val, unsigned long size, int prot,
>   				  bool use_prctl)
>   {
> @@ -458,7 +506,11 @@ static void test_prctl_fork(void)
>   
>   	child_pid = fork();
>   	if (!child_pid) {
> -		exit(prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0));
> +		if (prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0) != 1)
> +			exit(-1);
> +		if (child_test_merge() != 0)
> +			exit(-2);
> +		exit(0);
>   	} else if (child_pid < 0) {
>   		ksft_test_result_fail("fork() failed\n");
>   		return;
> @@ -467,8 +519,14 @@ static void test_prctl_fork(void)
>   	if (waitpid(child_pid, &status, 0) < 0) {
>   		ksft_test_result_fail("waitpid() failed\n");
>   		return;
> -	} else if (WEXITSTATUS(status) != 1) {
> -		ksft_test_result_fail("unexpected PR_GET_MEMORY_MERGE result in child\n");
> +	}
> +
> +	status = WEXITSTATUS(status);
> +	if (status != 0) {
> +		if (status == -1)
> +			ksft_test_result_fail("unexpected PR_GET_MEMORY_MERGE result in child\n");
> +		else
> +			ksft_test_result_fail("fail to merge in child\n");
>   		return;
>   	}
>   
> @@ -483,7 +541,13 @@ static void test_prctl_fork(void)
>   static int ksm_fork_exec_child(void)
>   {
>   	/* Test if KSM is enabled for the process. */
> -	return prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0) == 1;
> +	if (prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0) != 1)
> +		return -1;
> +
> +	if (child_test_merge() != 0)

You can drop the "!=0". But maybe, you can just inline the call to 
mmap_and_merge_range() here.

> +		return -2;
> +
> +	return 0;


-- 
Cheers,

David / dhildenb