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

[PATCH v3 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.

No changes since v2:
https://lore.kernel.org/all/20221025071907.1251820-1-davidgow@google.com/

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 d7f60e8aab30..b948c32a7b6b 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.1.584.g0f3c55d4c2-goog

[PATCH v3 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 (or m), 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.

Note that kunit_get_current_test() does not work if KUnit is built as a
module. This mirrors the existing restriction on kunit_fail_current_test().

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 v2:
https://lore.kernel.org/all/20221025071907.1251820-2-davidgow@google.com/
- Only add kunit_get_current_test() when KUnit is built-in, as the
  static key isn't available otherwise.
  - I'm going to try to put together some patches to make things like
    this available when CONFIG_KUNIT=m in the future.
  - Also update the documentation to note this.

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 | 25 +++++++-----
 include/kunit/test-bug.h                | 53 +++++++++++++++++++++++--
 2 files changed, 66 insertions(+), 12 deletions(-)

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 2737863ef365..e70014b82350 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -625,17 +625,21 @@ 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`` 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, in which case 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 +648,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 +679,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 +707,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.
diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
index 5fc58081d511..87a953dceeaa 100644
--- a/include/kunit/test-bug.h
+++ b/include/kunit/test-bug.h
@@ -9,16 +9,63 @@
 #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 */
+#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;
+}
+
+
+/**
+ * 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
 
+static inline struct kunit *kunit_get_current_test(void) { return NULL; }
+
+/* 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.1.584.g0f3c55d4c2-goog

Re: [PATCH v3 2/3] kunit: Use the static key when retrieving the current test

[Daniel Latypov]

On Sat, Nov 19, 2022 at 12:13 AM 'David Gow' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> 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 (or m), 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.
>
> Note that kunit_get_current_test() does not work if KUnit is built as a
> module. This mirrors the existing restriction on kunit_fail_current_test().
>
> 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>

Reviewed-by: Daniel Latypov <dlatypov@google.com>

Looks good to me, but some small optional nits about the Documentation.

I'm a bit sad that the kunit_fail_current_test() macro is now a bit
more complicated (has two definitions).
Optional: perhaps it's long enough now that we should have a comment
after the #endif, i.e.
#endif   /* IS_BUILTIN(CONFIG_KUNIT) */

<snip>

>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index 2737863ef365..e70014b82350 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -625,17 +625,21 @@ 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

nit: there are two spaces after "for example, "

Personal preference: I'd rather keep "For example," as the start of a
new sentence.

> +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``.

Personal preference: kunit_get_current_test()
IMO that would make it easier to pick up when the reader is quickly
scanning over.

>
> -The example below includes how to implement "mocking":
> +``kunit_get_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, in which case it will quickly return ``NULL``.

I find this sentence a bit confusing.

I think it's trying to convey that
* it can be called no matter how the kernel is built or what cmdline
args are given
* but it doesn't work properly for CONFIG_KUNIT=m
* for CONFIG_KUNIT=n, it's a static inline func that just returns NULL
* when booting with `kunit.enabled=0`, it's fast (thanks to static keys)

But the current wording basically says "the func requires
CONFIG_KUNIT=y" then says it's safe to call it even if CONFIG_KUNIT=n.
It feels a bit whiplashy.

Should this be reworded to say the function can be used regardless of
whether KUnit is enabled but add a `note` block about how it doesn't
properly for CONFIG_KUNIT=m?

> +
> +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 +648,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 +679,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.

Oops, thanks for cleaning this up.
I guess I forgot to update this when splitting out resource.rst or my
change raced with the rewrite of this file.

>
>  Failing The Current Test
>  ------------------------
> @@ -703,3 +707,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.

This is the same wording as above.
I think it's clearer since what it's trying to convey is simpler, so
it's probably fine.

But if we do end up thinking of a good way to reword the previous bit,
we might want to reword it here too.

Re: [PATCH v3 2/3] kunit: Use the static key when retrieving the current test

[David Gow]

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

On Tue, Nov 22, 2022 at 10:21 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Sat, Nov 19, 2022 at 12:13 AM 'David Gow' via KUnit Development
> <kunit-dev@googlegroups.com> wrote:
> >
> > 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 (or m), 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.
> >
> > Note that kunit_get_current_test() does not work if KUnit is built as a
> > module. This mirrors the existing restriction on kunit_fail_current_test().
> >
> > 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>
>
> Reviewed-by: Daniel Latypov <dlatypov@google.com>
>
> Looks good to me, but some small optional nits about the Documentation.
>
> I'm a bit sad that the kunit_fail_current_test() macro is now a bit
> more complicated (has two definitions).

I'm not too happy with it either, but I think it's worth having the
printf() format string checking, as well as making it possible to
optimise the call out (without needing LTO), and I can't think of a
better way of doing that at the moment.

The only other option I can think of would be to have lots of #ifdefs
for the _contents_ of the functions, and that seemed more ugly to me.

> Optional: perhaps it's long enough now that we should have a comment
> after the #endif, i.e.
> #endif   /* IS_BUILTIN(CONFIG_KUNIT) */
>

Makes sense to me. Will add in v3.

> <snip>
>
> >
> > diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> > index 2737863ef365..e70014b82350 100644
> > --- a/Documentation/dev-tools/kunit/usage.rst
> > +++ b/Documentation/dev-tools/kunit/usage.rst
> > @@ -625,17 +625,21 @@ 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
>
> nit: there are two spaces after "for example, "
>
> Personal preference: I'd rather keep "For example," as the start of a
> new sentence.
>
> > +any current test from within an error handler.


Hmm... I found it a bit ugly to keep "For example" at the start of the
sentence, as we then have to stick a (possibly duplicated) verb in to
make it actually a sentence.

How about:
In some cases, we need to call test-only code from outside the test
file. For example, this is useful when providing a fake implementation
of a function, or if we wish to fail the 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``.
>
> Personal preference: kunit_get_current_test()
> IMO that would make it easier to pick up when the reader is quickly
> scanning over.
>

Agreed, will fix in v3.

> >
> > -The example below includes how to implement "mocking":
> > +``kunit_get_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, in which case it will quickly return ``NULL``.
>
> I find this sentence a bit confusing.
>
> I think it's trying to convey that
> * it can be called no matter how the kernel is built or what cmdline
> args are given
> * but it doesn't work properly for CONFIG_KUNIT=m
> * for CONFIG_KUNIT=n, it's a static inline func that just returns NULL
> * when booting with `kunit.enabled=0`, it's fast (thanks to static keys)
>

Yeah: that's the goal.

> But the current wording basically says "the func requires
> CONFIG_KUNIT=y" then says it's safe to call it even if CONFIG_KUNIT=n.
> It feels a bit whiplashy.
>
> Should this be reworded to say the function can be used regardless of
> whether KUnit is enabled but add a `note` block about how it doesn't
> properly for CONFIG_KUNIT=m?
>

How about:
``kunit_get_current_test()`` is safe to call even if KUnit is not
enabled. If KUnit is not enabled (or was built as a module), or no
test is running, it will return NULL.

Or:
``kunit_get_current_test()`` is always available, but will only return
a test if KUnit is built-in to the kernel (i.e, CONFIG_KUNIT=y). In
all other cases, it will return NULL.

We could add a:
This will compile to either a no-op or a static key, so will have
negligible performance impact when no test is running.

Thoughts?

Regardless, the plan is to eventually get rid of the restriction with
modules, so hopefully that part of the awkwardness won't last too
long.

> > +
> > +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 +648,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 +679,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.
>
> Oops, thanks for cleaning this up.
> I guess I forgot to update this when splitting out resource.rst or my
> change raced with the rewrite of this file.
>
> >
> >  Failing The Current Test
> >  ------------------------
> > @@ -703,3 +707,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.
>
> This is the same wording as above.
> I think it's clearer since what it's trying to convey is simpler, so
> it's probably fine.
>
> But if we do end up thinking of a good way to reword the previous bit,
> we might want to reword it here too.

Yeah: I wrote this one first, then copied it above, so that's why this
one is a bit simpler. If we come up with something better for the
first one, we can keep it.

_Maybe_ if we moved things to a .. note block, then we could share
that between both of these sections, though that has its own issues.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v3 2/3] kunit: Use the static key when retrieving the current test

