Messages in this thread | | | Date | Mon, 2 Jan 2023 16:53:20 +0100 | Subject | Re: [PATCH v2] selftest/vm: add mremap expand merge offset test | From | David Hildenbrand <> |
| |
On 02.01.23 16:49, Lorenzo Stoakes wrote: > 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@gmail.com> >>> --- >>> 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 :) >
Let's agree to disagree ;) Too bad we don't have prefix push/pop functionality as we have in other similar testing frameworks -- to avoid exactly that duplication.
[...]
>> 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.
Other tests we have handle it "easier". :)
-- Thanks,
David / dhildenb
| |