Den tirs. 25. aug. 2026 kl. 13.00 skrev David Gow <[email protected]>:
>
> Le 24/08/2026 à 21:32, Malte Wechter a écrit :
> > add extra assertions for each individual test case, that checks that
> > both `debug_locks` and `TAINT_WARN` are intact after the test is run.
> > The assertions are optional behind CONFIG_KUNIT_EXTRA_ASSERTS.
> >
> > Signed-off-by: Malte Wechter <[email protected]>
> > ---
>
> Thanks for this.
>
> I'd prefer to split this up into two separate changes, one to check for
> warnings, and one for lockdep issues.
I agree that this would be better.
>
> There's already a patch series to handle lockdep failures here:
> https://lore.kernel.org/all/[email protected]/
>
> It's pretty similar to this, but may have some good ideas in it.
>
> As for WARN, we have a new feature which allows individual WARN calls to
> be expected and ignored. This works by having the WARN() macro directly
> check if a test is failing. The difference there is that the code only
> runs if the current thread is part of the test (so it wouldn't be
> affected by a WARN() in another, non-test thread). There are advantages
> and disadvantages to this, but it's probably sensible to be consistent here.
>
> Take a look at this for more details on how it works:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=85347718ab0dd7ede9c3e1dcff2d604c7073df05
>
> (That being said, the current implementation does seem to work pretty
> well even with the backtrace suppression, so continuing to use this
> implementation is not necessarily a dealbreaker.)
>
> Finally, I'd appreciate there being some more documentation of this, if
> possible. In particular, it'd be nice to extend the KUnit documentation
> in Documentation/dev-tools/kunit to mention this and provide some
> examples of how to enable it. In particular, it'd be good to have an
> example kunit.py invocation which enables lockdep, e.g.
> ./tools/testing/kunit/kunit.py run --kconfig_add
> CONFIG_KUNIT_EXTRA_ASSERTS=y --kconfig_add CONFIG_PROVE_LOCKING=y
> --kconfig_add CONFIG_DEBUG_KERNEL=y
Will do!
>
> Thanks again for looking into this!
>
> Cheers,
> -- David
>
>
> > lib/kunit/Kconfig | 12 ++++++++++++
> > lib/kunit/try-catch.c | 23 ++++++++++++++++++++++-
> > 2 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> > index 94ff8e4089bfb..38801f7493669 100644
> > --- a/lib/kunit/Kconfig
> > +++ b/lib/kunit/Kconfig
> > @@ -142,4 +142,16 @@ config KUNIT_UML_PCI
> >
> > If unsure, say N.
> >
> > +config KUNIT_EXTRA_ASSERTS
> > + bool "Enable extra assertions in KUnit tests"
> > + depends on LOCKDEP
> > + default n
> > + help
> > + Enables all extra assertions for KUnit which includes
> > asserting `TAINT_WARN` and
> > + `debug_locks` from lockdep. A KUnit test suite (and test
> > case) is inserted
> > + at the start of all KUnit test suites. This makes assertions
> > prior to running any
> > + tests, as a pre-test integrity check. Assertions are made
> > after each test case which
> > + asserts that each test case did not trigger either
> > `TAINT_WARN` or `debug_locks`.
> > +
> > + If unsure, say N.
>
> A few notes here:
> - I'd rather have the taint/warn and lockdep assertions separate here.
> - A more descriptive name than "EXTRA_ASSERTS" would be nice. Perhaps
> something like KUNIT_FAIL_TEST_ON_WARN / KUNIT_FAIL_ON_LOCKDEP or similar?
>
> > endif # KUNIT
> > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> > index d84a879f0a789..7eea3af4c9671 100644
> > --- a/lib/kunit/try-catch.c
> > +++ b/lib/kunit/try-catch.c
> > @@ -41,6 +41,11 @@ void kunit_try_catch_run(struct kunit_try_catch
> > *try_catch, void *context)
> > struct completion *task_done;
> > int exit_code, time_remaining;
> >
> > + #ifdef CONFIG_KUNIT_EXTRA_ASSERTS
> > + int debug_locks_snapshot = debug_locks;
> > + int tainted_warn_snapshot = test_taint(TAINT_WARN);
> > + #endif
>
> Nit: Let's keep the preprocessor #ifdef/#endif lines un-indented here.
>
> > +
> > try_catch->context = context;
> > try_catch->try_result = 0;
> > task_struct = kthread_create(kunit_generic_run_threadfn_adapter,
> > @@ -70,7 +75,23 @@ void kunit_try_catch_run(struct kunit_try_catch
> > *try_catch, void *context)
> > put_task_struct(task_struct);
> > exit_code = try_catch->try_result;
> >
> > - if (!exit_code)
> > + #ifdef CONFIG_KUNIT_EXTRA_ASSERTS
> > + bool extra_assert = false;
>
> Please define all variables at the top of the function / scope.
>
> That being said, we don't need this extra_assert variable at all if you
> move the setting of try_result above moving it to exit_code above?
>
> > +
> > + if (debug_locks_snapshot != debug_locks && !exit_code) {
> > + extra_assert = true;
> > + try_catch->try_result = -EDEADLK;
>
> Why are we setting this, and then never handling it?
>
> > + kunit_err(test, "Test triggered lockdep\n");
> > + } else if (tainted_warn_snapshot != test_taint(TAINT_WARN) &&
> > !exit_code) {
> > + extra_assert = true;
> > + try_catch->try_result = -EDEADLK;
>
> -EDEADLK made sense for the lockdep implementation, but makes less sense
> for WARN(). If you really want to keep it, please document it.
>
> That being said, it should be possible to do this outside of the
> try/catch scope, as both lockdep and taints are global. And if you fail
> the test using the hook mechanism (like the warning suppression does),
> you won't need a separate check afterwards.
I believe that installing two additional hooks (one for lockdep and
one for warn, possibly a third for the initial suite check) is a
cleaner approach. I
will move the assertion logic into KUnit hooks instead of having it
inside of the try_catch_run.
>
> > + kunit_err(test, "Test tainted kernel with TAINT_WARN\n");
> > + }
> > + #else
> > + bool extra_assert = false;
>
> If we declare this unconditionally at the top of the function, there's
> no need to have it in an #ifdef.
>
> > + #endif
> > +
> > + if (!exit_code && !extra_assert)
> > return;
> >
> > if (exit_code == -EFAULT)
> >
>
Best regards,
Malte :)
