On Sat, Jun 14, 2025 at 4:47 AM David Gow <david...@google.com> wrote:
>
> From: Ujwal Jain <ujwalj...@google.com>
>
> Currently, the in-kernel kunit test case timeout is 300 seconds. (There
> is a separate timeout mechanism for the whole test execution in
> kunit.py, but that's unrelated.) However, tests marked 'slow' or 'very
> slow' may timeout, particularly on slower machines.
>
> Implement a multiplier to the test-case timeout, so that slower tests
> have longer to complete:
> - DEFAULT -> 1x default timeout
> - KUNIT_SPEED_SLOW -> 3x default timeout
> - KUNIT_SPEED_VERY_SLOW -> 12x default timeout
Hello!
This change is looking great to me. No major concerns with the code
and the tests are all passing.
Just a few thoughts: I am wondering where the multipliers of 3 and 12
came from? Are there specific tests that need those timeout amounts?
And then given this changes the behavior of tests marked as slow and
very_slow, we should update the documentation. And if possible, we
should also add tests to check this feature.
Otherwise, this looks good to me!
Thanks!
-Rae
Reviewed-by: Rae Moar <rm...@google.com>
>
> A further change is planned to allow user configuration of the
> default/base timeout to allow people with faster or slower machines to
> adjust these to their use-cases.
>
> Signed-off-by: Ujwal Jain <ujwalj...@google.com>
> Co-developed-by: David Gow <david...@google.com>
> Signed-off-by: David Gow <david...@google.com>
> ---
> include/kunit/try-catch.h | 1 +
> lib/kunit/kunit-test.c | 9 +++++---
> lib/kunit/test.c | 46 ++++++++++++++++++++++++++++++++++++--
> lib/kunit/try-catch-impl.h | 4 +++-
> lib/kunit/try-catch.c | 29 ++----------------------
> 5 files changed, 56 insertions(+), 33 deletions(-)
>
> diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
> index 7c966a1adbd3..d4e1a5b98ed6 100644
> --- a/include/kunit/try-catch.h
> +++ b/include/kunit/try-catch.h
> @@ -47,6 +47,7 @@ struct kunit_try_catch {
> int try_result;
> kunit_try_catch_func_t try;
> kunit_try_catch_func_t catch;
> + unsigned long timeout;
> void *context;
> };
>
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index d9c781c859fd..387cdf7782f6 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -43,7 +43,8 @@ static void
> kunit_test_try_catch_successful_try_no_catch(struct kunit *test)
> kunit_try_catch_init(try_catch,
> test,
> kunit_test_successful_try,
> - kunit_test_no_catch);
> + kunit_test_no_catch,
> + 300 * msecs_to_jiffies(MSEC_PER_SEC));
> kunit_try_catch_run(try_catch, test);
>
> KUNIT_EXPECT_TRUE(test, ctx->function_called);
> @@ -75,7 +76,8 @@ static void
> kunit_test_try_catch_unsuccessful_try_does_catch(struct kunit *test)
> kunit_try_catch_init(try_catch,
> test,
> kunit_test_unsuccessful_try,
> - kunit_test_catch);
> + kunit_test_catch,
> + 300 * msecs_to_jiffies(MSEC_PER_SEC));
> kunit_try_catch_run(try_catch, test);
>
> KUNIT_EXPECT_TRUE(test, ctx->function_called);
> @@ -129,7 +131,8 @@ static void kunit_test_fault_null_dereference(struct
> kunit *test)
> kunit_try_catch_init(try_catch,
> test,
> kunit_test_null_dereference,
> - kunit_test_catch);
> + kunit_test_catch,
> + 300 * msecs_to_jiffies(MSEC_PER_SEC));
> kunit_try_catch_run(try_catch, test);
>
> KUNIT_EXPECT_EQ(test, try_catch->try_result, -EINTR);
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 146d1b48a096..002121675605 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -373,6 +373,46 @@ static void kunit_run_case_check_speed(struct kunit
> *test,
> duration.tv_sec, duration.tv_nsec);
> }
>
> +/* Returns timeout multiplier based on speed.
> + * DEFAULT: 1
> + * KUNIT_SPEED_SLOW: 3
> + * KUNIT_SPEED_VERY_SLOW: 12
> + */
> +static int kunit_timeout_mult(enum kunit_speed speed)
> +{
> + switch (speed) {
> + case KUNIT_SPEED_SLOW:
> + return 3;
> + case KUNIT_SPEED_VERY_SLOW:
> + return 12;
> + default:
> + return 1;
> + }
> +}
> +
> +static unsigned long kunit_test_timeout(struct kunit_suite *suite, struct
> kunit_case *test_case)
> +{
> + int mult = 1;
> + /*
> + * TODO: Make the default (base) timeout configurable, so that users
> with
> + * particularly slow or fast machines can successfully run tests,
> while
> + * still taking advantage of the relative speed.
> + */
> + unsigned long default_timeout = 300;
> +
> + /*
> + * The default test timeout is 300 seconds and will be adjusted by
> mult
> + * based on the test speed. The test speed will be overridden by the
> + * innermost test component.
> + */
> + if (suite->attr.speed != KUNIT_SPEED_UNSET)
> + mult = kunit_timeout_mult(suite->attr.speed);
> + if (test_case->attr.speed != KUNIT_SPEED_UNSET)
> + mult = kunit_timeout_mult(test_case->attr.speed);
> + return mult * default_timeout * msecs_to_jiffies(MSEC_PER_SEC);
> +}
> +
> +
> /*
> * Initializes and runs test case. Does not clean up or do post validations.
> */
> @@ -527,7 +567,8 @@ static void kunit_run_case_catch_errors(struct
> kunit_suite *suite,
> kunit_try_catch_init(try_catch,
> test,
> kunit_try_run_case,
> - kunit_catch_run_case);
> + kunit_catch_run_case,
> + kunit_test_timeout(suite, test_case));
> context.test = test;
> context.suite = suite;
> context.test_case = test_case;
> @@ -537,7 +578,8 @@ static void kunit_run_case_catch_errors(struct
> kunit_suite *suite,
> kunit_try_catch_init(try_catch,
> test,
> kunit_try_run_case_cleanup,
> - kunit_catch_run_case_cleanup);
> + kunit_catch_run_case_cleanup,
> + kunit_test_timeout(suite, test_case));
> kunit_try_catch_run(try_catch, &context);
>
> /* Propagate the parameter result to the test case. */
> diff --git a/lib/kunit/try-catch-impl.h b/lib/kunit/try-catch-impl.h
> index 203ba6a5e740..6f401b97cd0b 100644
> --- a/lib/kunit/try-catch-impl.h
> +++ b/lib/kunit/try-catch-impl.h
> @@ -17,11 +17,13 @@ struct kunit;
> static inline void kunit_try_catch_init(struct kunit_try_catch *try_catch,
> struct kunit *test,
> kunit_try_catch_func_t try,
> - kunit_try_catch_func_t catch)
> + kunit_try_catch_func_t catch,
> + unsigned long timeout)
> {
> try_catch->test = test;
> try_catch->try = try;
> try_catch->catch = catch;
> + try_catch->timeout = timeout;
> }
>
> #endif /* _KUNIT_TRY_CATCH_IMPL_H */
> diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> index 6bbe0025b079..d84a879f0a78 100644
> --- a/lib/kunit/try-catch.c
> +++ b/lib/kunit/try-catch.c
> @@ -34,31 +34,6 @@ static int kunit_generic_run_threadfn_adapter(void *data)
> return 0;
> }
>
> -static unsigned long kunit_test_timeout(void)
> -{
> - /*
> - * TODO(brendanhigg...@google.com): We should probably have some type
> of
> - * variable timeout here. The only question is what that timeout value
> - * should be.
> - *
> - * The intention has always been, at some point, to be able to label
> - * tests with some type of size bucket (unit/small,
> integration/medium,
> - * large/system/end-to-end, etc), where each size bucket would get a
> - * default timeout value kind of like what Bazel does:
> - *
> https://docs.bazel.build/versions/master/be/common-definitions.html#test.size
> - * There is still some debate to be had on exactly how we do this.
> (For
> - * one, we probably want to have some sort of test runner level
> - * timeout.)
> - *
> - * For more background on this topic, see:
> - * https://mike-bland.com/2011/11/01/small-medium-large.html
> - *
> - * If tests timeout due to exceeding sysctl_hung_task_timeout_secs,
> - * the task will be killed and an oops generated.
> - */
> - return 300 * msecs_to_jiffies(MSEC_PER_SEC); /* 5 min */
> -}
> -
> void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
> {
> struct kunit *test = try_catch->test;
> @@ -85,8 +60,8 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
> void *context)
> task_done = task_struct->vfork_done;
> wake_up_process(task_struct);
>
> - time_remaining = wait_for_completion_timeout(task_done,
> - kunit_test_timeout());
> + time_remaining = wait_for_completion_timeout(
> + task_done, try_catch->timeout);
> if (time_remaining == 0) {
> try_catch->try_result = -ETIMEDOUT;
> kthread_stop(task_struct);
> --
> 2.50.0.rc1.591.g9c95f17f64-goog
>
