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)
...
> - 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?
>
> 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));
?
--
Cheers,
David
