[PATCH 1/2] kunit: Provide a static key to check if KUnit is actively running tests

[PATCH 1/2] kunit: Provide a static key to check if KUnit is actively running tests

[David Gow]

KUnit does a few expensive things when enabled. This hasn't been a
problem because KUnit was only enabled on test kernels, but with a few
people enabling (but not _using_) KUnit on production systems, we need a
runtime way of handling this.

Provide a 'kunit_running' static key (defaulting to false), which allows
us to hide any KUnit code behind a static branch. This should reduce the
performance impact (on other code) of having KUnit enabled to a single
NOP when no tests are running.

Note that, while it looks unintuitive, tests always run entirely within
__kunit_test_suites_init(), so it's safe to decrement the static key at
the end of this function, rather than in __kunit_test_suites_exit(),
which is only there to clean up results in debugfs.

Signed-off-by: David Gow <davidgow@google.com>
---

This should be a no-op (other than a possible performance improvement)
functionality-wise, and lays the groundwork for a more optimised static
stub implementation.

---
 include/kunit/test.h | 4 ++++
 lib/kunit/test.c     | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index b1ab6b32216d..450a778a039e 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -16,6 +16,7 @@
 #include <linux/container_of.h>
 #include <linux/err.h>
 #include <linux/init.h>
+#include <linux/jump_label.h>
 #include <linux/kconfig.h>
 #include <linux/kref.h>
 #include <linux/list.h>
@@ -27,6 +28,9 @@
 
 #include <asm/rwonce.h>
 
+/* Static key: true if any KUnit tests are currently running */
+extern struct static_key_false kunit_running;
+
 struct kunit;
 
 /* Size of log associated with test. */
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 90640a43cf62..314717b63080 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -20,6 +20,8 @@
 #include "string-stream.h"
 #include "try-catch-impl.h"
 
+DEFINE_STATIC_KEY_FALSE(kunit_running);
+
 #if IS_BUILTIN(CONFIG_KUNIT)
 /*
  * Fail the current test and print an error message to the log.
@@ -612,10 +614,14 @@ int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_
 		return 0;
 	}
 
+	static_branch_inc(&kunit_running);
+
 	for (i = 0; i < num_suites; i++) {
 		kunit_init_suite(suites[i]);
 		kunit_run_tests(suites[i]);
 	}
+
+	static_branch_dec(&kunit_running);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(__kunit_test_suites_init);
-- 
2.38.0.135.g90850a2211-goog

[PATCH 2/2] kunit: Use the static key in kunit_fail_current_test()

[David Gow]

Speed up the case where kunit_fail_current_test() is called when no test
is running. This should make it convenient for code to call this
unconditionally in some error paths, without fear of causing a
performance problem.

If CONFIG_KUNIT=n, this compiles away to nothing. If CONFIG_KUNIT=y, it
will compile down to a NOP (on most architectures) if no KUnit test is
currently running. kunit_fail_current_test() does not work if KUnit
itself is built as a module, though this is a pre-existing limitation.

Note that the definition of kunit_fail_current_test() still wraps an
empty, inline function if KUnit is not built-in. This is to ensure that
the printf format string __attribute__ will still work.

Signed-off-by: David Gow <davidgow@google.com>
---
 include/kunit/test-bug.h | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
index 5fc58081d511..ba9558a9f9c0 100644
--- a/include/kunit/test-bug.h
+++ b/include/kunit/test-bug.h
@@ -9,16 +9,29 @@
 #ifndef _KUNIT_TEST_BUG_H
 #define _KUNIT_TEST_BUG_H
 
-#define kunit_fail_current_test(fmt, ...) \
-	__kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__)
-
 #if IS_BUILTIN(CONFIG_KUNIT)
 
+#include <linux/jump_label.h> /* For static branch */
+
+/* Static key if KUnit is running any tests. */
+extern struct static_key_false kunit_running;
+
+#define kunit_fail_current_test(fmt, ...) do {					\
+	if (static_branch_unlikely(&kunit_running)) {				\
+		__kunit_fail_current_test(__FILE__, __LINE__,			\
+					  fmt, ##__VA_ARGS__);			\
+	} while (0)
+
+
 extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,
 						    const char *fmt, ...);
 
 #else
 
+/* We define this with an empty helper function so format string warnings work */
+#define kunit_fail_current_test(fmt, ...) \
+		__kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__)
+
 static inline __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,
 							    const char *fmt, ...)
 {
-- 
2.38.0.135.g90850a2211-goog

Re: [PATCH 2/2] kunit: Use the static key in kunit_fail_current_test()

[Kees Cook]

On October 21, 2022 12:28:55 AM PDT, David Gow <davidgow@google.com> wrote:
>Speed up the case where kunit_fail_current_test() is called when no test
>is running. This should make it convenient for code to call this
>unconditionally in some error paths, without fear of causing a
>performance problem.

A third patch showing these cases may help in understanding the utility. I get it, but I lack imagination on where/why they would be added. :)

>
>If CONFIG_KUNIT=n, this compiles away to nothing. If CONFIG_KUNIT=y, it
>will compile down to a NOP (on most architectures) if no KUnit test is
>currently running. kunit_fail_current_test() does not work if KUnit
>itself is built as a module, though this is a pre-existing limitation.
>
>Note that the definition of kunit_fail_current_test() still wraps an
>empty, inline function if KUnit is not built-in. This is to ensure that
>the printf format string __attribute__ will still work.
>
>Signed-off-by: David Gow <davidgow@google.com>
>---
> include/kunit/test-bug.h | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
>diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
>index 5fc58081d511..ba9558a9f9c0 100644
>--- a/include/kunit/test-bug.h
>+++ b/include/kunit/test-bug.h
>@@ -9,16 +9,29 @@
> #ifndef _KUNIT_TEST_BUG_H
> #define _KUNIT_TEST_BUG_H
> 
>-#define kunit_fail_current_test(fmt, ...) \
>-	__kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__)
>-
> #if IS_BUILTIN(CONFIG_KUNIT)
> 
>+#include <linux/jump_label.h> /* For static branch */
>+
>+/* Static key if KUnit is running any tests. */
>+extern struct static_key_false kunit_running;
>+
>+#define kunit_fail_current_test(fmt, ...) do {					\
>+	if (static_branch_unlikely(&kunit_running)) {				\

This trailing { should be dropped.

>+		__kunit_fail_current_test(__FILE__, __LINE__,			\
>+					  fmt, ##__VA_ARGS__);			\

Or a closing one added here. (The {}s are unbalanced, as 0day complained about.)

>+	} while (0)
>+
>+
> extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,
> 						    const char *fmt, ...);
> 
> #else
> 
>+/* We define this with an empty helper function so format string warnings work */
>+#define kunit_fail_current_test(fmt, ...) \
>+		__kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__)
>+
> static inline __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,
> 							    const char *fmt, ...)
> {


-- 
Kees Cook