On Thu, Dec 04, 2025 at 11:10:46AM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <[email protected]> > > Some of the recent changes to the kunit framework caused the stack usage > for kunit_run_tests() to grow higher than most other kernel functions, > which triggers a warning when CONFIG_FRAME_WARN is set to a relatively > low value: > > lib/kunit/test.c: In function 'kunit_run_tests': > lib/kunit/test.c:801:1: error: the frame size of 1312 bytes is larger than > 1280 bytes [-Werror=frame-larger-than=] > > Split out the inner loop into a separate function to ensure that each > function remains under the limit, and pass the kunit_result_stats > structures by reference to avoid excessive copies. > > Signed-off-by: Arnd Bergmann <[email protected]> > --- > Sending it now as I ran into the build failure while testing 6.18. > I only build-tested this, so please test it properly before applying.
I just ran this through our CI with no problems. Tested-by: Carlos Llamas <[email protected]> > --- > lib/kunit/test.c | 221 ++++++++++++++++++++++++----------------------- > 1 file changed, 115 insertions(+), 106 deletions(-) > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index 62eb529824c6..c5fce199d880 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -94,7 +94,7 @@ struct kunit_result_stats { > unsigned long total; > }; > > -static bool kunit_should_print_stats(struct kunit_result_stats stats) > +static bool kunit_should_print_stats(struct kunit_result_stats *stats) > { > if (kunit_stats_enabled == 0) > return false; > @@ -102,11 +102,11 @@ static bool kunit_should_print_stats(struct > kunit_result_stats stats) > if (kunit_stats_enabled == 2) > return true; > > - return (stats.total > 1); > + return (stats->total > 1); > } > > static void kunit_print_test_stats(struct kunit *test, > - struct kunit_result_stats stats) > + struct kunit_result_stats *stats) > { > if (!kunit_should_print_stats(stats)) > return; > @@ -115,10 +115,10 @@ static void kunit_print_test_stats(struct kunit *test, > KUNIT_SUBTEST_INDENT > "# %s: pass:%lu fail:%lu skip:%lu total:%lu", > test->name, > - stats.passed, > - stats.failed, > - stats.skipped, > - stats.total); > + stats->passed, > + stats->failed, > + stats->skipped, > + stats->total); > } > > /* Append formatted message to log. */ > @@ -600,26 +600,26 @@ static void kunit_run_case_catch_errors(struct > kunit_suite *suite, > } > > static void kunit_print_suite_stats(struct kunit_suite *suite, > - struct kunit_result_stats suite_stats, > - struct kunit_result_stats param_stats) > + struct kunit_result_stats *suite_stats, > + struct kunit_result_stats *param_stats) > { > if (kunit_should_print_stats(suite_stats)) { > kunit_log(KERN_INFO, suite, > "# %s: pass:%lu fail:%lu skip:%lu total:%lu", > suite->name, > - suite_stats.passed, > - suite_stats.failed, > - suite_stats.skipped, > - suite_stats.total); > + suite_stats->passed, > + suite_stats->failed, > + suite_stats->skipped, > + suite_stats->total); > } > > if (kunit_should_print_stats(param_stats)) { > kunit_log(KERN_INFO, suite, > "# Totals: pass:%lu fail:%lu skip:%lu total:%lu", > - param_stats.passed, > - param_stats.failed, > - param_stats.skipped, > - param_stats.total); > + param_stats->passed, > + param_stats->failed, > + param_stats->skipped, > + param_stats->total); > } > } > > @@ -681,13 +681,106 @@ static void kunit_init_parent_param_test(struct > kunit_case *test_case, struct ku > } > } > > -int kunit_run_tests(struct kunit_suite *suite) > +static int kunit_run_one_test(struct kunit_suite *suite, struct kunit_case > *test_case, > + struct kunit_result_stats *suite_stats, > + struct kunit_result_stats *total_stats) > { > + struct kunit test = { .param_value = NULL, .param_index = 0 }; > + struct kunit_result_stats param_stats = { 0 }; > char param_desc[KUNIT_PARAM_DESC_SIZE]; > + const void *curr_param; > + > + kunit_init_test(&test, test_case->name, test_case->log); > + if (test_case->status == KUNIT_SKIPPED) { > + /* Test marked as skip */ > + test.status = KUNIT_SKIPPED; > + kunit_update_stats(¶m_stats, test.status); > + } else if (!test_case->generate_params) { > + /* Non-parameterised test. */ > + test_case->status = KUNIT_SKIPPED; > + kunit_run_case_catch_errors(suite, test_case, &test); > + kunit_update_stats(¶m_stats, test.status); > + } else { > + kunit_init_parent_param_test(test_case, &test); > + if (test_case->status == KUNIT_FAILURE) { > + kunit_update_stats(¶m_stats, test.status); > + goto test_case_end; > + } > + /* Get initial param. */ > + param_desc[0] = '\0'; > + /* TODO: Make generate_params try-catch */ > + curr_param = test_case->generate_params(&test, NULL, > param_desc); > + test_case->status = KUNIT_SKIPPED; > + kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT > KUNIT_SUBTEST_INDENT > + "KTAP version 1\n"); > + kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT > KUNIT_SUBTEST_INDENT > + "# Subtest: %s", test_case->name); > + if (test.params_array.params && > + test_case->generate_params == kunit_array_gen_params) { > + kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT > + KUNIT_SUBTEST_INDENT "1..%zd\n", > + test.params_array.num_params); > + } > + > + while (curr_param) { > + struct kunit param_test = { > + .param_value = curr_param, > + .param_index = ++test.param_index, > + .parent = &test, > + }; > + kunit_init_test(¶m_test, test_case->name, NULL); > + param_test.log = test_case->log; > + kunit_run_case_catch_errors(suite, test_case, > ¶m_test); > + > + if (param_desc[0] == '\0') { > + snprintf(param_desc, sizeof(param_desc), > + "param-%d", param_test.param_index); > + } > + > + kunit_print_ok_not_ok(¶m_test, > KUNIT_LEVEL_CASE_PARAM, > + param_test.status, > + param_test.param_index, > + param_desc, > + param_test.status_comment); > + > + kunit_update_stats(¶m_stats, param_test.status); > + > + /* Get next param. */ > + param_desc[0] = '\0'; > + curr_param = test_case->generate_params(&test, > curr_param, > + param_desc); > + } > + /* > + * TODO: Put into a try catch. Since we don't need suite->exit > + * for it we can't reuse kunit_try_run_cleanup for this yet. > + */ > + if (test_case->param_exit) > + test_case->param_exit(&test); > + /* TODO: Put this kunit_cleanup into a try-catch. */ > + kunit_cleanup(&test); > + } > +test_case_end: > + kunit_print_attr((void *)test_case, true, KUNIT_LEVEL_CASE); > + > + kunit_print_test_stats(&test, ¶m_stats); > + > + kunit_print_ok_not_ok(&test, KUNIT_LEVEL_CASE, test_case->status, > + kunit_test_case_num(suite, test_case), > + test_case->name, > + test.status_comment); > + > + kunit_update_stats(suite_stats, test_case->status); > + kunit_accumulate_stats(total_stats, param_stats); > + > + return 0; nit: the return value is always zero and it's not being used either, so maybe switch to void? > +} > + > + > +int kunit_run_tests(struct kunit_suite *suite) > +{ > struct kunit_case *test_case; > struct kunit_result_stats suite_stats = { 0 }; > struct kunit_result_stats total_stats = { 0 }; > - const void *curr_param; > > /* Taint the kernel so we know we've run tests. */ > add_taint(TAINT_TEST, LOCKDEP_STILL_OK); > @@ -703,97 +796,13 @@ int kunit_run_tests(struct kunit_suite *suite) > > kunit_print_suite_start(suite); > > - kunit_suite_for_each_test_case(suite, test_case) { > - struct kunit test = { .param_value = NULL, .param_index = 0 }; > - struct kunit_result_stats param_stats = { 0 }; > - > - kunit_init_test(&test, test_case->name, test_case->log); > - if (test_case->status == KUNIT_SKIPPED) { > - /* Test marked as skip */ > - test.status = KUNIT_SKIPPED; > - kunit_update_stats(¶m_stats, test.status); > - } else if (!test_case->generate_params) { > - /* Non-parameterised test. */ > - test_case->status = KUNIT_SKIPPED; > - kunit_run_case_catch_errors(suite, test_case, &test); > - kunit_update_stats(¶m_stats, test.status); > - } else { > - kunit_init_parent_param_test(test_case, &test); > - if (test_case->status == KUNIT_FAILURE) { > - kunit_update_stats(¶m_stats, test.status); > - goto test_case_end; > - } > - /* Get initial param. */ > - param_desc[0] = '\0'; > - /* TODO: Make generate_params try-catch */ > - curr_param = test_case->generate_params(&test, NULL, > param_desc); > - test_case->status = KUNIT_SKIPPED; > - kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT > KUNIT_SUBTEST_INDENT > - "KTAP version 1\n"); > - kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT > KUNIT_SUBTEST_INDENT > - "# Subtest: %s", test_case->name); > - if (test.params_array.params && > - test_case->generate_params == > kunit_array_gen_params) { > - kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT > - KUNIT_SUBTEST_INDENT "1..%zd\n", > - test.params_array.num_params); > - } > - > - while (curr_param) { > - struct kunit param_test = { > - .param_value = curr_param, > - .param_index = ++test.param_index, > - .parent = &test, > - }; > - kunit_init_test(¶m_test, test_case->name, > NULL); > - param_test.log = test_case->log; > - kunit_run_case_catch_errors(suite, test_case, > ¶m_test); > - > - if (param_desc[0] == '\0') { > - snprintf(param_desc, sizeof(param_desc), > - "param-%d", > param_test.param_index); > - } > - > - kunit_print_ok_not_ok(¶m_test, > KUNIT_LEVEL_CASE_PARAM, > - param_test.status, > - param_test.param_index, > - param_desc, > - > param_test.status_comment); > - > - kunit_update_stats(¶m_stats, > param_test.status); > - > - /* Get next param. */ > - param_desc[0] = '\0'; > - curr_param = test_case->generate_params(&test, > curr_param, > - > param_desc); > - } > - /* > - * TODO: Put into a try catch. Since we don't need > suite->exit > - * for it we can't reuse kunit_try_run_cleanup for this > yet. > - */ > - if (test_case->param_exit) > - test_case->param_exit(&test); > - /* TODO: Put this kunit_cleanup into a try-catch. */ > - kunit_cleanup(&test); > - } > -test_case_end: > - kunit_print_attr((void *)test_case, true, KUNIT_LEVEL_CASE); > - > - kunit_print_test_stats(&test, param_stats); > - > - kunit_print_ok_not_ok(&test, KUNIT_LEVEL_CASE, > test_case->status, > - kunit_test_case_num(suite, test_case), > - test_case->name, > - test.status_comment); > - > - kunit_update_stats(&suite_stats, test_case->status); > - kunit_accumulate_stats(&total_stats, param_stats); > - } > + kunit_suite_for_each_test_case(suite, test_case) > + kunit_run_one_test(suite, test_case, &suite_stats, > &total_stats); > > if (suite->suite_exit) > suite->suite_exit(suite); > > - kunit_print_suite_stats(suite, suite_stats, total_stats); > + kunit_print_suite_stats(suite, &suite_stats, &total_stats); > suite_end: > kunit_print_suite_end(suite); > > -- I don't see any logical changes, everything seems to have been extracted cleanly into kunit_run_one_test(). So this LGTM, just a minor nit. -- Carlos LLamas
