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