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.
