Re: [PATCH v6 5/6] tools/mm: make gup_bench a benchmark only tool

> Remove the functional modes (GUP_BASIC_TEST, PIN_BASIC_TEST and
> DUMP_USER_PAGES_TEST) from gup_bench. Drop kselftest dependency
> and use normal diagnostics and exit statuses.
> 
> When no arguments are supplied, run a single GUP_FAST_BENCHMARK
> with existing default values. Let users select other configs
> through command line options. Also validate numeric arguments
> and reject positional arguments.
> 
> Restore hugeTLB settings on failure and after every run. Also
> handle failures without relying on assert() calls.
> 
> Suggested-by: David Hildenbrand (Arm) <[email protected]>
> Signed-off-by: Sarthak Sharma <[email protected]>

...

>  int main(int argc, char **argv)
>  {
>  	struct gup_test gup = { 0 };
> -	int filed, i, opt, nr_pages = 1, thp = -1, write = 1, nthreads = 1, ret;
> +	int filed, i, opt, nr_pages = 1, thp = -1, write = 1;
> +	int nthreads = 1, ret, started_threads = 0;
>  	int flags = MAP_PRIVATE;
> -	char *file = "/dev/zero";
> -	bool hugetlb = false;
> +	const char *file = "/dev/zero";
> +	bool hugetlb = false, restore_hugetlb = false;
> +	unsigned long nr_pages_per_call;
>  	pthread_t *tid;
>  	char *p;
>  
> -	while ((opt = getopt(argc, argv, "m:r:n:F:f:abcj:tTLUuwWSHpz")) != -1) {
> +	while ((opt = getopt(argc, argv, "m:r:n:F:f:aj:tTLuwWSH")) != -1) {
>  		switch (opt) {
>  		case 'a':
>  			cmd = PIN_FAST_BENCHMARK;
>  			break;
> -		case 'b':
> -			cmd = PIN_BASIC_TEST;
> -			break;
>  		case 'L':
>  			cmd = PIN_LONGTERM_BENCHMARK;
>  			break;
> -		case 'c':
> -			cmd = DUMP_USER_PAGES_TEST;
> -			/*
> -			 * Dump page 0 (index 1). May be overridden later, by
> -			 * user's non-option arguments.
> -			 *
> -			 * .which_pages is zero-based, so that zero can mean "do
> -			 * nothing".
> -			 */
> -			gup.which_pages[0] = 1;
> -			break;
> -		case 'p':
> -			/* works only with DUMP_USER_PAGES_TEST */
> -			gup.test_flags |= GUP_TEST_FLAG_DUMP_PAGES_USE_PIN;
> -			break;
> -		case 'F':
> -			/* strtol, so you can pass flags in hex form */
> -			gup.gup_flags = strtol(optarg, 0, 0);
> +		case 'F': {
> +			long val;
> +
> +			val = parse_long_arg_base(optarg, "GUP flags", 0);
> +			if (val < 0 || val > UINT_MAX) {
> +				fprintf(stderr, "Invalid GUP flags '%s'\n", optarg);
> +				exit(1);
> +			}
> +
> +			gup.gup_flags = val;
>  			break;
> -		case 'j':
> -			nthreads = atoi(optarg);
> +		}
> +		case 'j': {
> +			long val;
> +
> +			val = parse_positive_long_arg(optarg, "thread count");
> +			if (val > INT_MAX ||
> +			    (size_t)val > SIZE_MAX / sizeof(pthread_t)) {
> +				fprintf(stderr, "Invalid thread count '%s'\n", optarg);
> +				exit(1);
> +			}
> +			nthreads = val;
>  			break;
> +		}
>  		case 'm':
> -			size = atoi(optarg) * MB;
> +			size = parse_positive_long_arg(optarg, "size");
> +			if (size > ULONG_MAX / MB) {
> +				fprintf(stderr, "Invalid size '%s'\n", optarg);
> +				exit(1);
> +			}
> +			size *= MB;
>  			break;
> -		case 'r':
> -			repeats = atoi(optarg);
> +		case 'r': {
> +			long val;
> +
> +			val = parse_positive_long_arg(optarg, "repeat count");
> +			if (val > INT_MAX) {
> +				fprintf(stderr, "Invalid repeat count '%s'\n", optarg);
> +				exit(1);
> +			}
> +			repeats = val;
>  			break;
> -		case 'n':
> -			nr_pages = atoi(optarg);
> -			if (nr_pages < 0)
> -				nr_pages = size / getpagesize();
> +		}
> +		case 'n': {
> +			long val;
> +
> +			val = parse_long_arg(optarg, "page count");

It's better to name the numbers parsing after what they do:
parse_flags() and parse_num().

> +			if (val != -1 && (val < 1 || val > INT_MAX)) {
 
And the limit checks seem wierd all over the place, like if we can loop
infinitely, why do we care about INT_MAX?

And what exact limit ULONG_MAX / MB or SIZE_MAX / sizeof(ptread_t) are
supposed to express?

> +				fprintf(stderr, "Invalid page count '%s'\n", optarg);
> +				exit(1);
> +			}
> +			nr_pages = val;
>  			break;

...

>  	if (hugetlb) {
>  		unsigned long hp_size = default_huge_page_size();
>  
> -		if (!hp_size)
> -			ksft_exit_skip("HugeTLB is unavailable\n");
> +		if (!hp_size) {
> +			fprintf(stderr, "Could not determine huge page size\n");
> +			return 1;
> +		}
> +
> +		if (size > ULONG_MAX - (hp_size - 1)) {
> +			fprintf(stderr, "HugeTLB mapping size is too large\n");
> +			return 1;
> +		}
>  
>  		size = (size + hp_size - 1) & ~(hp_size - 1);
> -		if (!hugetlb_setup_default(size / hp_size))
> -			ksft_exit_skip("Not enough huge pages\n");
> +		if (!hugetlb_setup_default(size / hp_size)) {
> +			fprintf(stderr, "Not enough huge pages\n");
> +			hugetlb_restore_settings();

you don't need to explicitly call hugetlb_restore_settings(),
_setup_defaults() sets up automatic restore on exit.

> +			return 1;
> +		}
> +		restore_hugetlb = true;
>  	}

...

>  	gup_fd = open(GUP_TEST_FILE, O_RDWR);
>  	if (gup_fd == -1) {
> -		switch (errno) {
> -		case EACCES:
> -			if (getuid())
> -				ksft_print_msg("Please run this test as root\n");
> -			break;
> -		case ENOENT:
> -			if (opendir("/sys/kernel/debug") == NULL)
> -				ksft_print_msg("mount debugfs at /sys/kernel/debug\n");
> -			ksft_print_msg("check if CONFIG_GUP_TEST is enabled in kernel config\n");
> -			break;
> -		default:
> -			ksft_print_msg("failed to open %s: %s\n", GUP_TEST_FILE, strerror(errno));
> -			break;
> -		}
> -		ksft_test_result_skip("Please run this test as root\n");
> -		ksft_exit_pass();
> +		int err = errno;
> +
> +		close(filed);
> +		if (err == EACCES)

What was wrong with switch (errno) ?

> +			fprintf(stderr, "Please run as root\n");

Please add root check upfront and skip EACCES here

> +		else if (err == ENOENT) {
> +			DIR *debugfs = opendir("/sys/kernel/debug");
> +
> +			if (!debugfs)
> +				fprintf(stderr, "Mount debugfs at /sys/kernel/debug\n");

Just replace the prints, no need to refactor the logic there.

> +			else {
> +				closedir(debugfs);
> +				fprintf(stderr, "Check CONFIG_GUP_TEST in kernel config\n");
> +			}
> +		} else
> +			fprintf(stderr, "Failed to open %s: %s\n", GUP_TEST_FILE,
> +				strerror(err));
> +		if (restore_hugetlb)
> +			hugetlb_restore_settings();
> +		return 1;
>  	}
>  
>  	p = mmap(NULL, size, PROT_READ | PROT_WRITE, flags, filed, 0);
> -	if (p == MAP_FAILED)
> -		ksft_exit_fail_msg("mmap: %s\n", strerror(errno));
> +	if (p == MAP_FAILED) {
> +		fprintf(stderr, "mmap: %s\n", strerror(errno));
> +		close(filed);
> +		close(gup_fd);
> +
> +		if (restore_hugetlb)
> +			hugetlb_restore_settings();

Use goto err_do_cleanup here and everywhere else. Piling cleanups in 
if (something_failed) is error prone and unmaintainable.

> +		return 1;
> +	}
> +	close(filed);
>  	gup.addr = (unsigned long)p;
>  
>  	if (thp == 1)
  
...

>  	free(tid);
> +	munmap((void *)gup.addr, size);
> +	close(gup_fd);
> +	if (restore_hugetlb)
> +		hugetlb_restore_settings();
>  
> -	ksft_exit_pass();
> +	return bench_error ? 1 : 0;

Using goto for cleanup gives you clean return 1 on error and return 0 on
success.

-- 
Sincerely yours,
Mike.