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

[PATCH v2 1/3] 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.

The remaining patches in the series add a kunit_get_current_test()
function which is a more friendly and performant wrapper around
current->kunit_test, and use this in the slub test. They also improve
the documentation a bit.

If there are no objections, we'll take the whole series via the KUnit
tree.

Changes since v1:
https://lore.kernel.org/linux-kselftest/20221021072854.333010-1-davidgow@google.com/
- No changes in this patch.
- Patch 2/3 is reworked, patch 3/3 is new.

---
 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 v2 2/3] kunit: Use the static key when retrieving the current test

[David Gow]

In order to detect if a KUnit test is running, and to access its
context, the 'kunit_test' member of the current task_struct is used.
Usually, this is accessed directly or via the kunit_fail_current_task()
function.

In order to speed up the case where no test is running, add a wrapper,
kunit_get_current_test(), which uses the static key to fail early.
Equally, Speed up kunit_fail_current_test() by using the static key.

This should make it convenient for code to call this
unconditionally in fakes or error paths, without worrying that this will
slow the code down significantly.

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.

Also update the documentation to suggest users use the new
kunit_get_current_test() function, update the example, and to describe
the behaviour when KUnit is disabled better.

Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Sadiya Kazi <sadiyakazi@google.com>
Signed-off-by: David Gow <davidgow@google.com>
---

As-is, the only code which will be directly affected by this (via the
kunit_fail_current_test() change) will be UBSAN's KUnit integration.

Patches to port other tests to use kunit_get_current_test() will be sent
separately (other than the SLUB one in patch 3/3). KASAN in particular
are reworking their KUnit tests and integration, so we'll use this in a
follow up to avoid introducing a conflict.

Changes since v1:
https://lore.kernel.org/linux-kselftest/20221021072854.333010-2-davidgow@google.com/
- Fix a missing '}' which broke everything. Thanks Kees, kernel test
  robot.
- Add the new kunit_get_current_test() function, as most of the cases
  where we retrieve the current test (even to fail it) were accessing
  current->kunit_test directly, not using kunit_fail_current_test().
- Add some documentation comments.
- Update the documentation in usage.rst.
  - The version in tips.rst was not updated, and will be removed:
  https://lore.kernel.org/linux-kselftest/20221025055844.1231592-1-davidgow@google.com/

---
 Documentation/dev-tools/kunit/usage.rst | 24 +++++++----
 include/kunit/test-bug.h                | 56 ++++++++++++++++++++++++-
 2 files changed, 69 insertions(+), 11 deletions(-)

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 2737863ef365..05786564b968 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -625,17 +625,20 @@ as shown in next section: *Accessing The Current Test*.
 Accessing The Current Test
 --------------------------
 
-In some cases, we need to call test-only code from outside the test file.
-For example, see example in section *Injecting Test-Only Code* or if
-we are providing a fake implementation of an ops struct. Using
-``kunit_test`` field in ``task_struct``, we can access it via
-``current->kunit_test``.
+In some cases, we need to call test-only code from outside the test file,
+for example,  when providing a fake implementation of a function, or to fail
+any current test from within an error handler.
+We can do this via the ``kunit_test`` field in ``task_struct``, which we can
+access using the ``kunit_get_current_test`` function in ``kunit/test-bug.h``.
 
-The example below includes how to implement "mocking":
+``kunit_get_current_test`` is safe to call even if KUnit is not enabled. If KUnit
+is disabled, or no test is running in the current task, it will quickly return ``NULL``.
+
+The example below uses this to implement a "mock" implementation of a function, ``foo``:
 
 .. code-block:: c
 
