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

[David Hildenbrand (Arm)]

On 4/15/26 10:35, Dev Jain wrote:
> 
> 
> 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@xxxxxxx>
>>> Tested-by: Sarthak Sharma <sarthak.sharma@xxxxxxx>
>>> Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
>>> ---
>>
>> 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.

Ah, thanks for clarifying, I got lost in the changes.

So to your patch here, definitely

Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

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

Exactly. And we should strive for tests in selftests/mm that just work
(tm). Any other useful (benchmarking) tools can go to tools/mm.

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


Thanks a bunch!


-- 
Cheers,

David