[Daniel Latypov]

On Mon, Nov 21, 2022 at 7:16 PM David Gow <davidgow@google.com> wrote:
> > Reviewed-by: Daniel Latypov <dlatypov@google.com>
> >
> > Looks good to me, but some small optional nits about the Documentation.
> >
> > I'm a bit sad that the kunit_fail_current_test() macro is now a bit
> > more complicated (has two definitions).
>
> I'm not too happy with it either, but I think it's worth having the
> printf() format string checking, as well as making it possible to
> optimise the call out (without needing LTO), and I can't think of a
> better way of doing that at the moment.
>
> The only other option I can think of would be to have lots of #ifdefs
> for the _contents_ of the functions, and that seemed more ugly to me.

Sorry, I should have been more clear.
I'm fine with it as-is.

It's just a bit sad that it could have remained a single definition,
but that would sacrifice performance.
The version in this patch can avoid the call to
__kunit_fail_current_test() via static key, so that's more important.

>
> > Optional: perhaps it's long enough now that we should have a comment
> > after the #endif, i.e.
> > #endif   /* IS_BUILTIN(CONFIG_KUNIT) */
> >
>
> Makes sense to me. Will add in v3.
>
> > <snip>
> >
> > >
> > > diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> > > index 2737863ef365..e70014b82350 100644
> > > --- a/Documentation/dev-tools/kunit/usage.rst
> > > +++ b/Documentation/dev-tools/kunit/usage.rst
> > > @@ -625,17 +625,21 @@ 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
> >
> > nit: there are two spaces after "for example, "
> >
> > Personal preference: I'd rather keep "For example," as the start of a
> > new sentence.
> >
> > > +any current test from within an error handler.
>
>
> Hmm... I found it a bit ugly to keep "For example" at the start of the
> sentence, as we then have to stick a (possibly duplicated) verb in to
> make it actually a sentence.
>
> How about:
> In some cases, we need to call test-only code from outside the test
> file. For example, this is useful when providing a fake implementation
> of a function, or if we wish to fail the current test from within an
> error handler.

