Re: [PATCH v7 3/3] selftests/mm: add new selftests for KSM

[Stefan Roesch <shr@devkernel.io>]

David Hildenbrand <david@redhat.com> writes:

> Thanks for moving the functional tests. Some more feedback forksm_functional_tests change. Writing tests in the
> ksft testing framework can be a bit "special".
>
>
> I'm seeing some weird test failures due to
>
> prctl(PR_GET_MEMORY_MERGE, 0)
>
> Apparently, these go away when using
>
> prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0)
>

I changed the test programs to always specify all the 5 parameters.

> to explicitly force the other values to 0. Most probably, we should do that
> for PR_SET_MEMORY_MERGE as well (especially if we check for the arguments as
> well).
>
> [...]
>
>> @@ -15,8 +15,10 @@
>>   #include <errno.h>
>>   #include <fcntl.h>
>>   #include <sys/mman.h>
>> +#include <sys/prctl.h>
>>   #include <sys/syscall.h>
>>   #include <sys/ioctl.h>
>> +#include <sys/wait.h>
>>   #include <linux/userfaultfd.h>
>>     #include "../kselftest.h"
>> @@ -326,9 +328,80 @@ static void test_unmerge_uffd_wp(void)
>>   }
>>   #endif
>>   +/* Verify that KSM can be enabled / queried with prctl. */
>> +static void test_ksm_prctl(void)
>
> Maybe call this "test_prctl", because after all, these are all KSM tests.
>

I renamed it to test_prctl in the next version.

>> +{
>> +	bool ret = false;
>> +	int is_on;
>> +	int is_off;
>> +
>> +	ksft_print_msg("[RUN] %s\n", __func__);
>> +
>> +	if (prctl(PR_SET_MEMORY_MERGE, 1)) {
>> +		perror("prctl set");
>> +		goto out;
>> +	}
>> +
>> +	is_on = prctl(PR_GET_MEMORY_MERGE, 0);
>> +	if (prctl(PR_SET_MEMORY_MERGE, 0)) {
>> +		perror("prctl set");
>> +		goto out;
>> +	}
>> +
>> +	is_off = prctl(PR_GET_MEMORY_MERGE, 0);
>> +	if (is_on && is_off)
>> +		ret = true;
>> +
>> +out:
>> +	ksft_test_result(ret, "prctl get / set\n");
>
> The test fails if the kernel does not support PR_SET_MEMORY_MERGE.
>
>
> I'd modify this test to:
>
> (1) skip if the first PR_SET_MEMORY_MERGE=1 failed with EINVAL.
> (2) distinguish for PR_GET_MEMORY_MERGE whether it returned an error or
>     whether it returned a wrong value. Feel free to keep that as is, whatever
>     you prefer.
> (3) exit early for all failures, you get exactly one expected skip/pass/fail for the
>     test and use specific test failure messages.
> (4) Pass "0" for all other arguments of prctl.
>
>
> Something like:
>
> static void test_prctl(void)
> {
> 	int ret;
>
> 	ksft_print_msg("[RUN] %s\n", __func__);
>
> 	ret = prctl(PR_SET_MEMORY_MERGE, 1, 0, 0, 0);
> 	if (ret < 0 && errno == EINVAL){
> 		ksft_test_result_skip("PR_SET_MEMORY_MERGE not supported\n");
> 		return;
> 	} else if (ret) {
> 		ksft_test_result_fail("PR_SET_MEMORY_MERGE=1 failed\n");
> 		return;
> 	}
>
> 	ret = prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0);
> 	if (ret < 0) {
> 		ksft_test_result_fail("PR_GET_MEMORY_MERGE failed\n");
> 		return;
> 	} else if (ret != 1) {
> 		ksft_test_result_fail("PR_SET_MEMORY_MERGE=1 not effective\n");
> 		return;
> 	}
>
> 	ret = prctl(PR_SET_MEMORY_MERGE, 0, 0, 0, 0);
> 	if (ret){
> 		ksft_test_result_fail("PR_SET_MEMORY_MERGE=0 failed\n");
> 		return;
> 	}
>
> 	ret = prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0);
> 	if (ret < 0) {
> 		ksft_test_result_fail("PR_GET_MEMORY_MERGE failed\n");
> 		return;
> 	} else if (ret != 0) {
> 		ksft_test_result_fail("PR_SET_MEMORY_MERGE=0 not effective\n");
> 		return;
> 	}
>
> 	ksft_test_result_pass("Setting/clearing PR_SET_MEMORY_MERGE works\n");
> }
>
>

