| Message ID | 20240322060947.3254967-3-tujinjiang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/ksm: fix ksm exec support for prctl | expand |
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;
在 2024/3/22 19:43, David Hildenbrand 写道: > 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); I have considered this before. But, mmap_and_merge_range() calls ksft_test_result_fail() when error occurs, ksft_test_result_fail() prints prefixed with ksft_fail count. When mmap_and_merge_range() is called in the child process, the ksft_fail isn't consisent with the parent process due to the global variable ksft_fail is CoWed. As a result, ksft_print_msg() is intended to be called in child process. Maybe, We could introduce a macro ksm_print() to control which function is called according to ksm_merge_mode : #define ksm_print(mode, fmt, ...) do { \ if ((mode) == KSM_MERGE_NONE) \ ksft_print_msg(fmt, ##__VA_ARGS__);\ else \ ksft_test_result_fail(fmt, ##__VA_ARGS__);\ } while (0) > > 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; > >
On 25.03.24 03:24, Jinjiang Tu wrote: > > 在 2024/3/22 19:43, David Hildenbrand 写道: >> 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); > I have considered this before. But, mmap_and_merge_range() calls > ksft_test_result_fail() when error occurs, ksft_test_result_fail() > prints prefixed with ksft_fail count. When mmap_and_merge_range() is > called in the child process, the ksft_fail isn't consisent with the > parent process due to the global variable ksft_fail is CoWed. As a > result, ksft_print_msg() is intended to be called in child process. > > Maybe, We could introduce a macro ksm_print() to control which function > is called according to ksm_merge_mode : As an alternative, convert all ksft_test_result_fail() in there into ksft_print_msg(), and in the callers of mmap_and_merge_range(), do something like map = mmap_and_merge_range() ... if (map == MAP_FAILED) { ksft_test_result_fail("Merging memory failed"); return; }
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; + } + + 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) + return -2; + + return 0; } static void test_prctl_fork_exec(void) @@ -517,9 +581,12 @@ static void test_prctl_fork_exec(void) if (waitpid(child_pid, &status, 0) > 0) { if (WIFEXITED(status)) { status = WEXITSTATUS(status); - if (status) { + if (status == -1) { ksft_test_result_fail("KSM not enabled\n"); return; + } else if (status == -2) { + ksft_test_result_fail("fail to merge in child\n"); + return; } } else { ksft_test_result_fail("program didn't terminate normally\n"); @@ -599,7 +666,7 @@ int main(int argc, char **argv) int err; if (argc > 1 && !strcmp(argv[1], FORK_EXEC_CHILD_PRG_NAME)) { - exit(ksm_fork_exec_child() == 1 ? 0 : 1); + exit(ksm_fork_exec_child()); } #ifdef __NR_userfaultfd
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(-)