I see what you mean. The initial wording is good as-is, I think.
I thought I had some ideas of how to reword it, but they don't sound
so good when I actually write them out.

>
>
> > > +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``.
> >
> > Personal preference: kunit_get_current_test()
> > IMO that would make it easier to pick up when the reader is quickly
> > scanning over.
> >
>
> Agreed, will fix in v3.
>
> > >
> > > -The example below includes how to implement "mocking":
> > > +``kunit_get_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, in which case it will quickly return ``NULL``.
> >
> > I find this sentence a bit confusing.
> >
> > I think it's trying to convey that
> > * it can be called no matter how the kernel is built or what cmdline
> > args are given
> > * but it doesn't work properly for CONFIG_KUNIT=m
> > * for CONFIG_KUNIT=n, it's a static inline func that just returns NULL
> > * when booting with `kunit.enabled=0`, it's fast (thanks to static keys)
> >
>
> Yeah: that's the goal.
>
> > But the current wording basically says "the func requires
> > CONFIG_KUNIT=y" then says it's safe to call it even if CONFIG_KUNIT=n.
> > It feels a bit whiplashy.
> >
> > Should this be reworded to say the function can be used regardless of
> > whether KUnit is enabled but add a `note` block about how it doesn't
> > properly for CONFIG_KUNIT=m?
> >
>
> How about:
> ``kunit_get_current_test()`` is safe to call even if KUnit is not
> enabled. If KUnit is not enabled (or was built as a module), or no
> test is running, it will return NULL.
>
> Or:
> ``kunit_get_current_test()`` is always available, but will only return
> a test if KUnit is built-in to the kernel (i.e, CONFIG_KUNIT=y). In
> all other cases, it will return NULL.
>
> We could add a:
> This will compile to either a no-op or a static key, so will have

*static key check?

> negligible performance impact when no test is running.
>
> Thoughts?
>
> Regardless, the plan is to eventually get rid of the restriction with
> modules, so hopefully that part of the awkwardness won't last too
> long.

I think both of these work, w/ a slight preference to the first.
I think it more clearly explains how the function behaves, even if the
gotcha "this function won't do what you expect with moduels" is not
immediately apparent. But hopefully we can fix that soon so this
becomes a moot point.
I also think it works better for the section down below about
kunit_fail_current_test().

Up to you if you want to include the bit about the static key.
I can see arguments either way.

Daniel