I made changes to the test program according to the code above.
>> +}
>> +
>> +/* Verify that prctl ksm flag is inherited. */
>> +static void test_ksm_fork(void)
>
> Maybe call it "test_prctl_fork"
>
I changed it to test_prctl_fork.

>> +{
>> +	int status;
>> +	bool ret = false;
>> +	pid_t child_pid;
>> +
>> +	ksft_print_msg("[RUN] %s\n", __func__);
>> +
>> +	if (prctl(PR_SET_MEMORY_MERGE, 1)) {
>> +		ksft_test_result_fail("prctl failed\n");
>> +		goto out;
>> +	}
>> +
>> +	child_pid = fork();
>> +	if (child_pid == 0) {
>> +		int is_on = +
>> +		if (!is_on)
>> +			exit(-1);
>> +
>> +		exit(0);
>> +	}
>> +
>> +	if (child_pid < 0) {
>> +		ksft_test_result_fail("child pid < 0\n");
>> +		goto out;> +
>> +	if (waitpid(child_pid, &status, 0) < 0 || WEXITSTATUS(status) != 0) {
>> +		ksft_test_result_fail("wait pid < 0\n");
>> +		goto out;
>> +	}
>> +
>> +	if (prctl(PR_SET_MEMORY_MERGE, 0))
>> +		ksft_test_result_fail("prctl 2 failed\n");
>> +	else
>> +		ret = true;
>> +
>> +out:
>> +	ksft_test_result(ret, "ksm_flag is inherited\n");
>> +}
>
> Again, test fails if kernel support is not around.
>
> I'd modify this test to:
>
> (1) skip if the first PR_SET_MEMORY_MERGE=1 failed with EINVAL just as in the other test.
> (2) Use a simple exit(prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0)); in the child.
> (3) exit early for all failures, you get exactly one expected skip/pass/fail for the
>     test and use specific test failure messages.
> (4) Split up the waitpid() check to test what failed.
> (5) Pass "0" for all other arguments of prctl.
>
>
> Something like:
>
> static void test_prctl_fork(void)
> {
> 	int ret, status;
> 	pid_t child_pid;
>
> 	ksft_print_msg("[RUN] %s\n", __func__);
>
> 	ret = prctl(PR_SET_MEMORY_MERGE, 1, 0, 0, 0);
> 	if (ret < 0 && errno == EINVAL){
> 		ksft_test_result_skip("PR_SET_MEMORY_MERGE not supported\n");
> 		return;
> 	} else if (ret) {
> 		ksft_test_result_fail("PR_SET_MEMORY_MERGE=1 failed\n");
> 		return;
> 	}
>
> 	child_pid = fork();
> 	if (!child_pid) {
> 		exit(prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0));
> 	} else if (child_pid < 0) {
> 		ksft_test_result_fail("fork() failed\n");
> 		return;
> 	}
>
> 	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");
> 		return;
> 	}
>
> 	if (prctl(PR_SET_MEMORY_MERGE, 0, 0, 0, 0)) {
> 		ksft_test_result_fail("PR_SET_MEMORY_MERGE=0 failed\n");
> 		return;
> 	}
>
> 	ksft_test_result_pass("PR_SET_MEMORY_MERGE value is inherited\n");
> }
>
>
>

