Message-ID: <Y7L9JfyQrwIRVo2A@lucifer>
Date:   Mon, 2 Jan 2023 15:49:57 +0000
From:   Lorenzo Stoakes <lstoakes@...il.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org, Vlastimil Babka <vbabka@...e.cz>,
        Jakub Matena <matenajakub@...il.com>,
        Matthew Wilcox <willy@...radead.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Michal Hocko <mhocko@...nel.org>
Subject: Re: [PATCH v2] selftest/vm: add mremap expand merge offset test

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@...il.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 :)

> > +	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@...hat.com>
>

Thanks!

> --
> Thanks,
>
> David / dhildenb
>

Powered by blists - more mailing lists