>
> > > +
> > > +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 +648,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 +679,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.
> >
> > Oops, thanks for cleaning this up.
> > I guess I forgot to update this when splitting out resource.rst or my
> > change raced with the rewrite of this file.
> >
> > >
> > >  Failing The Current Test
> > >  ------------------------
> > > @@ -703,3 +707,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.
> >
> > This is the same wording as above.
> > I think it's clearer since what it's trying to convey is simpler, so
> > it's probably fine.
> >
> > But if we do end up thinking of a good way to reword the previous bit,
> > we might want to reword it here too.
>
> Yeah: I wrote this one first, then copied it above, so that's why this
> one is a bit simpler. If we come up with something better for the
> first one, we can keep it.
>
> _Maybe_ if we moved things to a .. note block, then we could share
> that between both of these sections, though that has its own issues.

Re: [PATCH v3 2/3] kunit: Use the static key when retrieving the current test

[Sadiya Kazi]

Thank you, David. This looks fine to me but I do have a few comments
for the documentation. Please see my comments inline below.
Additionally, it would be great to use second person, but we can
reserve that change for another time.

Best Regards,
Sadiya




On Sat, Nov 19, 2022 at 1:43 PM 'David Gow' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> 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 (or m), 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.
>
> Note that kunit_get_current_test() does not work if KUnit is built as a
> module. This mirrors the existing restriction on kunit_fail_current_test().
>
> 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 v2:
> https://lore.kernel.org/all/20221025071907.1251820-2-davidgow@google.com/
> - Only add kunit_get_current_test() when KUnit is built-in, as the
>   static key isn't available otherwise.
>   - I'm going to try to put together some patches to make things like
>     this available when CONFIG_KUNIT=m in the future.
>   - Also update the documentation to note this.
>
> 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 | 25 +++++++-----
>  include/kunit/test-bug.h                | 53 +++++++++++++++++++++++--
>  2 files changed, 66 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index 2737863ef365..e70014b82350 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -625,17 +625,21 @@ 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``.
>
How about this:
In some cases, we need to call test-only code from outside the test
file. This is helpful, for instance, when providing a fake implementation
of a function, or if we wish to fail the current test from within an
error handler.

> -The example below includes how to implement "mocking":
> +``kunit_get_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, in which case it will quickly return ``NULL``.

Suggestion:
Although the function ``kunit get current test()`` is always
available, it will only
produce a test if the kernel includes KUnit (i.e., if CONFIG KUNIT=y). It will
return NULL in all other circumstances.
> +
> +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 +648,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 +679,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 +707,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.

same as above

> diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
> index 5fc58081d511..87a953dceeaa 100644
> --- a/include/kunit/test-bug.h
> +++ b/include/kunit/test-bug.h
> @@ -9,16 +9,63 @@
>  #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 */
> +#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.
Suggestion: You can use "currently running KUnit test" or just say
"current 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.

How about this:
Returns a pointer to the associated struct kunit if a KUnit test is
currently running
in the task. This pointer can then be passed to any KUnit function or assertion.
Returns NULL if no tests are running (or tests are running in a different task).

> + *
> + * 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.
> + */

How about this: You can safely call this function even when KUnit is disabled.
If CONFIG_KUNIT is not enabled, it will compile to nothing and will
run quickly if no tests are running.



