On Mon, Jan 02, 2023 at 04:34:14PM +0100, David Hildenbrand wrote:
> On 02.01.23 15: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@xxxxxxxxx>
> > ---
> > tools/testing/selftests/vm/mremap_test.c | 115 ++++++++++++++++++-----
> > 1 file changed, 93 insertions(+), 22 deletions(-)
> >
> > diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
>
>
> ...
>
> > +
> > + start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> > + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > +
> > + if (start == MAP_FAILED) {
> > + ksft_print_msg("mmap failed: %s\n", strerror(errno));
>
> I'd
>
> ksft_test_result_fail(...)
> return;
>
> > + goto out;
> > + }
> > +
> > + munmap(start + page_size, page_size);
> > + remap = mremap(start, page_size, 2 * page_size, 0);
> > + if (remap == MAP_FAILED) {
> > + ksft_print_msg("mremap failed: %s\n", strerror(errno));
> > + munmap(start, page_size);
> > + munmap(start + 2 * page_size, page_size);
> > + goto out;
>
> dito
>
> ksft_test_result_fail(...)
> ...
> return;
>
> > + }
> > +
> > + success = is_range_mapped(maps_fp, start, start + 3 * page_size);
> > + munmap(start, 3 * page_size);
> > +
> > +out:
>
> then you can drop the out label.
>
I have to disagree on this, to be consistent with the other tests the
failure messages should include the test name, and putting the
ksft_test_result_fail("test name") in each branch as well as the error
message would just be wilful duplication.
I do think it's a pity C lacks mechanisms such that gotos are sometimes
necessary, but I can only right so many wrongs in this patch :)
> > + 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(FILE *maps_fp, unsigned long page_size)
> > +{
> > +
> > + char *test_name = "mremap expand merge offset";
> > + bool success = false;
> > + char *remap, *start;
> > +
> > + start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> > + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > +
> > + if (start == MAP_FAILED) {
> > + ksft_print_msg("mmap failed: %s\n", strerror(errno));
> > + goto out;
> > + }
> > +
> > + /* 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) {
> > + ksft_print_msg("mremap failed: %s\n", strerror(errno));
> > + munmap(start, 2 * page_size);
> > + goto out;
> > + }
> > +
> > + success = is_range_mapped(maps_fp, start, start + 3 * page_size);
> > + munmap(start, 3 * page_size);
> > +
> > +out:
>
> dito.
>
> > if (success)
> > ksft_test_result_pass("%s\n", test_name);
> > else
> > ksft_test_result_fail("%s\n", test_name);
> > - fclose(fp);
> > }
> > /*
> > @@ -385,6 +447,7 @@ int main(int argc, char **argv)
> > struct test perf_test_cases[MAX_PERF_TEST];
> > int page_size;
> > time_t t;
> > + FILE *maps_fp;
>
> I'd simply use a global variable, same applies for page_size. But passing it
> around is also ok.
>
I am trying to keep things consistent with what's gone before in this code,
and given page_size is being passed around I think the 'when in Rome'
principle applies equally to passing the fp around I think.
> > pattern_seed = (unsigned int) time(&t);
> > @@ -458,7 +521,15 @@ int main(int argc, char **argv)
> > run_mremap_test_case(test_cases[i], &failures, threshold_mb,
> > pattern_seed);
> > - mremap_expand_merge(page_size);
> > + maps_fp = fopen("/proc/self/maps", "r");
> > + if (maps_fp == NULL) {
> > + ksft_print_msg("Failed to read /proc/self/maps: %s\n", strerror(errno));
>
> Maybe simply fail the test completely and return -errno ?
>
Ack on this one, will spin a v3 with this as otherwise we might miss test
failures here.
> > + } else {
> > + mremap_expand_merge(maps_fp, page_size);
> > + mremap_expand_merge_offset(maps_fp, page_size);
> > +
> > + fclose(maps_fp);
>
> No need to fclose, just keep it open ...
>
I'd rather we did fclose() to keep things clean, as who knows what
additional tests may be added later and to be pedantic about matching an
fopen() with an fclose().
> > + }
> > if (run_perf_tests) {
> > ksft_print_msg("\n%s\n",
>
>
> Acked-by: David Hildenbrand <david@xxxxxxxxxx>
>
Thanks!
> --
> Thanks,
>
> David / dhildenb
>
