Message-ID: <18a2c79d-3feb-41aa-93a9-bafbfb188cbc@lucifer.local>
Date:   Sun, 27 Aug 2023 11:12:14 +0100
From:   Lorenzo Stoakes <lstoakes@...il.com>
To:     "Joel Fernandes (Google)" <joel@...lfernandes.org>
Cc:     linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-mm@...ck.org, Shuah Khan <shuah@...nel.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Michal Hocko <mhocko@...e.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Kirill A Shutemov <kirill@...temov.name>,
        "Liam R. Howlett" <liam.howlett@...cle.com>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Kalesh Singh <kaleshsingh@...gle.com>,
        Lokesh Gidra <lokeshgidra@...gle.com>
Subject: Re: [PATCH v5 7/7] selftests: mm: Add a test for moving from an
 offset from start of mapping

On Tue, Aug 22, 2023 at 01:55:00AM +0000, Joel Fernandes (Google) wrote:
> From: Joel Fernandes <joel@...lfernandes.org>
>
> It is possible that the aligned address falls on no existing mapping,
> however that does not mean that we can just align it down to that.
> This test verifies that the "vma->vm_start != addr_to_align" check in
> can_align_down() prevents disastrous results if aligning down when
> source and dest are mutually aligned within a PMD but the source/dest
> addresses requested are not at the beginning of the respective mapping
> containing these addresses.
>
> Signed-off-by: Joel Fernandes (Google) <joel@...lfernandes.org>
> ---
>  tools/testing/selftests/mm/mremap_test.c | 189 ++++++++++++++++-------
>  1 file changed, 134 insertions(+), 55 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/mremap_test.c b/tools/testing/selftests/mm/mremap_test.c
> index f45d1abedc9c..c71ac8306c89 100644
> --- a/tools/testing/selftests/mm/mremap_test.c
> +++ b/tools/testing/selftests/mm/mremap_test.c
> @@ -24,6 +24,7 @@
>
>  #define MIN(X, Y) ((X) < (Y) ? (X) : (Y))
>  #define SIZE_MB(m) ((size_t)m * (1024 * 1024))
> +#define SIZE_KB(k) ((size_t)k * 1024)

Same comment as previous re: wrapping k in parens, it doesn't really matter
here that much but for good practice (and consistency with MIN()) would be
nice to do.

>
>  struct config {
>  	unsigned long long src_alignment;
> @@ -148,6 +149,60 @@ static bool is_range_mapped(FILE *maps_fp, void *start, void *end)
>  	return success;
>  }
>
> +/*
> + * Returns the start address of the mapping on success, else returns
> + * NULL on failure.
> + */
> +static void *get_source_mapping(struct config c)
> +{
> +	unsigned long long addr = 0ULL;
> +	void *src_addr = NULL;
> +	unsigned long long mmap_min_addr;
> +
> +	mmap_min_addr = get_mmap_min_addr();
> +	/*
> +	 * For some tests, we need to not have any mappings below the
> +	 * source mapping. Add some headroom to mmap_min_addr for this.
> +	 */
> +	mmap_min_addr += 10 * _4MB;
> +
> +retry:
> +	addr += c.src_alignment;
> +	if (addr < mmap_min_addr)
> +		goto retry;
> +
> +	src_addr = mmap((void *) addr, c.region_size, PROT_READ | PROT_WRITE,
> +					MAP_FIXED_NOREPLACE | MAP_ANONYMOUS | MAP_SHARED,
> +					-1, 0);
> +	if (src_addr == MAP_FAILED) {
> +		if (errno == EPERM || errno == EEXIST)
> +			goto retry;
> +		goto error;
> +	}
> +	/*
> +	 * Check that the address is aligned to the specified alignment.
> +	 * Addresses which have alignments that are multiples of that
> +	 * specified are not considered valid. For instance, 1GB address is
> +	 * 2MB-aligned, however it will not be considered valid for a
> +	 * requested alignment of 2MB. This is done to reduce coincidental
> +	 * alignment in the tests.
> +	 */
> +	if (((unsigned long long) src_addr & (c.src_alignment - 1)) ||
> +			!((unsigned long long) src_addr & c.src_alignment)) {
> +		munmap(src_addr, c.region_size);
> +		goto retry;
> +	}
> +
> +	if (!src_addr)
> +		goto error;
> +
> +	return src_addr;
> +error:
> +	ksft_print_msg("Failed to map source region: %s\n",
> +			strerror(errno));
> +	return NULL;
> +}
> +