> +static inline struct kunit *kunit_get_current_test(void)
> +{
> +       if (!static_branch_unlikely(&kunit_running))
> +               return NULL;
> +
> +       return current->kunit_test;
> +}
> +
> +
> +/**
> + * 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
>
> +static inline struct kunit *kunit_get_current_test(void) { return NULL; }
> +
> +/* 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.1.584.g0f3c55d4c2-goog
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20221119081252.3864249-2-davidgow%40google.com.

[PATCH v3 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>
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.

We'll take this whole series via the kselftest/kunit tree.

Changes since v2:
https://lore.kernel.org/all/20221025071907.1251820-3-davidgow@google.com/
- Get rid of a redundant 'likely' (Thanks Vlastimil Babka)
- Use current->kunit_test directly when we already know a test is
  running. (Thanks Vlastimil Babka)
- Add Vlastimil's Acked-by.

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

Cheers,
-- David

---
 lib/slub_kunit.c | 1 +
 mm/slub.c        | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

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..1887996cb703 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,7 +604,7 @@ static bool slab_add_kunit_errors(void)
 {
 	struct kunit_resource *resource;
 
-	if (likely(!current->kunit_test))
+	if (!kunit_get_current_test())
 		return false;
 
 	resource = kunit_find_named_resource(current->kunit_test, "slab_errors");
-- 
2.38.1.584.g0f3c55d4c2-goog

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

[Hyeonggon Yoo]

On Sat, Nov 19, 2022 at 04:12:52PM +0800, 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>

Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
with small comment:

> ---
> 
> 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.
> 
> We'll take this whole series via the kselftest/kunit tree.
> 
> Changes since v2:
> https://lore.kernel.org/all/20221025071907.1251820-3-davidgow@google.com/
> - Get rid of a redundant 'likely' (Thanks Vlastimil Babka)
> - Use current->kunit_test directly when we already know a test is
>   running. (Thanks Vlastimil Babka)
> - Add Vlastimil's Acked-by.
> 
> 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
> 
> Cheers,
> -- David
> 
> ---
>  lib/slub_kunit.c | 1 +
>  mm/slub.c        | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> 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>

Is this #include needed in slub_kunit.c?

>  #include <linux/mm.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> diff --git a/mm/slub.c b/mm/slub.c
> index 157527d7101b..1887996cb703 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,7 +604,7 @@ static bool slab_add_kunit_errors(void)
>  {
>  	struct kunit_resource *resource;
>  
> -	if (likely(!current->kunit_test))
> +	if (!kunit_get_current_test())
>  		return false;
>  
>  	resource = kunit_find_named_resource(current->kunit_test, "slab_errors");
> -- 
> 2.38.1.584.g0f3c55d4c2-goog
> 

-- 
Thanks,
Hyeonggon

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

[David Gow]

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

On Thu, Nov 24, 2022 at 6:43 PM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
>
> On Sat, Nov 19, 2022 at 04:12:52PM +0800, 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>
>
> Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> with small comment:
>

Thanks very much!

> > ---
> >
> > 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.
> >
> > We'll take this whole series via the kselftest/kunit tree.
> >
> > Changes since v2:
> > https://lore.kernel.org/all/20221025071907.1251820-3-davidgow@google.com/
> > - Get rid of a redundant 'likely' (Thanks Vlastimil Babka)
> > - Use current->kunit_test directly when we already know a test is
> >   running. (Thanks Vlastimil Babka)
> > - Add Vlastimil's Acked-by.
> >
> > 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
> >
> > Cheers,
> > -- David
> >
> > ---
> >  lib/slub_kunit.c | 1 +
> >  mm/slub.c        | 3 ++-
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > 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>
>
> Is this #include needed in slub_kunit.c?
>

Yes: kunit_get_current_test() is defined in test-bug.h.

This header contains functions which are always available, even when
KUnit is not enabled.

(It's name isn't great: we may rename/refactor it down the line...)

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

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

[Daniel Latypov]

On Sat, Nov 19, 2022 at 12:13 AM David Gow <davidgow@google.com> wrote:
>
> 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>

Reviewed-by: Daniel Latypov <dlatypov@google.com>

I didn't know anything about the static key support in the kernel
before this patch.
But from what I read and saw of other uses, this looks good to me.

One small question/nit about how we declare the key below.

<snip>

> +/* Static key: true if any KUnit tests are currently running */
> +extern struct static_key_false kunit_running;

Is there any documented preference between this and
  DECLARE_STATIC_KEY_FALSE(kunit_running);
?

I see 89 instances of this macro and 45 of `extern struct static_key_false`.
So I'd vote for the macro since it seems like the newer approach and
more common.

Daniel

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

[David Gow]

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

On Tue, Nov 22, 2022 at 9:31 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Sat, Nov 19, 2022 at 12:13 AM David Gow <davidgow@google.com> wrote:
> >
> > 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>
>
> Reviewed-by: Daniel Latypov <dlatypov@google.com>
>
> I didn't know anything about the static key support in the kernel
> before this patch.
> But from what I read and saw of other uses, this looks good to me.
>
> One small question/nit about how we declare the key below.
>
> <snip>
>
> > +/* Static key: true if any KUnit tests are currently running */
> > +extern struct static_key_false kunit_running;
>
> Is there any documented preference between this and
>   DECLARE_STATIC_KEY_FALSE(kunit_running);
> ?
>
> I see 89 instances of this macro and 45 of `extern struct static_key_false`.
> So I'd vote for the macro since it seems like the newer approach and
> more common.
>

Yeah, there was no particular reason I put 'extern struct
static_key_false'. I'll change it to DECLARE_STATIC_KEY_FALSE in v3.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]