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 <[email protected]>
>> Tested-by: Sarthak Sharma <[email protected]>
>> Signed-off-by: Dev Jain <[email protected]>
>> ---
>
> 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.
>
