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

[Malte Wechter]

Hi,

- I was actually considering this as an option, and as you might also
believe, extending KUnit with a test case that is guaranteed to run
before any user implemented a test case, might be too complex of a
task.
- For the taint i could implement the inverse of `test_taint(flag)`
that simply checks that no taints except `flag` is set. Or it could be
tailored specifically for the `TAINT_TEST`.
- And for the case where the kernel becomes tainted somewhere in the
middle and starts skipping unit tests, it is a rare case and would
require more work to be able to distinguish between a KUnit test
triggering a taint/lockdep or somewhere else in the kernel. I believe
this is important to recognize and document, but the change is still
good even with this edge case present.

Best regards,
Malte :)

Den tirs. 6. okt. 2026 kl. 08.47 skrev David Gow <david@xxxxxxxxxxxx>:
>
> Le 01/09/2026 à 19:02, Malte Wechter a écrit :
> > Den tirs. 25. aug. 2026 kl. 13.00 skrev David Gow <david@xxxxxxxxxxxx>:
> >>
> >> 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@xxxxxxxxx>
> >>> ---
> >>
> >> 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?
> > I get your point, the purpose of this special test suite is to assert
> > that the kernel is in a "fine" state before any unit tests are run. If
> > this
> > check is left out the false positives could occur if the system is in
> > a bad state before the tests are run.
> > The reason that the case was handled differently compared to other was
> > because this assertion _must_ be run before any other tests,
> > and when kunit_merge_suite_sets() is called from kunit_run_all_tests()
> > it also filters the test suites, which would not give any guarentee
> > that this
> > special suite gets run first.
> >
> > I do agree that this initial way is maybe a bit coarse, and i will see
> > if i can find a better fit for this assertion. But i dont want to
> > leave it out.
>
> Had a chat with Andreas about this yesterday, and we think we've come up
> with another possible option here: instead of doing one check to verify
> the state before any KUnit tests are run, have an option to check the
> taint before _every_ test, and to skip it if the kernel isn't clean.
>
> There are a few consequences of this:
> - You'd need to be able to mask out taints you don't care about,
> especially TAINT_TEST (as without it all tests but the first would
> fail). Of course, that might still be useful if you really want to
> ensure isolation between tests (e.g. --run_isolated)
> - If the kernel is already tainted, and so all tests are skipped,
> kunit.py should report 'no tests run' as an error. I think this actually
> only reports an error if no tests are executed at the moment, so this
> may need looking at.
> - This will trigger if the kernel is tainted by some other part of the
> kernel either between KUnit tests, or in another kthread. However, in
> this case, the results will look like a series of successful tests,
> followed by skipped ones, with no failed tests. (And hence won't trigger
> a failure exit code from kunit.py). This is likely to be incredibly
> rare, but we could add a fail-on-skip option or similar if needed.
>
> Thoughts?
>
> Cheers,
> -- David
>
> >>
> >> 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)
> >>>
> >>
> > Best regards,
> > Malte :)
>