| Message ID | 20220111194231.1797841-3-dlatypov@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4fdacef8ac5a5382eeb1bc6fc2632d71a09d52cd |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: refactor assertions to use less stack | expand |
On Wed, Jan 12, 2022 at 3:42 AM Daniel Latypov <dlatypov@google.com> wrote: > > Currently the code always calls kunit_do_assertion() even though it does > nothing when `pass` is true. > > This change moves the `if(!(pass))` check into the macro instead > and renames the function to kunit_do_failed_assertion(). > I feel this a bit easier to read and understand. > > This has the potential upside of avoiding a function call that does > nothing most of the time (assuming your tests are passing) but comes > with the downside of generating a bit more code and branches. We try to > mitigate the branches by tagging them with `unlikely()`. > > This also means we don't have to initialize structs that we don't need, > which will become a tiny bit more expensive if we switch over to using > static variables to try and reduce stack usage. (There's runtime code > to check if the variable has been initialized yet or not). > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > --- This looks good. I'm still not 100% sold that putting the if() outside the function is significantly easier to read, but I don't think it's harder to read either, and getting rid of the function call is probably worth it. Reviewed-by: David Gow <davidgow@google.com> -- David > include/kunit/test.h | 21 +++++++++++---------- > lib/kunit/test.c | 13 ++++--------- > 2 files changed, 15 insertions(+), 19 deletions(-) > > diff --git a/include/kunit/test.h b/include/kunit/test.h > index b26400731c02..12cabd15449a 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -12,6 +12,7 @@ > #include <kunit/assert.h> > #include <kunit/try-catch.h> > > +#include <linux/compiler.h> > #include <linux/container_of.h> > #include <linux/err.h> > #include <linux/init.h> > @@ -770,18 +771,18 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...); > */ > #define KUNIT_SUCCEED(test) do {} while (0) > > -void kunit_do_assertion(struct kunit *test, > - struct kunit_assert *assert, > - bool pass, > - const char *fmt, ...); > +void kunit_do_failed_assertion(struct kunit *test, > + struct kunit_assert *assert, > + const char *fmt, ...); > > #define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do { \ > - struct assert_class __assertion = INITIALIZER; \ > - kunit_do_assertion(test, \ > - &__assertion.assert, \ > - pass, \ > - fmt, \ > - ##__VA_ARGS__); \ > + if (unlikely(!(pass))) { \ > + struct assert_class __assertion = INITIALIZER; \ > + kunit_do_failed_assertion(test, \ > + &__assertion.assert, \ > + fmt, \ > + ##__VA_ARGS__); \ > + } \ > } while (0) > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index c7ed4aabec04..3a52c321c280 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -275,16 +275,11 @@ static void __noreturn kunit_abort(struct kunit *test) > WARN_ONCE(true, "Throw could not abort from test!\n"); > } > > -void kunit_do_assertion(struct kunit *test, > - struct kunit_assert *assert, > - bool pass, > - const char *fmt, ...) > +void kunit_do_failed_assertion(struct kunit *test, > + struct kunit_assert *assert, > + const char *fmt, ...) > { > va_list args; > - > - if (pass) > - return; > - > va_start(args, fmt); > > assert->message.fmt = fmt; > @@ -297,7 +292,7 @@ void kunit_do_assertion(struct kunit *test, > if (assert->type == KUNIT_ASSERTION) > kunit_abort(test); > } > -EXPORT_SYMBOL_GPL(kunit_do_assertion); > +EXPORT_SYMBOL_GPL(kunit_do_failed_assertion); > > void kunit_init_test(struct kunit *test, const char *name, char *log) > { > -- > 2.34.1.575.g55b058a8bb-goog >
diff --git a/include/kunit/test.h b/include/kunit/test.h index b26400731c02..12cabd15449a 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -12,6 +12,7 @@ #include <kunit/assert.h> #include <kunit/try-catch.h> +#include <linux/compiler.h> #include <linux/container_of.h> #include <linux/err.h> #include <linux/init.h> @@ -770,18 +771,18 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...); */ #define KUNIT_SUCCEED(test) do {} while (0) -void kunit_do_assertion(struct kunit *test, - struct kunit_assert *assert, - bool pass, - const char *fmt, ...); +void kunit_do_failed_assertion(struct kunit *test, + struct kunit_assert *assert, + const char *fmt, ...); #define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do { \ - struct assert_class __assertion = INITIALIZER; \ - kunit_do_assertion(test, \ - &__assertion.assert, \ - pass, \ - fmt, \ - ##__VA_ARGS__); \ + if (unlikely(!(pass))) { \ + struct assert_class __assertion = INITIALIZER; \ + kunit_do_failed_assertion(test, \ + &__assertion.assert, \ + fmt, \ + ##__VA_ARGS__); \ + } \ } while (0) diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c7ed4aabec04..3a52c321c280 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -275,16 +275,11 @@ static void __noreturn kunit_abort(struct kunit *test) WARN_ONCE(true, "Throw could not abort from test!\n"); } -void kunit_do_assertion(struct kunit *test, - struct kunit_assert *assert, - bool pass, - const char *fmt, ...) +void kunit_do_failed_assertion(struct kunit *test, + struct kunit_assert *assert, + const char *fmt, ...) { va_list args; - - if (pass) - return; - va_start(args, fmt); assert->message.fmt = fmt; @@ -297,7 +292,7 @@ void kunit_do_assertion(struct kunit *test, if (assert->type == KUNIT_ASSERTION) kunit_abort(test); } -EXPORT_SYMBOL_GPL(kunit_do_assertion); +EXPORT_SYMBOL_GPL(kunit_do_failed_assertion); void kunit_init_test(struct kunit *test, const char *name, char *log) {