| Message ID | 20200806203737.3636830-1-urielguajardojr@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kunit: added lockdep support | expand |
On Thu, Aug 6, 2020 at 3:37 PM Uriel Guajardo <urielguajardojr@gmail.com> wrote: > > From: Uriel Guajardo <urielguajardo@google.com> > > KUnit tests will now fail if lockdep detects an error during a test > case. > > The idea comes from how lib/locking-selftest [1] checks for lock errors: we > first if lock debugging is turned on. If not, an error must have > occurred, so we fail the test and restart lockdep for the next test case. > > 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@gmail.com/ > > Signed-off-by: Uriel Guajardo <urielguajardo@google.com> > --- > lib/kunit/test.c | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index d8189d827368..0838ececa005 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -11,6 +11,8 @@ > #include <linux/kref.h> > #include <linux/sched/debug.h> > #include <linux/sched.h> > +#include <linux/lockdep.h> > +#include <linux/debug_locks.h> > > #include "debugfs.h" > #include "string-stream.h" > @@ -22,6 +24,26 @@ void kunit_fail_current_test(void) > kunit_set_failure(current->kunit_test); > } > > +static inline void kunit_check_locking_bugs(struct kunit *test, > + unsigned long saved_preempt_count) > +{ > + preempt_count_set(saved_preempt_count); > +#ifdef CONFIG_TRACE_IRQFLAGS > + if (softirq_count()) > + current->softirqs_enabled = 0; > + else > + current->softirqs_enabled = 1; > +#endif I am not entirely sure why lib/locking-selftests enables/disables softirqs, but I suspect it has to do with the fact that preempt_count became corrupted, and somehow softirqs became incorrectly enabled/disabled as a result. The resetting of the preemption count will undo the enabling/disabling accordingly. Any insight on this would be appreciated! > +#if IS_ENABLED(CONFIG_LOCKDEP) > + local_irq_disable(); > + if (!debug_locks) { > + kunit_set_failure(test); > + lockdep_reset(); > + } > + local_irq_enable(); > +#endif > +} > + > static void kunit_print_tap_version(void) > { > static bool kunit_has_printed_tap_version; > @@ -289,6 +311,7 @@ static void kunit_try_run_case(void *data) > struct kunit *test = ctx->test; > struct kunit_suite *suite = ctx->suite; > struct kunit_case *test_case = ctx->test_case; > + unsigned long saved_preempt_count = preempt_count(); > > current->kunit_test = test; > > @@ -298,7 +321,8 @@ static void kunit_try_run_case(void *data) > * thread will resume control and handle any necessary clean up. > */ > kunit_run_case_internal(test, suite, test_case); > - /* This line may never be reached. */ > + /* These lines may never be reached. */ > + kunit_check_locking_bugs(test, saved_preempt_count); > kunit_run_case_cleanup(test, suite); > } > > -- > 2.28.0.236.gb10cc79966-goog >
On Thu, Aug 6, 2020 at 1:37 PM Uriel Guajardo <urielguajardojr@gmail.com> wrote: > > From: Uriel Guajardo <urielguajardo@google.com> > > KUnit tests will now fail if lockdep detects an error during a test > case. > > The idea comes from how lib/locking-selftest [1] checks for lock errors: we > first if lock debugging is turned on. If not, an error must have > occurred, so we fail the test and restart lockdep for the next test case. > > 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@gmail.com/ > > Signed-off-by: Uriel Guajardo <urielguajardo@google.com> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
On Thu, Aug 6, 2020 at 1:43 PM Uriel Guajardo <urielguajardo@google.com> wrote: > > On Thu, Aug 6, 2020 at 3:37 PM Uriel Guajardo <urielguajardojr@gmail.com> wrote: > > > > From: Uriel Guajardo <urielguajardo@google.com> > > > > KUnit tests will now fail if lockdep detects an error during a test > > case. > > > > The idea comes from how lib/locking-selftest [1] checks for lock errors: we > > first if lock debugging is turned on. If not, an error must have > > occurred, so we fail the test and restart lockdep for the next test case. > > > > Like the locking selftests, we also fix possible preemption count > > corruption from lock bugs. Sorry, just noticed: You probably want to send this to some of the lockdep maintainers or the maintainers of the kselftest for lockdep. > > 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@gmail.com/ > > > > Signed-off-by: Uriel Guajardo <urielguajardo@google.com> > > --- > > lib/kunit/test.c | 26 +++++++++++++++++++++++++- > > 1 file changed, 25 insertions(+), 1 deletion(-) > > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > index d8189d827368..0838ececa005 100644 > > --- a/lib/kunit/test.c > > +++ b/lib/kunit/test.c > > @@ -11,6 +11,8 @@ > > #include <linux/kref.h> > > #include <linux/sched/debug.h> > > #include <linux/sched.h> > > +#include <linux/lockdep.h> > > +#include <linux/debug_locks.h> > > > > #include "debugfs.h" > > #include "string-stream.h" > > @@ -22,6 +24,26 @@ void kunit_fail_current_test(void) > > kunit_set_failure(current->kunit_test); > > } > > > > +static inline void kunit_check_locking_bugs(struct kunit *test, > > + unsigned long saved_preempt_count) > > +{ > > + preempt_count_set(saved_preempt_count); > > +#ifdef CONFIG_TRACE_IRQFLAGS > > + if (softirq_count()) > > + current->softirqs_enabled = 0; > > + else > > + current->softirqs_enabled = 1; > > +#endif > > I am not entirely sure why lib/locking-selftests enables/disables > softirqs, but I suspect it has to do with the fact that preempt_count > became corrupted, and somehow softirqs became incorrectly > enabled/disabled as a result. The resetting of the preemption count > will undo the enabling/disabling accordingly. Any insight on this > would be appreciated! > > > +#if IS_ENABLED(CONFIG_LOCKDEP) > > + local_irq_disable(); > > + if (!debug_locks) { > > + kunit_set_failure(test); > > + lockdep_reset(); > > + } > > + local_irq_enable(); > > +#endif > > +} > > + > > static void kunit_print_tap_version(void) > > { > > static bool kunit_has_printed_tap_version; > > @@ -289,6 +311,7 @@ static void kunit_try_run_case(void *data) > > struct kunit *test = ctx->test; > > struct kunit_suite *suite = ctx->suite; > > struct kunit_case *test_case = ctx->test_case; > > + unsigned long saved_preempt_count = preempt_count(); > > > > current->kunit_test = test; > > > > @@ -298,7 +321,8 @@ static void kunit_try_run_case(void *data) > > * thread will resume control and handle any necessary clean up. > > */ > > kunit_run_case_internal(test, suite, test_case); > > - /* This line may never be reached. */ > > + /* These lines may never be reached. */ > > + kunit_check_locking_bugs(test, saved_preempt_count); > > kunit_run_case_cleanup(test, suite); > > } > > > > -- > > 2.28.0.236.gb10cc79966-goog > >
On Mon, Aug 10, 2020 at 3:35 PM Brendan Higgins <brendanhiggins@google.com> wrote: > > On Thu, Aug 6, 2020 at 1:43 PM Uriel Guajardo <urielguajardo@google.com> wrote: > > > > On Thu, Aug 6, 2020 at 3:37 PM Uriel Guajardo <urielguajardojr@gmail.com> wrote: > > > > > > From: Uriel Guajardo <urielguajardo@google.com> > > > > > > KUnit tests will now fail if lockdep detects an error during a test > > > case. > > > > > > The idea comes from how lib/locking-selftest [1] checks for lock errors: we > > > first if lock debugging is turned on. If not, an error must have > > > occurred, so we fail the test and restart lockdep for the next test case. > > > > > > Like the locking selftests, we also fix possible preemption count > > > corruption from lock bugs. > > Sorry, just noticed: You probably want to send this to some of the > lockdep maintainers or the maintainers of the kselftest for lockdep. > Thanks for the reminder. CC'ed lockdep maintainers. > > > 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@gmail.com/ > > > > > > Signed-off-by: Uriel Guajardo <urielguajardo@google.com> > > > --- > > > lib/kunit/test.c | 26 +++++++++++++++++++++++++- > > > 1 file changed, 25 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > > index d8189d827368..0838ececa005 100644 > > > --- a/lib/kunit/test.c > > > +++ b/lib/kunit/test.c > > > @@ -11,6 +11,8 @@ > > > #include <linux/kref.h> > > > #include <linux/sched/debug.h> > > > #include <linux/sched.h> > > > +#include <linux/lockdep.h> > > > +#include <linux/debug_locks.h> > > > > > > #include "debugfs.h" > > > #include "string-stream.h" > > > @@ -22,6 +24,26 @@ void kunit_fail_current_test(void) > > > kunit_set_failure(current->kunit_test); > > > } > > > > > > +static inline void kunit_check_locking_bugs(struct kunit *test, > > > + unsigned long saved_preempt_count) > > > +{ > > > + preempt_count_set(saved_preempt_count); > > > +#ifdef CONFIG_TRACE_IRQFLAGS > > > + if (softirq_count()) > > > + current->softirqs_enabled = 0; > > > + else > > > + current->softirqs_enabled = 1; > > > +#endif > > > > I am not entirely sure why lib/locking-selftests enables/disables > > softirqs, but I suspect it has to do with the fact that preempt_count > > became corrupted, and somehow softirqs became incorrectly > > enabled/disabled as a result. The resetting of the preemption count > > will undo the enabling/disabling accordingly. Any insight on this > > would be appreciated! > > > > > +#if IS_ENABLED(CONFIG_LOCKDEP) > > > + local_irq_disable(); > > > + if (!debug_locks) { > > > + kunit_set_failure(test); > > > + lockdep_reset(); > > > + } > > > + local_irq_enable(); > > > +#endif > > > +} > > > + > > > static void kunit_print_tap_version(void) > > > { > > > static bool kunit_has_printed_tap_version; > > > @@ -289,6 +311,7 @@ static void kunit_try_run_case(void *data) > > > struct kunit *test = ctx->test; > > > struct kunit_suite *suite = ctx->suite; > > > struct kunit_case *test_case = ctx->test_case; > > > + unsigned long saved_preempt_count = preempt_count(); > > > > > > current->kunit_test = test; > > > > > > @@ -298,7 +321,8 @@ static void kunit_try_run_case(void *data) > > > * thread will resume control and handle any necessary clean up. > > > */ > > > kunit_run_case_internal(test, suite, test_case); > > > - /* This line may never be reached. */ > > > + /* These lines may never be reached. */ > > > + kunit_check_locking_bugs(test, saved_preempt_count); > > > kunit_run_case_cleanup(test, suite); > > > } > > > > > > -- > > > 2.28.0.236.gb10cc79966-goog > > >
On Mon, Aug 10, 2020 at 04:17:42PM -0500, Uriel Guajardo wrote: > On Mon, Aug 10, 2020 at 3:35 PM Brendan Higgins > <brendanhiggins@google.com> wrote: > > > > On Thu, Aug 6, 2020 at 1:43 PM Uriel Guajardo <urielguajardo@google.com> wrote: > > > > > > On Thu, Aug 6, 2020 at 3:37 PM Uriel Guajardo <urielguajardojr@gmail.com> wrote: > > > > > > > > From: Uriel Guajardo <urielguajardo@google.com> > > > > > > > > KUnit tests will now fail if lockdep detects an error during a test > > > > case. > > > > > > > > The idea comes from how lib/locking-selftest [1] checks for lock errors: we > > > > first if lock debugging is turned on. If not, an error must have > > > > occurred, so we fail the test and restart lockdep for the next test case. > > > > > > > > Like the locking selftests, we also fix possible preemption count > > > > corruption from lock bugs. > > > > Sorry, just noticed: You probably want to send this to some of the > > lockdep maintainers or the maintainers of the kselftest for lockdep. > > > > Thanks for the reminder. CC'ed lockdep maintainers. Please resend as a whole, this is an unreadable mess.
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index d8189d827368..0838ececa005 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -11,6 +11,8 @@ #include <linux/kref.h> #include <linux/sched/debug.h> #include <linux/sched.h> +#include <linux/lockdep.h> +#include <linux/debug_locks.h> #include "debugfs.h" #include "string-stream.h" @@ -22,6 +24,26 @@ void kunit_fail_current_test(void) kunit_set_failure(current->kunit_test); } +static inline void kunit_check_locking_bugs(struct kunit *test, + unsigned long saved_preempt_count) +{ + 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) + local_irq_disable(); + if (!debug_locks) { + kunit_set_failure(test); + lockdep_reset(); + } + local_irq_enable(); +#endif +} + static void kunit_print_tap_version(void) { static bool kunit_has_printed_tap_version; @@ -289,6 +311,7 @@ static void kunit_try_run_case(void *data) struct kunit *test = ctx->test; struct kunit_suite *suite = ctx->suite; struct kunit_case *test_case = ctx->test_case; + unsigned long saved_preempt_count = preempt_count(); current->kunit_test = test; @@ -298,7 +321,8 @@ static void kunit_try_run_case(void *data) * thread will resume control and handle any necessary clean up. */ kunit_run_case_internal(test, suite, test_case); - /* This line may never be reached. */ + /* These lines may never be reached. */ + kunit_check_locking_bugs(test, saved_preempt_count); kunit_run_case_cleanup(test, suite); }