Hi David!
On 9/29/26 1:28 PM, David Hildenbrand (Arm) wrote:
> On 9/24/26 07:00, Sarthak Sharma wrote:
>> mremap_test currently uses a lot of fprintf() and perror()
>> calls. It also uses a variable "failures" to track the number
>> of failed table driven tests.
>>
>> Use ksft_print_msg() and ksft_perror() for diagnostics.
>> Remove the variable "failures" and let kselftest counters
>> handle the final exit status. Use ksft_finished() at
>> the end instead of manually checking if failures > 0. Replace
>>
>> if (success)
>> ksft_test_result_pass(...);
>> else
>> ksft_test_result_fail(...);
>>
>> calls with ksft_test_result(success, ...);
>>
>> Also correct the duplicated "mremap" in "mremap move within
>> range" and the spelling of "dontunmap".
>>
>> Signed-off-by: Sarthak Sharma <[email protected]>
>> ---
>
> [...]
>
>> #endif /* __NR_userfaultfd */
>>
>> @@ -1124,7 +1096,7 @@ static void mremap_move_1mb_from_start(unsigned int
>> pattern_seed,
>> 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");
>> + ksft_perror("mremap");
>> success = 0;
>> goto out;
>> }
>> @@ -1145,59 +1117,49 @@ static void mremap_move_1mb_from_start(unsigned int
>> pattern_seed,
>>
>> out:
>> if (src && munmap(src, c.region_size) == -1)
>
> While at it ... why the comparison with -1. And why do we worry about munmap()
> failing at all? We generally ignore these errors on the exit path, as it's
> unlikely we would ever hit them, and there isn't a lot we can do.
>
> So maybe just remove printing errors entirely?
>
> if (src)
> munmap(src, c.region_size)
Yup, this check is not supposed to be on the exit path. I'll fix this.
>
> ...
>
>> - perror("munmap src");
>> + ksft_perror("munmap src");
>>
>> if (dest && munmap(dest, c.region_size) == -1)
>> - perror("munmap dest");
>> + ksft_perror("munmap dest");
>>
>> - if (success)
>> - ksft_test_result_pass("%s\n", test_name);
>> - else
>> - ksft_test_result_fail("%s\n", test_name);
>> + ksft_test_result(success, "%s\n", test_name);
>> }
>
> [...]
>
>> static void usage(const char *cmd)
>> {
>> - fprintf(stderr,
>> - "Usage: %s [[-t <threshold_mb>] [-p <pattern_seed>]]\n"
>> - "-t\t only validate threshold_mb of the remapped region\n"
>> - " \t if 0 is supplied no threshold is used; all tests\n"
>> - " \t are run and remapped regions validated fully.\n"
>> - " \t The default threshold used is 4MB.\n"
>> - "-p\t provide a seed to generate the random pattern for\n"
>> - " \t validating the remapped region.\n", cmd);
>> + ksft_print_msg("Usage: %s [[-t <threshold_mb>] [-p <pattern_seed>]]\n",
>> cmd);
>> + ksft_print_msg("-t\t only validate threshold_mb of the remapped
>> region\n");
>> + ksft_print_msg(" \t if 0 is supplied no threshold is used; all
>> tests\n");
>> + ksft_print_msg(" \t are run and remapped regions validated fully.\n");
>> + ksft_print_msg(" \t The default threshold used is 4MB.\n");
>> + ksft_print_msg("-p\t provide a seed to generate the random pattern
>> for\n");
>> + ksft_print_msg(" \t validating the remapped region.\n");
>> }
>
> That looks odd, as we will now print this as "# ". I would have assumed that
> removing all parameters as the first patch would make things cleaner?
>
> So as a first patch I think we should just remove the parameters entirely.
> They
> are unused by our infrastrcture:
>
> run_vmtests.sh:CATEGORY="mremap" run_test ./mremap_test
>
> Does anything speak against that?
Okay, I'd kept the 7th and 8th patch of the series for that. I'll make
them the first and second ones then, this will remove all this churn.
Or maybe remove all parameters altogether in the first patch itself.
>
>>
>> static int parse_args(int argc, char **argv, unsigned int *threshold_mb,
>> @@ -1232,7 +1194,6 @@ static int parse_args(int argc, char **argv, unsigned
>> int *threshold_mb,
>> #define MAX_PERF_TEST 3
>> int main(int argc, char **argv)
>> {
>> - int failures = 0;
>> unsigned int i;
>> int run_perf_tests;
>> unsigned int threshold_mb = VALIDATION_DEFAULT_THRESHOLD;
>> @@ -1260,7 +1221,7 @@ int main(int argc, char **argv)
>> pattern_seed = (unsigned int) time(&t);
>>
>> if (parse_args(argc, argv, &threshold_mb, &pattern_seed) < 0)
>> - exit(EXIT_FAILURE);
>> + ksft_exit_fail_msg("Invalid arguments\n");
>>
>> ksft_print_msg("Test configs:\n");
>> ksft_print_msg("threshold_mb=%u\n", threshold_mb);
>> @@ -1282,7 +1243,7 @@ int main(int argc, char **argv)
>> rand_addr = (char *)mmap(NULL, rand_size, PROT_READ | PROT_WRITE,
>> MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>> if (rand_addr == MAP_FAILED) {
>> - perror("mmap");
>> + ksft_perror("mmap");
>> ksft_exit_fail_msg("cannot mmap rand_addr\n");
>
> Would be better combined like:
>
> ksft_exit_fail_msg("cannot mmap rand_addr: %s\n", strerror(errno));
>
> ?
Yup, or maybe with a ksft_exit_fail_perror() directly. Will change.
