Re: [PATCH RESEND 1/9] selftests/mm: mremap_test: use kselftest helpers

["David Hildenbrand (Arm)" <david@kernel.org>]

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 <sarthak.sharma@arm.com>
> ---

[...]

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