I made changes to the test program according to the code above.
>> +
>>   int main(int argc, char **argv)
>>   {
>> -	unsigned int tests = 2;
>> +	unsigned int tests = 6;
>
> Assuming you execute exactly one ksft_test_result_skip/fail/pass on every path of your two
> test, this would become "4".
>
Changed it to 4.
>>   	int err;
>>     #ifdef __NR_userfaultfd
>> @@ -358,6 +431,8 @@ int main(int argc, char **argv)
>>   #ifdef __NR_userfaultfd
>>   	test_unmerge_uffd_wp();
>>   #endif
>> +	test_ksm_prctl();
>> +	test_ksm_fork();
>>
>
>
> With above outlined changes (feel free to integrate what you consider valuable),
> on an older kernel I get:
>
> $ sudo ./ksm_functional_tests
> TAP version 13
> 1..5
> # [RUN] test_unmerge
> ok 1 Pages were unmerged
> # [RUN] test_unmerge_discarded
> ok 2 Pages were unmerged
> # [RUN] test_unmerge_uffd_wp
> ok 3 Pages were unmerged
> # [RUN] test_prctl
> ok 4 # SKIP PR_SET_MEMORY_MERGE not supported
> # [RUN] test_prctl_fork
> ok 5 # SKIP PR_SET_MEMORY_MERGE not supported
> # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:2 error:0
>
>
> On a kernel with your patch #1:
>
> # ./ksm_functional_tests
> TAP version 13
> 1..5
> # [RUN] test_unmerge
> ok 1 Pages were unmerged
> # [RUN] test_unmerge_discarded
> ok 2 Pages were unmerged
> # [RUN] test_unmerge_uffd_wp
> ok 3 Pages were unmerged
> # [RUN] test_prctl
> ok 4 Setting/clearing PR_SET_MEMORY_MERGE works
> # [RUN] test_prctl_fork
> ok 5 PR_SET_MEMORY_MERGE value is inherited
> # Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0
>
>
>
>
>>   	err = ksft_get_fail_cnt();
>>   	if (err)
>> diff --git a/tools/testing/selftests/mm/ksm_tests.c b/tools/testing/selftests/mm/ksm_tests.c
>> index f9eb4d67e0dd..35b3828d44b4 100644
>> --- a/tools/testing/selftests/mm/ksm_tests.c
>> +++ b/tools/testing/selftests/mm/ksm_tests.c
>> @@ -1,6 +1,8 @@
>>   // SPDX-License-Identifier: GPL-2.0
>
> [...]
>
>
> Changes to ksm_tests mostly look good. Two comments:
>
>
>> -	if (ksm_merge_pages(map_ptr, page_size * page_count, start_time, timeout))
>> +	if (ksm_merge_pages(merge_type, map_ptr, page_size * page_count, start_time, timeout))
>>   		goto err_out;
>>     	/* verify that the right number of pages are merged */
>>   	if (assert_ksm_pages_count(page_count)) {
>>   		printf("OK\n");
>> -		munmap(map_ptr, page_size * page_count);
>> +		if (merge_type == KSM_MERGE_MADVISE)
>> +			munmap(map_ptr, page_size * page_count);
>> +		else if (merge_type == KSM_MERGE_PRCTL)
>> +			prctl(PR_SET_MEMORY_MERGE, 0);
>
> Are you sure that we don't want to unmap here? I'd assume we want to unmap in either way.
>
> [...]
>
I changed it to always unmap.

>> +		case 'd':
>> +			debug = 1;
>> +			break;
>>   		case 's':
>>   			size_MB = atoi(optarg);
>>   			if (size_MB <= 0) {
>>   				printf("Size must be greater than 0\n");
>>   				return KSFT_FAIL;
>>   			}
>> +		case 't':
>> +			{
>> +				int tmp = atoi(optarg);
>> +
>> +				if (tmp < 0 || tmp > KSM_MERGE_LAST) {
>> +					printf("Invalid merge type\n");
>> +					return KSFT_FAIL;
>> +				}
>> +				merge_type = atoi(optarg);
>
> You can simply reuse tmp
>
> merge_type = tmp;

Changed it.