Re: [PATCH v2] selftest/vm: add mremap expand merge offset test

[David Hildenbrand <david@redhat.com>]

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.

> +	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.

>   
>   	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 ?

> +	} 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 ...

> +	}
>   
>   	if (run_perf_tests) {
>   		ksft_print_msg("\n%s\n",


Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb