Re: [PATCH v2] selftests/mm: Simplify byte pattern checking in mremap_test

[Dev Jain <dev.jain@arm.com>]

On 15/04/26 1:18 pm, David Hildenbrand (Arm) wrote:
> On 4/15/26 06:45, Dev Jain wrote:
>> The original version of mremap_test (7df666253f26: "kselftests: vm: add
>> mremap tests") validated remapped contents byte-by-byte and printed a
>> mismatch index in case the bytes streams didn't match. That was rather
>> inefficient, especially also if the test passed.
>>
>> Later, commit 7033c6cc9620 ("selftests/mm: mremap_test: optimize
>> execution time from minutes to seconds using chunkwise memcmp") used
>> memcmp() on bigger chunks, to fallback to byte-wise scanning to detect
>> the problematic index only if it discovered a problem.
>>
>> However, the implementation is overly complicated (e.g., get_sqrt() is
>> currently not optimal) and we don't really have to report the exact
>> index: whoever debugs the failing test can figure that out.
>>
>> Let's simplify by just comparing both byte streams with memcmp() and not
>> detecting the exact failed index.
>>
>> Reported-by: Sarthak Sharma <sarthak.sharma@arm.com>
>> Tested-by: Sarthak Sharma <sarthak.sharma@arm.com>
>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>> ---
> 
> I'll note something interesting: before 7033c6cc9620, we would check
> random bytes in the stream. With 7033c6cc9620 we only check the first
> threshold bytes IIUC.

Before 7033c6cc9620, the block of code was:

	/* Verify byte pattern after remapping */
	srand(pattern_seed);
	for (t = 0; t < threshold; t++) {
		char c = (char) rand();

		if (((char *) dest_addr)[t] != c) {
			ksft_print_msg("Data after remap doesn't match at offset %llu\n",
				       t);
			ksft_print_msg("Expected: %#x\t Got: %#x\n", c & 0xff,
					((char *) dest_addr)[t] & 0xff);
			ret = -1;
			goto clean_up_dest;
		}
	}

which is still checking the first threshold bytes only. Note that
pattern_seed remains constant at runtime, so 7033c6cc9620 just replaces
this with a buffer filled with the rand() stream.

> 
> That means, that we are not actually verifying most of the area at all
> anymore?
> 
> 
> The whole test options are extremely questionable:
> 
> $ ./mremap_test --help
> ./mremap_test: invalid option -- '-'
> Usage: ./mremap_test [[-t <threshold_mb>] [-p <pattern_seed>]]
> -t       only validate threshold_mb of the remapped region
>          if 0 is supplied no threshold is used; all tests
>          are run and remapped regions validated fully.
>          The default threshold used is 4MB.
> -p       provide a seed to generate the random pattern for
>          validating the remapped region.
> 
> Nobody will ever set these parameters, really. And tests that test
> different things each time they are run are not particularly helpful.
> 
> We should just remove all that and do something reasonable internally.
> 
> That is
> 
> a) Remove all the perf crap (ehm sorry, "advanced tests that don't
>    belong here and that nobody ever runs") from this functional test

Hmm... perhaps this is useful, we can keep this by default so we can
detect if a bug comes up in PMD/PUD mremap? If the test takes too long
we know we have messed up something there. Although "test taking too
long" is not a nice way to know that there is a bug ...

and test won't even take long perhaps since memcmp on 1G will be fairly
fast. In case we mess up PMD/PUD mremap real bug reports will come sooner
than anyone detecting this from mremap_test.

So I'll remove this.

> 
> b) Remove all options from the test. Nobody ever uses them. They are
>    stupid.

Agreed.

> 
> c) Remove any randomization from the test. There is no need for random
>    patterns, just fill pages with increasing numbers.

Agreed.

> 
> d) Just always verify the whole regions. Without the rand() magic this
>    will probably be just ... fairly fast?

Yeah we are doing a simple memcmp() so it is fine.

I'll implement these changes.

>