Messages in this thread Patch in this message | | | Date | Mon, 2 Jan 2023 14:32:55 +0100 | From | David Hildenbrand <> | Subject | Re: [PATCH] selftest/vm: add mremap expand merge offset test |
| |
On 16.12.22 22:44, Lorenzo Stoakes wrote: > Add a test to assert that we can mremap() and expand a mapping starting > from an offset within an existing mapping. We unmap the last page in a 3 > page mapping to ensure that the remap should always succeed, before > remapping from the 2nd page. > > This is additionally a regression test for the issue solved in "mm, mremap: > fix mremap() expanding vma with addr inside vma" and confirmed to fail > prior to the change and pass after it. > > Finally, this patch updates the existing mremap expand merge test to check > error conditions and reduce code duplication between the two tests. > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> > --- > tools/testing/selftests/vm/mremap_test.c | 111 +++++++++++++++++++---- > 1 file changed, 93 insertions(+), 18 deletions(-) > > diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c > index 9496346973d4..28a17d4e8afd 100644 > --- a/tools/testing/selftests/vm/mremap_test.c > +++ b/tools/testing/selftests/vm/mremap_test.c > @@ -119,30 +119,19 @@ static unsigned long long get_mmap_min_addr(void) > } > > /* > - * This test validates that merge is called when expanding a mapping. > - * Mapping containing three pages is created, middle page is unmapped > - * and then the mapping containing the first page is expanded so that > - * it fills the created hole. The two parts should merge creating > - * single mapping with three pages. > + * Using /proc/self/maps, assert that the specified address range is contained > + * within a single mapping. > */ > -static void mremap_expand_merge(unsigned long page_size) > +static bool is_range_mapped(void *start, void *end) > { > - char *test_name = "mremap expand merge"; > FILE *fp; > char *line = NULL; > size_t len = 0; > bool success = false; > - char *start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE, > - MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > - > - munmap(start + page_size, page_size); > - mremap(start, page_size, 2 * page_size, 0); > > fp = fopen("/proc/self/maps", "r"); > - if (fp == NULL) { > - ksft_test_result_fail("%s\n", test_name); > - return; > - } > + if (fp == NULL) > + return false;
This is unexpected. It would be valuable to ksft_print_msg("[INFO] .." something, indicating that we don't know because we cannot access that info.
ksft_print_msg("[INFO] Opening /proc/self/maps failed"
But I'd even suggest opening "/proc/self/maps" once in main() and failing directly there. Then we don't have to worry about it here.
> > while (getline(&line, &len, fp) != -1) { > char *first = strtok(line, "- "); > @@ -150,16 +139,101 @@ static void mremap_expand_merge(unsigned long page_size) > char *second = strtok(NULL, "- "); > void *second_val = (void *) strtol(second, NULL, 16); > > - if (first_val == start && second_val == start + 3 * page_size) { > + if (first_val <= start && second_val >= end) { > success = true; > break; > } > } > + > + fclose(fp); > + return success; > +} > + > +/* > + * This test validates that merge is called when expanding a mapping. > + * Mapping containing three pages is created, middle page is unmapped > + * and then the mapping containing the first page is expanded so that > + * it fills the created hole. The two parts should merge creating > + * single mapping with three pages. > + */ > +static void mremap_expand_merge(unsigned long page_size) > +{ > + char *test_name = "mremap expand merge"; > + bool success = false; > + int errsv = 0; > + char *remap; > + char *start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
I'd suggest
char *remap, *start;
start = mmap() if (start == MAP_FAILED) { ...
to make this easier to read.
> + > + if (start == MAP_FAILED) { > + errsv = errno; > + goto error; > + } > + > + munmap(start + page_size, page_size); > + remap = mremap(start, page_size, 2 * page_size, 0); > + if (remap == MAP_FAILED) { > + errsv = errno; > + munmap(start, page_size); > + munmap(start + 2 * page_size, page_size); > + goto error; > + } > + > + success = is_range_mapped(start, start + 3 * page_size); > + > + munmap(start, 3 * page_size); > + goto out; > + > +error: > + ksft_print_msg("Unexpected mapping/remapping error: %s\n", > + strerror(errsv));
Please avoid the "error" label and just print proper errors directly at the two callsites. Then, remove the "goto out".
> +out: > + if (success) > + ksft_test_result_pass("%s\n", test_name); > + else > + ksft_test_result_fail("%s\n", test_name); > +} > + > +/* > + * Similar to mremap_expand_merge() except instead of removing the middle page, > + * we remove the last then attempt to remap offset from the second page. This > + * should result in the mapping being restored to its former state. > + */ > +static void mremap_expand_merge_offset(unsigned long page_size) > +{ > + > + char *test_name = "mremap expand merge offset"; > + bool success = false; > + int errsv = 0; > + char *remap; > + char *start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
Dito.
> + > + if (start == MAP_FAILED) { > + errsv = errno; > + goto error; > + } > + > + /* Unmap final page to ensure we have space to expand. */ > + munmap(start + 2 * page_size, page_size); > + remap = mremap(start + page_size, page_size, 2 * page_size, 0); > + if (remap == MAP_FAILED) { > + errsv = errno; > + munmap(start, 2 * page_size); > + goto error; > + } > + > + success = is_range_mapped(start, start + 3 * page_size); > + goto out; > + > +error: > + ksft_print_msg("Unexpected mapping/remapping error: %s\n", > + strerror(errsv));
Dito.
> +out:
-- Thanks,
David / dhildenb
| |