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 <[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.
Ah, thanks for clarifying, I got lost in the changes.
So to your patch here, definitely
Acked-by: David Hildenbrand (Arm) <[email protected]>
>
>>
>> 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
