On Wed, 12 Aug 2020, Uriel Guajardo wrote: > KUnit will fail tests upon observing a lockdep failure. Because lockdep > turns itself off after its first failure, only fail the first test and > warn users to not expect any future failures from lockdep. > > Similar to lib/locking-selftest [1], we check if the status of > debug_locks has changed after the execution of a test case. However, we > do not reset lockdep afterwards. > > Like the locking selftests, we also fix possible preemption count > corruption from lock bugs. > > Depends on kunit: support failure from dynamic analysis tools [2] > > [1] https://elixir.bootlin.com/linux/v5.7.12/source/lib/locking-selftest.c#L1137 > > [2] https://lore.kernel.org/linux-kselftest/20200806174326.3577537-1-urielguajardojr@xxxxxxxxx/ > > Signed-off-by: Uriel Guajardo <urielguajardo@xxxxxxxxxx> > --- > v2 Changes: > - Removed lockdep_reset > > - Added warning to users about lockdep shutting off > --- > lib/kunit/test.c | 27 ++++++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index d8189d827368..7e477482457b 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -11,6 +11,7 @@ > #include <linux/kref.h> > #include <linux/sched/debug.h> > #include <linux/sched.h> > +#include <linux/debug_locks.h> > > #include "debugfs.h" > #include "string-stream.h" > @@ -22,6 +23,26 @@ void kunit_fail_current_test(void) > kunit_set_failure(current->kunit_test); > } > > +static void kunit_check_locking_bugs(struct kunit *test, > + unsigned long saved_preempt_count, > + bool saved_debug_locks) > +{ > + preempt_count_set(saved_preempt_count); > +#ifdef CONFIG_TRACE_IRQFLAGS > + if (softirq_count()) > + current->softirqs_enabled = 0; > + else > + current->softirqs_enabled = 1; > +#endif > +#if IS_ENABLED(CONFIG_LOCKDEP) > + if (saved_debug_locks && !debug_locks) { > + kunit_set_failure(test); > + kunit_warn(test, "Dynamic analysis tool failure from LOCKDEP."); > + kunit_warn(test, "Further tests will have LOCKDEP disabled."); > + } > +#endif > +} Nit: I could be wrong but the general approach for this sort of feature is to do conditional compilation combined with "static inline" definitions to handle the case where the feature isn't enabled. Could we tidy this up a bit and haul this stuff out into a conditionally-compiled (if CONFIG_LOCKDEP) kunit lockdep.c file? Then in kunit's lockdep.h we'd have struct kunit_lockdep { int preempt_count; bool debug_locks; }; #if IS_ENABLED(CONFIG_LOCKDEP) void kunit_test_init_lockdep(struct kunit_test *test, struct kunit_lockdep *lockdep); void kunit_test_check_lockdep(struct kunit_test *test, struct kunit_lockdep *lockdep); #else static inline void kunit_init_lockdep(struct kunit_test *test, struct kunit_lockdep *lockdep) { } static inline void kunit_check_lockdep(struct kunit_test *test, struct kunit_lockdep *lockdep) { } #endif The test execution code could then call struct kunit_lockdep lockdep; kunit_test_init_lockdep(test, &lockdep); kunit_test_check_lockdep(test, &lockdep); If that approach makes sense, we could go a bit further and we might benefit from a bit more generalization here. _If_ the pattern of needing pre- and post- test actions is sustained across multiple analysis tools, could we add generic hooks for this? That would allow any additional dynamic analysis tools to utilize them. So kunit_try_run_case() would then cycle through the registered pre- hooks prior to running the case and post- hooks after, failing if any of the latter returned a failure value. I'm thinking something like kunit_register_external_test("lockdep", lockdep_pre, lockdep_post, &kunit_lockdep); (or we could define a kunit_external_test struct for better extensibility). A void * would be passed to pre/post, in this case it'd be a pointer to a struct containing the saved preempt count/debug locks, and the registration could be called during kunit initialization. This doesn't need to be done with your change of course but I wanted to float the idea as in addition to uncluttering the test case execution code, it might allow us to build facilities on top of that generic tool support for situations like "I'd like to see if the test passes absent any lockdep issues, so I'd like to disable lockdep-based failure". Such situations are more likely to arise in a world where kunit+tests are built as modules and run multiple times within a single system boot admittedly, but worth considering I think. For that we'd need a way to select which dynamic tools kunit enables(kernel/module parameters or debugfs could do this), but a generic approach might help that sort of thing. An external test under this model wouldn't have to necessarily be external to the area under test; the general criteria for such things would be "something I want to track across multiple test case execution". Again I'm not trying to put you on the hook for any of the above suggestions (having lockdep support like this is fantastic!), but I think it'd be good to see if there's a pattern here we could potentially exploit in other use cases. Thanks! Alan
