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

Hi Mike!

On 8/3/26 2:30 PM, Mike Rapoport wrote:
>> 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().

Ack

> 
>> +			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?

INT_MAX checks are there since nr_pages, nthreads and repeats are stored
as int.

But yes I can keep the parameters which are not there in the ioctl ABI
to be unsigned long, so these checks won't be required there.

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

ULONG_MAX / MB prevents size *= MB from overflowing. SIZE_MAX /
sizeof(ptread_t) prevents thread array allocation size from overflowing.
> 
>> +				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.

Ack

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

Ack

> 
>> +		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.

Okay

> 
>> +			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.

Ack

> 
>> +		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.