-	#include <linux/sched.h> /* for current */
+	#include <kunit/test-bug.h> /* for kunit_get_current_test */
 
 	struct test_data {
 		int foo_result;
@@ -644,7 +647,7 @@ The example below includes how to implement "mocking":
 
 	static int fake_foo(int arg)
 	{
-		struct kunit *test = current->kunit_test;
+		struct kunit *test = kunit_get_current_test();
 		struct test_data *test_data = test->priv;
 
 		KUNIT_EXPECT_EQ(test, test_data->want_foo_called_with, arg);
@@ -675,7 +678,7 @@ Each test can have multiple resources which have string names providing the same
 flexibility as a ``priv`` member, but also, for example, allowing helper
 functions to create resources without conflicting with each other. It is also
 possible to define a clean up function for each resource, making it easy to
-avoid resource leaks. For more information, see Documentation/dev-tools/kunit/api/test.rst.
+avoid resource leaks. For more information, see Documentation/dev-tools/kunit/api/resource.rst.
 
 Failing The Current Test
 ------------------------
@@ -703,3 +706,6 @@ structures as shown below:
 	static void my_debug_function(void) { }
 	#endif
 
+Note that ``kunit_fail_current_test`` requires KUnit be built-in to the kernel, i.e.
+``CONFIG_KUNIT=y``. It is safe to call even if KUnit is not enabled, is built as a module,
+or no test is currently running, but will do nothing.
\ No newline at end of file
diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
index 5fc58081d511..85b085030a7a 100644
--- a/include/kunit/test-bug.h
+++ b/include/kunit/test-bug.h
@@ -9,16 +9,68 @@
 #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_ENABLED(CONFIG_KUNIT)
+
+#include <linux/jump_label.h> /* For static branch */
+#include <linux/sched.h>
+
+/* Static key if KUnit is running any tests. */
+extern struct static_key_false kunit_running;
+
+/**
+ * kunit_get_current_test() - Return a pointer to the currently-running
+ *			      KUnit test.
+ *
+ * If a KUnit test is running in the current task, returns a pointer to
+ * its associated struct kunit, which can then be passed to any KUnit function
+ * or assertion. If no test is running (or a test is running in a different
+ * task), returns NULL.
+ *
+ * This function is safe to call even when KUnit is disabled: it will compile
+ * down to nothing if CONFIG_KUNIT is not enabled, and will be very fast if
+ * no test is running.
+ */
+static inline struct kunit *kunit_get_current_test(void)
+{
+	if (!static_branch_unlikely(&kunit_running))
+		return NULL;
+
+	return current->kunit_test;
+}
+
+#else
+
+static inline struct kunit *kunit_get_current_test(void) { return NULL; }
+
+#endif
 
 #if IS_BUILTIN(CONFIG_KUNIT)
 
+/**
+ * kunit_fail_current_test() - If a KUnit test is running, fail it.
+ *
+ * If a KUnit test is running in the current task, mark that test as failed.
+ *
+ * This macro will only work if KUnit is built-in (though the tests
+ * themselves can be modules). Otherwise, it compiles down to nothing.
+ */
+#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

[PATCH v2 3/3] mm: slub: test: Use the kunit_get_current_test() function

[David Gow]

Use the newly-added function kunit_get_current_test() instead of
accessing current->kunit_test directly. This function uses a static key
to return more quickly when KUnit is enabled, but no tests are actively
running. There should therefore be a negligible performance impact to
enabling the slub KUnit tests.

Other than the performance improvement, this should be a no-op.

Cc: Oliver Glitta <glittao@gmail.com>
Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: David Rientjes <rientjes@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: David Gow <davidgow@google.com>
---

This is intended as an example use of the new function. Other users
(such as KASAN) will be updated separately, as there would otherwise be
conflicts.

Assuming there are no objections, we'll take this whole series via the
kselftest/kunit tree.

There was no v1 of this patch. v1 of the series can be found here:
https://lore.kernel.org/linux-kselftest/20221021072854.333010-1-davidgow@google.com/T/#u

---
 lib/slub_kunit.c | 1 +
 mm/slub.c        | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
index 7a0564d7cb7a..8fd19c8301ad 100644
--- a/lib/slub_kunit.c
+++ b/lib/slub_kunit.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <kunit/test.h>
+#include <kunit/test-bug.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/module.h>
diff --git a/mm/slub.c b/mm/slub.c
index 157527d7101b..15d10d250ef2 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -39,6 +39,7 @@
 #include <linux/memcontrol.h>
 #include <linux/random.h>
 #include <kunit/test.h>
+#include <kunit/test-bug.h>
 #include <linux/sort.h>
 
 #include <linux/debugfs.h>
@@ -603,10 +604,10 @@ static bool slab_add_kunit_errors(void)
 {
 	struct kunit_resource *resource;
 
-	if (likely(!current->kunit_test))
+	if (likely(!kunit_get_current_test()))
 		return false;
 
-	resource = kunit_find_named_resource(current->kunit_test, "slab_errors");
+	resource = kunit_find_named_resource(kunit_get_current_test(), "slab_errors");
 	if (!resource)
 		return false;
 
-- 
2.38.0.135.g90850a2211-goog

Re: [PATCH v2 3/3] mm: slub: test: Use the kunit_get_current_test() function

[Vlastimil Babka]

On 10/25/22 09:19, David Gow wrote:
> Use the newly-added function kunit_get_current_test() instead of
> accessing current->kunit_test directly. This function uses a static key
> to return more quickly when KUnit is enabled, but no tests are actively
> running. There should therefore be a negligible performance impact to
> enabling the slub KUnit tests.
> 
> Other than the performance improvement, this should be a no-op.
> 
> Cc: Oliver Glitta <glittao@gmail.com>
> Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: David Gow <davidgow@google.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
> 
> This is intended as an example use of the new function. Other users
> (such as KASAN) will be updated separately, as there would otherwise be
> conflicts.
> 
> Assuming there are no objections, we'll take this whole series via the
> kselftest/kunit tree.

OK, please do.

Some possible improvements below:

> There was no v1 of this patch. v1 of the series can be found here:
> https://lore.kernel.org/linux-kselftest/20221021072854.333010-1-davidgow@google.com/T/#u
> 
> ---
>  lib/slub_kunit.c | 1 +
>  mm/slub.c        | 5 +++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
> index 7a0564d7cb7a..8fd19c8301ad 100644
> --- a/lib/slub_kunit.c
> +++ b/lib/slub_kunit.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <kunit/test.h>
> +#include <kunit/test-bug.h>
>  #include <linux/mm.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> diff --git a/mm/slub.c b/mm/slub.c
> index 157527d7101b..15d10d250ef2 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -39,6 +39,7 @@
>  #include <linux/memcontrol.h>
>  #include <linux/random.h>
>  #include <kunit/test.h>
> +#include <kunit/test-bug.h>
>  #include <linux/sort.h>
>  
>  #include <linux/debugfs.h>
> @@ -603,10 +604,10 @@ static bool slab_add_kunit_errors(void)
>  {
>  	struct kunit_resource *resource;
>  
> -	if (likely(!current->kunit_test))
> +	if (likely(!kunit_get_current_test()))

Given that kunit_get_current_test() is basically an inline
!static_branch_unlikely(), IMHO the likely() here doesn't add anything and
could be removed?

>  		return false;
>  
> -	resource = kunit_find_named_resource(current->kunit_test, "slab_errors");
> +	resource = kunit_find_named_resource(kunit_get_current_test(), "slab_errors");

We just passed kunit_get_current_test() above so maybe we could just keep
using current->kunit_test here? Seems unnecessary adding another jump label.

>  	if (!resource)
>  		return false;
>  

Re: [PATCH v2 3/3] mm: slub: test: Use the kunit_get_current_test() function

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2255 bytes --]

Hi David,

I love your patch! Yet something to improve:

[auto build test ERROR on vbabka-slab/for-next]
[also build test ERROR on kees/for-next/pstore linus/master v6.1-rc2 next-20221025]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Gow/kunit-Provide-a-static-key-to-check-if-KUnit-is-actively-running-tests/20221025-152023
base:   git://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git for-next
patch link:    https://lore.kernel.org/r/20221025071907.1251820-3-davidgow%40google.com
patch subject: [PATCH v2 3/3] mm: slub: test: Use the kunit_get_current_test() function
config: s390-defconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/048f50673812037a89a222fd04beaeaa59a2c2bb
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review David-Gow/kunit-Provide-a-static-key-to-check-if-KUnit-is-actively-running-tests/20221025-152023
        git checkout 048f50673812037a89a222fd04beaeaa59a2c2bb
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> s390-linux-ld: mm/slub.o:(__jump_table+0xc8): undefined reference to `kunit_running'
   s390-linux-ld: mm/slub.o: `kunit_running' non-PLT reloc for symbol defined in shared library and accessed from executable (rebuild file with -fPIC ?)
   s390-linux-ld: final link failed: bad value
   pahole: .tmp_vmlinux.btf: No such file or directory
   .btf.vmlinux.bin.o: file not recognized: file format not recognized

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21450 bytes --]

Re: [PATCH v2 3/3] mm: slub: test: Use the kunit_get_current_test() function

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]

Hi David,

I love your patch! Yet something to improve:

[auto build test ERROR on vbabka-slab/for-next]
[also build test ERROR on kees/for-next/pstore linus/master v6.1-rc2 next-20221025]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Gow/kunit-Provide-a-static-key-to-check-if-KUnit-is-actively-running-tests/20221025-152023
base:   git://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git for-next
patch link:    https://lore.kernel.org/r/20221025071907.1251820-3-davidgow%40google.com
patch subject: [PATCH v2 3/3] mm: slub: test: Use the kunit_get_current_test() function
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/048f50673812037a89a222fd04beaeaa59a2c2bb
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review David-Gow/kunit-Provide-a-static-key-to-check-if-KUnit-is-actively-running-tests/20221025-152023
        git checkout 048f50673812037a89a222fd04beaeaa59a2c2bb
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   m68k-linux-ld: mm/slub.o: in function `slab_add_kunit_errors':
>> slub.c:(.text+0x1262): undefined reference to `kunit_running'

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65896 bytes --]