lkml.org 
[lkml]   [2023]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2] selftest/vm: add mremap expand merge offset test
From
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

\
 
 \ /
  Last update: 2023-03-26 23:24    [W:0.055 / U:0.696 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site