David Hildenbrand <david@xxxxxxxxxx> 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.