Re: [PATCH v2 2/2] kunit: add KUnit test to assert kernel state before KUnit suites are run

[David Gow <david@davidgow.net>]

Le 24/08/2026 à 21:32, Malte Wechter a écrit :
> add pre-defined KUnit test suite and test case that asserts both
> `debug_locks` and `TAINT_WARN` prior to running any (user) KUnit tests.
> This asserts integrity before tests are run.
> 
> Signed-off-by: Malte Wechter <maltewechter@gmail.com>
> ---

I'm not quite as convinced by this as I am by the first patch. While 
ensuring the state of the system is good before tests are run is useful, 
this does seem a bit heavy-handed in some respects.

This could probably use a more detailed description, particularly 
describing why such a test is useful, and why it would need to be 
implemented in a special way.

And I do think the implementation here is a bit _too_ special-cased. One 
other possibility would be to prepend this suite using 
kunit_merge_suite_sets(), so we don't need to have any special handling 
of (e.g.) the test count. This could also allow this special suite to be 
filtered out (which has both advantages and disadvantages).

It might also be nice to have this configurable independently from the 
other checks, and maybe at runtime (via a KUnit module / command-line 
parameter), particularly if this can't be filtered on. And, as before, 
this definitely needs to be documented. People need to know how to 
enable it, and where all of these extra results from tests they didn't 
enable came from.

Thoughts?

Cheers,
-- David

>   lib/kunit/executor.c |  8 +++++++-
>   lib/kunit/test.c     | 30 ++++++++++++++++++++++++++++++
>   2 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index b0f8a41d61d36..0db67fe7f09f9 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -290,9 +290,15 @@ void kunit_exec_run_tests(struct kunit_suite_set *suite_set, bool builtin)
>   	size_t num_suites = suite_set->end - suite_set->start;
>   	bool autorun = kunit_autorun();
>   
> +	#ifdef CONFIG_KUNIT_EXTRA_ASSERTS

Nit: Let's not indent the #ifdefs.

> +	size_t num_suites_plus_extra = num_suites+1;
> +	#else
> +	size_t num_suites_plus_extra = num_suites;
> +	#endif
> +

I'm not particularly happy with this way of adding an extra suite.

>   	if (autorun && (builtin || num_suites)) {
>   		pr_info("KTAP version 1\n");
> -		pr_info("1..%zu\n", num_suites);
> +		pr_info("1..%zu\n", num_suites_plus_extra);
>   	}
>   
>   	__kunit_test_suites_init(suite_set->start, num_suites, autorun);
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 99773e000e1b7..e64c6d1575280 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -835,6 +835,30 @@ bool kunit_enabled(void)
>   	return enable_param;
>   }
>   
> +#ifdef CONFIG_KUNIT_EXTRA_ASSERTS
> +#define DEBUG_LOCKS_OK 1
> +#define TAINT_WARN_OK 0

Not totally sold on these #defines: I think I'd prefer to just have the 
literal 1/0.

> +
> +static void pre_kunit_assert(struct kunit *test)
> +{
> +	KUNIT_EXPECT_EQ_MSG(test, debug_locks, DEBUG_LOCKS_OK,
> +		"debug_locks are off before any test ran");
> +	KUNIT_EXPECT_EQ_MSG(test, test_taint(TAINT_WARN), TAINT_WARN_OK,
> +		"kernel already TAINT_WARN tainted before any test ran");
> +}

If we are going to generate a special suite, let's have the taint and 
lockdep checks as separate tests.

This would also make it easier to have them be configurable separately.

> +
> +static struct kunit_case pre_kunit_assert_cases[] = {
> +	KUNIT_CASE(pre_kunit_assert),
> +	{}
> +};
> +
> +static struct kunit_suite pre_kunit_assert_clean_state_suite = {
> +	.name = "pre_kunit_extra_asserts",

I think we could probably find a better name for this. 
"initial_system_state" or similar might be better?

> +	.test_cases = pre_kunit_assert_cases,
> +};
> +
> +#endif /* CONFIG_RUST_KUNIT_EXTRA_ASSERTS */
> +
>   int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_suites,
>   			     bool run_tests)
>   {
> @@ -857,6 +881,12 @@ int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_
>   	}
>   	static_branch_inc(&kunit_running);
>   
> +	#ifdef CONFIG_KUNIT_EXTRA_ASSERTS
> +	kunit_init_suite(&pre_kunit_assert_clean_state_suite);
> +	if (run_tests)
> +		kunit_run_tests(&pre_kunit_assert_clean_state_suite);
> +	#endif
> +
>   	for (i = 0; i < num_suites; i++) {
>   		kunit_init_suite(suites[i]);
>   		if (run_tests)
>