A meta thing, but it'd be nice to separate out _moving_ functions into
their own patch to make reviewing easier as here it's not obvious whether
you changed anything or not (I eyeballed and seems like you didn't though!)

>  /*
>   * This test validates that merge is called when expanding a mapping.
>   * Mapping containing three pages is created, middle page is unmapped
> @@ -300,60 +355,6 @@ static void mremap_move_within_range(char pattern_seed)
>  		ksft_test_result_fail("%s\n", test_name);
>  }
>

> -/*
> - * Returns the start address of the mapping on success, else returns
> - * NULL on failure.
> - */
> -static void *get_source_mapping(struct config c)
> -{
> -	unsigned long long addr = 0ULL;
> -	void *src_addr = NULL;
> -	unsigned long long mmap_min_addr;
> -
> -	mmap_min_addr = get_mmap_min_addr();
> -	/*
> -	 * For some tests, we need to not have any mappings below the
> -	 * source mapping. Add some headroom to mmap_min_addr for this.
> -	 */
> -	mmap_min_addr += 10 * _4MB;
> -
> -retry:
> -	addr += c.src_alignment;
> -	if (addr < mmap_min_addr)
> -		goto retry;
> -
> -	src_addr = mmap((void *) addr, c.region_size, PROT_READ | PROT_WRITE,
> -					MAP_FIXED_NOREPLACE | MAP_ANONYMOUS | MAP_SHARED,
> -					-1, 0);
> -	if (src_addr == MAP_FAILED) {
> -		if (errno == EPERM || errno == EEXIST)
> -			goto retry;
> -		goto error;
> -	}
> -	/*
> -	 * Check that the address is aligned to the specified alignment.
> -	 * Addresses which have alignments that are multiples of that
> -	 * specified are not considered valid. For instance, 1GB address is
> -	 * 2MB-aligned, however it will not be considered valid for a
> -	 * requested alignment of 2MB. This is done to reduce coincidental
> -	 * alignment in the tests.
> -	 */
> -	if (((unsigned long long) src_addr & (c.src_alignment - 1)) ||
> -			!((unsigned long long) src_addr & c.src_alignment)) {
> -		munmap(src_addr, c.region_size);
> -		goto retry;
> -	}
> -
> -	if (!src_addr)
> -		goto error;
> -
> -	return src_addr;
> -error:
> -	ksft_print_msg("Failed to map source region: %s\n",
> -			strerror(errno));
> -	return NULL;
> -}
> -
>  /* Returns the time taken for the remap on success else returns -1. */
>  static long long remap_region(struct config c, unsigned int threshold_mb,
>  			      char pattern_seed)
> @@ -487,6 +488,83 @@ static long long remap_region(struct config c, unsigned int threshold_mb,
>  	return ret;
>  }
>
> +/*
> + * Verify that an mremap aligning down does not destroy
> + * the beginning of the mapping just because the aligned
> + * down address landed on a mapping that maybe does not exist.
> + */
> +static void mremap_move_1mb_from_start(char pattern_seed)
> +{
> +	char *test_name = "mremap move 1mb from start at 2MB+256KB aligned src";
> +	void *src = NULL, *dest = NULL;
> +	int i, success = 1;
> +
> +	/* Config to reuse get_source_mapping() to do an aligned mmap. */
> +	struct config c = {
> +		.src_alignment = SIZE_MB(1) + SIZE_KB(256),
> +		.region_size = SIZE_MB(6)
> +	};
> +
> +	src = get_source_mapping(c);
> +	if (!src) {
> +		success = 0;
> +		goto out;
> +	}
> +
> +	c.src_alignment = SIZE_MB(1) + SIZE_KB(256);

Why are you setting this again?

> +	dest = get_source_mapping(c);
> +	if (!dest) {
> +		success = 0;
> +		goto out;
> +	}
> +
> +	/* Set byte pattern for source block. */
> +	srand(pattern_seed);
> +	for (i = 0; i < SIZE_MB(2); i++) {
> +		((char *)src)[i] = (char) rand();
> +	}
> +
> +	/*
> +	 * Unmap the beginning of dest so that the aligned address
> +	 * falls on no mapping.
> +	 */
> +	munmap(dest, SIZE_MB(1));

This actually aligns (no pun intended) with my comments on the min mmap
address + 4 MiB comments previously. But I guess for the generalised mremap
test you will still need to have that headroom...

> +
> +	void *new_ptr = mremap(src + SIZE_MB(1), SIZE_MB(1), SIZE_MB(1),
> +						   MREMAP_MAYMOVE | MREMAP_FIXED, dest + SIZE_MB(1));
> +	if (new_ptr == MAP_FAILED) {
> +		perror("mremap");
> +		success = 0;
> +		goto out;
> +	}
> +
> +	/* Verify byte pattern after remapping */
> +	srand(pattern_seed);
> +	for (i = 0; i < SIZE_MB(1); i++) {
> +		char c = (char) rand();
> +
> +		if (((char *)src)[i] != c) {
> +			ksft_print_msg("Data at src at %d got corrupted due to unrelated mremap\n",
> +				       i);
> +			ksft_print_msg("Expected: %#x\t Got: %#x\n", c & 0xff,
> +					((char *) src)[i] & 0xff);
> +			success = 0;
> +		}
> +	}
> +
> +out:
> +	if (src && munmap(src, c.region_size) == -1)
> +		perror("munmap src");
> +
> +	if (dest && munmap(dest, c.region_size) == -1)
> +		perror("munmap dest");
> +
> +	if (success)
> +		ksft_test_result_pass("%s\n", test_name);
> +	else
> +		ksft_test_result_fail("%s\n", test_name);
> +}
> +
>  static void run_mremap_test_case(struct test test_case, int *failures,
>  				 unsigned int threshold_mb,
>  				 unsigned int pattern_seed)
> @@ -565,7 +643,7 @@ int main(int argc, char **argv)
>  	unsigned int threshold_mb = VALIDATION_DEFAULT_THRESHOLD;
>  	unsigned int pattern_seed;
>  	int num_expand_tests = 2;
> -	int num_misc_tests = 1;
> +	int num_misc_tests = 2;
>  	struct test test_cases[MAX_TEST] = {};
>  	struct test perf_test_cases[MAX_PERF_TEST];
>  	int page_size;
> @@ -666,6 +744,7 @@ int main(int argc, char **argv)
>  	fclose(maps_fp);
>
>  	mremap_move_within_range(pattern_seed);
> +	mremap_move_1mb_from_start(pattern_seed);
>
>  	if (run_perf_tests) {
>  		ksft_print_msg("\n%s\n",
> --
> 2.42.0.rc1.204.g551eb34607-goog
>

Have you verified this test fails if you eliminate the vma->vm_start !=
addr_to_align check?

Unrelated to this patch, but I guess it might be tricky to come up with a
test for the shift_arg_pages() stack case?

Irrespective of the above, this looks correct to me so,

Reviewed-by: Lorenzo Stoakes <lstoakes@...il.com>

Thanks for putting in so much effort on the testing side too! :)

Powered by blists - more mailing lists