| Message ID | 20230822015501.791637-7-joel@joelfernandes.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Optimize mremap during mutual alignment within PMD | expand |
On Tue, Aug 22, 2023 at 01:54:59AM +0000, Joel Fernandes (Google) wrote: > Move a block of memory within a memory range. Any alignment optimization > on the source address may cause corruption. Verify using kselftest that > it works. I have also verified with tracing that such optimization does > not happen due to this check in can_align_down(): > > if (!for_stack && vma->vm_start <= addr_masked) > return false; > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > --- > tools/testing/selftests/mm/mremap_test.c | 79 +++++++++++++++++++++++- > 1 file changed, 78 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/mm/mremap_test.c b/tools/testing/selftests/mm/mremap_test.c > index d7366074e2a8..f45d1abedc9c 100644 > --- a/tools/testing/selftests/mm/mremap_test.c > +++ b/tools/testing/selftests/mm/mremap_test.c > @@ -23,6 +23,7 @@ > #define VALIDATION_NO_THRESHOLD 0 /* Verify the entire region */ > > #define MIN(X, Y) ((X) < (Y) ? (X) : (Y)) > +#define SIZE_MB(m) ((size_t)m * (1024 * 1024)) Nit in this instance since you always just use an integer, but generally for good practice's sake - shouldn't we place m in parens e.g. (size_t)(m) to avoid broken macro expansion? > > struct config { > unsigned long long src_alignment; > @@ -226,6 +227,79 @@ static void mremap_expand_merge_offset(FILE *maps_fp, unsigned long page_size) > ksft_test_result_fail("%s\n", test_name); > } > > +/* > + * Verify that an mremap within a range does not cause corruption > + * of unrelated part of range. > + * > + * Consider the following range which is 2MB aligned and is > + * a part of a larger 10MB range which is not shown. Each > + * character is 256KB below making the source and destination > + * 2MB each. The lower case letters are moved (s to d) and the > + * upper case letters are not moved. The below test verifies > + * that the upper case S letters are not corrupted by the > + * adjacent mremap. > + * > + * |DDDDddddSSSSssss| > + */ > +static void mremap_move_within_range(char pattern_seed) > +{ > + char *test_name = "mremap mremap move within range"; > + void *src, *dest; > + int i, success = 1; > + > + size_t size = SIZE_MB(20); > + void *ptr = mmap(NULL, size, PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > + if (ptr == MAP_FAILED) { > + perror("mmap"); > + success = 0; > + goto out; > + } > + memset(ptr, 0, size); > + > + src = ptr + SIZE_MB(6); > + src = (void *)((unsigned long)src & ~(SIZE_MB(2) - 1)); > + It's nitty I know for a test (sorry!) but it'd be nice to place a bitwise alignment trick like this into a helper function or macro if it isn't otherwise avialable. > + /* Set byte pattern for source block. */ > + srand(pattern_seed); > + for (i = 0; i < SIZE_MB(2); i++) { > + ((char *)src)[i] = (char) rand(); > + } > + > + dest = src - SIZE_MB(2); > + > + void *new_ptr = mremap(src + SIZE_MB(1), SIZE_MB(1), SIZE_MB(1), > + MREMAP_MAYMOVE | MREMAP_FIXED, dest + SIZE_MB(1)); > + if (new_ptr == MAP_FAILED) { > + perror("mremap"); > + success = 0; > + goto out; > + } > + > + /* Verify byte pattern after remapping */ > + srand(pattern_seed); > + for (i = 0; i < SIZE_MB(1); i++) { > + char c = (char) rand(); > + > + if (((char *)src)[i] != c) { > + ksft_print_msg("Data at src at %d got corrupted due to unrelated mremap\n", > + i); > + ksft_print_msg("Expected: %#x\t Got: %#x\n", c & 0xff, > + ((char *) src)[i] & 0xff); > + success = 0; > + } > + } > + > +out: > + if (munmap(ptr, size) == -1) > + perror("munmap"); > + > + if (success) > + ksft_test_result_pass("%s\n", test_name); > + else > + ksft_test_result_fail("%s\n", test_name); > +} > + > /* > * Returns the start address of the mapping on success, else returns > * NULL on failure. > @@ -491,6 +565,7 @@ int main(int argc, char **argv) > unsigned int threshold_mb = VALIDATION_DEFAULT_THRESHOLD; > unsigned int pattern_seed; > int num_expand_tests = 2; > + int num_misc_tests = 1; > struct test test_cases[MAX_TEST] = {}; > struct test perf_test_cases[MAX_PERF_TEST]; > int page_size; > @@ -572,7 +647,7 @@ int main(int argc, char **argv) > (threshold_mb * _1MB >= _1GB); > > ksft_set_plan(ARRAY_SIZE(test_cases) + (run_perf_tests ? > - ARRAY_SIZE(perf_test_cases) : 0) + num_expand_tests); > + ARRAY_SIZE(perf_test_cases) : 0) + num_expand_tests + num_misc_tests); > > for (i = 0; i < ARRAY_SIZE(test_cases); i++) > run_mremap_test_case(test_cases[i], &failures, threshold_mb, > @@ -590,6 +665,8 @@ int main(int argc, char **argv) > > fclose(maps_fp); > > + mremap_move_within_range(pattern_seed); > + > if (run_perf_tests) { > ksft_print_msg("\n%s\n", > "mremap HAVE_MOVE_PMD/PUD optimization time comparison for 1GB region:"); > -- > 2.42.0.rc1.204.g551eb34607-goog > I drew a little diagram for myself and was thereby convinced this was a good test, therefore, Reviewed-by: Lorenzo Stoakes <lstoakes@gmail.com>
On Sun, Aug 27, 2023 at 10:57:59AM +0100, Lorenzo Stoakes wrote: [snip] > > +/* > > + * Verify that an mremap within a range does not cause corruption > > + * of unrelated part of range. > > + * > > + * Consider the following range which is 2MB aligned and is > > + * a part of a larger 10MB range which is not shown. Each > > + * character is 256KB below making the source and destination Just noticed, I think you misspeak here, as this test doens't seem to offset by 256 KiB? That is the strategy for mremap_move_1mb_from_start() rather than this test so perhaps comment needs to be moved around? * 2MB each. The lower case letters are moved (s to d) and the * upper case letters are not moved. The below test verifies * that the upper case S letters are not corrupted by the * adjacent mremap. * * |DDDDddddSSSSssss| */ static void mremap_move_within_range(char pattern_seed) [snip]
On Sun, Aug 27, 2023 at 10:57:59AM +0100, Lorenzo Stoakes wrote: > On Tue, Aug 22, 2023 at 01:54:59AM +0000, Joel Fernandes (Google) wrote: > > Move a block of memory within a memory range. Any alignment optimization > > on the source address may cause corruption. Verify using kselftest that > > it works. I have also verified with tracing that such optimization does > > not happen due to this check in can_align_down(): > > > > if (!for_stack && vma->vm_start <= addr_masked) > > return false; > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > --- > > tools/testing/selftests/mm/mremap_test.c | 79 +++++++++++++++++++++++- > > 1 file changed, 78 insertions(+), 1 deletion(-) > > > > diff --git a/tools/testing/selftests/mm/mremap_test.c b/tools/testing/selftests/mm/mremap_test.c > > index d7366074e2a8..f45d1abedc9c 100644 > > --- a/tools/testing/selftests/mm/mremap_test.c > > +++ b/tools/testing/selftests/mm/mremap_test.c > > @@ -23,6 +23,7 @@ > > #define VALIDATION_NO_THRESHOLD 0 /* Verify the entire region */ > > > > #define MIN(X, Y) ((X) < (Y) ? (X) : (Y)) > > +#define SIZE_MB(m) ((size_t)m * (1024 * 1024)) > > Nit in this instance since you always just use an integer, but generally > for good practice's sake - shouldn't we place m in parens e.g. (size_t)(m) > to avoid broken macro expansion? Sure, I'll do that. Thanks. > I drew a little diagram for myself and was thereby convinced this was a good test, therefore, > > Reviewed-by: Lorenzo Stoakes <lstoakes@gmail.com> Thanks a lot! - Joel
On Sun, Aug 27, 2023 at 11:15:20AM +0100, Lorenzo Stoakes wrote: > On Sun, Aug 27, 2023 at 10:57:59AM +0100, Lorenzo Stoakes wrote: > [snip] > > > > +/* > > > + * Verify that an mremap within a range does not cause corruption > > > + * of unrelated part of range. > > > + * > > > + * Consider the following range which is 2MB aligned and is > > > + * a part of a larger 10MB range which is not shown. Each > > > + * character is 256KB below making the source and destination > > Just noticed, I think you misspeak here, as this test doens't seem to > offset by 256 KiB? That is the strategy for mremap_move_1mb_from_start() > rather than this test so perhaps comment needs to be moved around? > > * 2MB each. The lower case letters are moved (s to d) and the > * upper case letters are not moved. The below test verifies > * that the upper case S letters are not corrupted by the > * adjacent mremap. > * > * |DDDDddddSSSSssss| > */ > static void mremap_move_within_range(char pattern_seed) Here we are moving 1MB within a 4MB zone of a large mapping. Each character 's' or 'd' is 256KB. The 256KB there is just for illustration and not really significant as such. The 'ssss' is moved to 'dddd' 1MB each. Here we make sure that this move did not accidentally corrupt 'SSSS' and 'DDDD' due to alignment optimization. Basically to protect from this, we check in the code that the source address is beginning of the VMA: + if (vma->vm_start != addr_to_align) + return false; But you did point an issue which is I need to change the comment from 'larger 10MB' to 'larger 20MB'. In the mremap_move_1mb_from_start() test, I request for an alignment of 1.25MB so that when I align down, I fall no mapping. This is to catch a bug that Linus found which is that just because an aligned down address did not fall on a mapping, that doesn't mean we can just move it at PMD-level otherwise we destroy the mapping. I do need to update the test name in mremap_move_1mb_from_start() to: "mremap move 1mb from start at 1MB+256KB aligned src". So thanks for point this! Would that sort it out or is there still something in the comment I am missing? Thanks! - Joel > > [snip]
On Mon, Aug 28, 2023 at 06:59:12PM +0000, Joel Fernandes wrote: > On Sun, Aug 27, 2023 at 11:15:20AM +0100, Lorenzo Stoakes wrote: > > On Sun, Aug 27, 2023 at 10:57:59AM +0100, Lorenzo Stoakes wrote: > > [snip] > > > > > > +/* > > > > + * Verify that an mremap within a range does not cause corruption > > > > + * of unrelated part of range. > > > > + * > > > > + * Consider the following range which is 2MB aligned and is > > > > + * a part of a larger 10MB range which is not shown. Each > > > > + * character is 256KB below making the source and destination > > > > Just noticed, I think you misspeak here, as this test doens't seem to > > offset by 256 KiB? That is the strategy for mremap_move_1mb_from_start() > > rather than this test so perhaps comment needs to be moved around? > > > > * 2MB each. The lower case letters are moved (s to d) and the > > * upper case letters are not moved. The below test verifies > > * that the upper case S letters are not corrupted by the > > * adjacent mremap. > > * > > * |DDDDddddSSSSssss| > > */ > > static void mremap_move_within_range(char pattern_seed) > > Here we are moving 1MB within a 4MB zone of a large mapping. Each character > 's' or 'd' is 256KB. The 256KB there is just for illustration and not really > significant as such. The 'ssss' is moved to 'dddd' 1MB each. Here we make Ahhh I see. I find that a little confusing here, perhaps clearer to say 'each block of letters is 1 MiB in size' or something? > sure that this move did not accidentally corrupt 'SSSS' and 'DDDD' due to > alignment optimization. Basically to protect from this, we check in the code > that the source address is beginning of the VMA: > + if (vma->vm_start != addr_to_align) > + return false; > > But you did point an issue which is I need to change the comment from 'larger > 10MB' to 'larger 20MB'. Did I? Well happy to take credit ;) > > In the mremap_move_1mb_from_start() test, I request for an alignment of > 1.25MB so that when I align down, I fall no mapping. This is to catch a bug > that Linus found which is that just because an aligned down address did not > fall on a mapping, that doesn't mean we can just move it at PMD-level > otherwise we destroy the mapping. Yeah that case it all makes sense in, just wondered if the comment belonged there but just me not reading your comment carefully! > > I do need to update the test name in mremap_move_1mb_from_start() to: "mremap > move 1mb from start at 1MB+256KB aligned src". So thanks for point this! > > Would that sort it out or is there still something in the comment I am > missing? Just the suggestion above re: clarification, however it's not a show stopper more of a small nit. > > Thanks! > > - Joel Cheers! > > > > > > > > > > [snip]
On Mon, Aug 28, 2023 at 08:38:17PM +0100, Lorenzo Stoakes wrote: > On Mon, Aug 28, 2023 at 06:59:12PM +0000, Joel Fernandes wrote: > > On Sun, Aug 27, 2023 at 11:15:20AM +0100, Lorenzo Stoakes wrote: > > > On Sun, Aug 27, 2023 at 10:57:59AM +0100, Lorenzo Stoakes wrote: > > > [snip] > > > > > > > > +/* > > > > > + * Verify that an mremap within a range does not cause corruption > > > > > + * of unrelated part of range. > > > > > + * > > > > > + * Consider the following range which is 2MB aligned and is > > > > > + * a part of a larger 10MB range which is not shown. Each > > > > > + * character is 256KB below making the source and destination > > > > > > Just noticed, I think you misspeak here, as this test doens't seem to > > > offset by 256 KiB? That is the strategy for mremap_move_1mb_from_start() > > > rather than this test so perhaps comment needs to be moved around? > > > > > > * 2MB each. The lower case letters are moved (s to d) and the > > > * upper case letters are not moved. The below test verifies > > > * that the upper case S letters are not corrupted by the > > > * adjacent mremap. > > > * > > > * |DDDDddddSSSSssss| > > > */ > > > static void mremap_move_within_range(char pattern_seed) > > > > Here we are moving 1MB within a 4MB zone of a large mapping. Each character > > 's' or 'd' is 256KB. The 256KB there is just for illustration and not really > > significant as such. The 'ssss' is moved to 'dddd' 1MB each. Here we make > > Ahhh I see. I find that a little confusing here, perhaps clearer to say 'each > block of letters is 1 MiB in size' or something? Sure, I'll do that. > Cheers! Cheers. And thanks for the reviews! - Joel
diff --git a/tools/testing/selftests/mm/mremap_test.c b/tools/testing/selftests/mm/mremap_test.c index d7366074e2a8..f45d1abedc9c 100644 --- a/tools/testing/selftests/mm/mremap_test.c +++ b/tools/testing/selftests/mm/mremap_test.c @@ -23,6 +23,7 @@ #define VALIDATION_NO_THRESHOLD 0 /* Verify the entire region */ #define MIN(X, Y) ((X) < (Y) ? (X) : (Y)) +#define SIZE_MB(m) ((size_t)m * (1024 * 1024)) struct config { unsigned long long src_alignment; @@ -226,6 +227,79 @@ static void mremap_expand_merge_offset(FILE *maps_fp, unsigned long page_size) ksft_test_result_fail("%s\n", test_name); } +/* + * Verify that an mremap within a range does not cause corruption + * of unrelated part of range. + * + * Consider the following range which is 2MB aligned and is + * a part of a larger 10MB range which is not shown. Each + * character is 256KB below making the source and destination + * 2MB each. The lower case letters are moved (s to d) and the + * upper case letters are not moved. The below test verifies + * that the upper case S letters are not corrupted by the + * adjacent mremap. + * + * |DDDDddddSSSSssss| + */ +static void mremap_move_within_range(char pattern_seed) +{ + char *test_name = "mremap mremap move within range"; + void *src, *dest; + int i, success = 1; + + size_t size = SIZE_MB(20); + void *ptr = mmap(NULL, size, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + if (ptr == MAP_FAILED) { + perror("mmap"); + success = 0; + goto out; + } + memset(ptr, 0, size); + + src = ptr + SIZE_MB(6); + src = (void *)((unsigned long)src & ~(SIZE_MB(2) - 1)); + + /* Set byte pattern for source block. */ + srand(pattern_seed); + for (i = 0; i < SIZE_MB(2); i++) { + ((char *)src)[i] = (char) rand(); + } + + dest = src - SIZE_MB(2); + + void *new_ptr = mremap(src + SIZE_MB(1), SIZE_MB(1), SIZE_MB(1), + MREMAP_MAYMOVE | MREMAP_FIXED, dest + SIZE_MB(1)); + if (new_ptr == MAP_FAILED) { + perror("mremap"); + success = 0; + goto out; + } + + /* Verify byte pattern after remapping */ + srand(pattern_seed); + for (i = 0; i < SIZE_MB(1); i++) { + char c = (char) rand(); + + if (((char *)src)[i] != c) { + ksft_print_msg("Data at src at %d got corrupted due to unrelated mremap\n", + i); + ksft_print_msg("Expected: %#x\t Got: %#x\n", c & 0xff, + ((char *) src)[i] & 0xff); + success = 0; + } + } + +out: + if (munmap(ptr, size) == -1) + perror("munmap"); + + if (success) + ksft_test_result_pass("%s\n", test_name); + else + ksft_test_result_fail("%s\n", test_name); +} + /* * Returns the start address of the mapping on success, else returns * NULL on failure. @@ -491,6 +565,7 @@ int main(int argc, char **argv) unsigned int threshold_mb = VALIDATION_DEFAULT_THRESHOLD; unsigned int pattern_seed; int num_expand_tests = 2; + int num_misc_tests = 1; struct test test_cases[MAX_TEST] = {}; struct test perf_test_cases[MAX_PERF_TEST]; int page_size; @@ -572,7 +647,7 @@ int main(int argc, char **argv) (threshold_mb * _1MB >= _1GB); ksft_set_plan(ARRAY_SIZE(test_cases) + (run_perf_tests ? - ARRAY_SIZE(perf_test_cases) : 0) + num_expand_tests); + ARRAY_SIZE(perf_test_cases) : 0) + num_expand_tests + num_misc_tests); for (i = 0; i < ARRAY_SIZE(test_cases); i++) run_mremap_test_case(test_cases[i], &failures, threshold_mb, @@ -590,6 +665,8 @@ int main(int argc, char **argv) fclose(maps_fp); + mremap_move_within_range(pattern_seed); + if (run_perf_tests) { ksft_print_msg("\n%s\n", "mremap HAVE_MOVE_PMD/PUD optimization time comparison for 1GB region:");
Move a block of memory within a memory range. Any alignment optimization on the source address may cause corruption. Verify using kselftest that it works. I have also verified with tracing that such optimization does not happen due to this check in can_align_down(): if (!for_stack && vma->vm_start <= addr_masked) return false; Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- tools/testing/selftests/mm/mremap_test.c | 79 +++++++++++++++++++++++- 1 file changed, 78 insertions(+), 1 deletion(-)