On 8/3/26 20:49, Gabriele Monaco wrote:
On Mon, 2026-08-03 at 02:43 +0800, [email protected] wrote:
From: Wen Yang <[email protected]>
Add KUnit tests covering the reactor register/unregister lifecycle
(including duplicate and name-length rejection) and rv_react() dispatch
(a no-op without a callback, exactly one invocation with one; the mdelay
callback keeps the CPU busy so a timer interrupt exercises the LD_WAIT_SPIN
lockdep context). The Kconfig entry is tristate so the tests can be built
as a module when CONFIG_KUNIT=m; only RV_REACTORS is required.
Signed-off-by: Wen Yang <[email protected]>
---
kernel/trace/rv/Kconfig | 12 +++
kernel/trace/rv/Makefile | 1 +
kernel/trace/rv/rv_reactors_kunit.c | 143 ++++++++++++++++++++++++++++
3 files changed, 156 insertions(+)
create mode 100644 kernel/trace/rv/rv_reactors_kunit.c
diff --git a/kernel/trace/rv/Kconfig b/kernel/trace/rv/Kconfig
index efa930f94ea4..9bfd429ffdea 100644
--- a/kernel/trace/rv/Kconfig
+++ b/kernel/trace/rv/Kconfig
@@ -113,6 +113,18 @@ config RV_REACT_PANIC
Enables the panic reactor. The panic reactor emits a printk()
message if an exception is found and panic()s the system.
+config RV_REACTORS_KUNIT
+ tristate "KUnit tests for RV reactors" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ depends on RV_REACTORS
+ default KUNIT_ALL_TESTS
+ help
+ Enable KUnit tests for RV reactor registration and dispatch.
+ These tests verify the register/unregister lifecycle, duplicate
+ rejection, and that rv_react() correctly invokes callbacks.
+
+ If unsure, say N.
+
config RV_MONITORS_KUNIT_TEST
tristate "KUnit tests for RV monitors" if !KUNIT_ALL_TESTS
depends on KUNIT && RV && RV_REACTORS
diff --git a/kernel/trace/rv/Makefile b/kernel/trace/rv/Makefile
index cdbf68c84f5a..c895d81dfdad 100644
--- a/kernel/trace/rv/Makefile
+++ b/kernel/trace/rv/Makefile
@@ -25,4 +25,5 @@ obj-$(CONFIG_RV_MON_WAKEUP) += monitors/wakeup/wakeup.o
obj-$(CONFIG_RV_REACTORS) += rv_reactors.o
obj-$(CONFIG_RV_REACT_PRINTK) += reactor_printk.o
obj-$(CONFIG_RV_REACT_PANIC) += reactor_panic.o
+obj-$(CONFIG_RV_REACTORS_KUNIT) += rv_reactors_kunit.o
obj-$(CONFIG_RV_MONITORS_KUNIT_TEST) += rv_monitors_test.o
diff --git a/kernel/trace/rv/rv_reactors_kunit.c
b/kernel/trace/rv/rv_reactors_kunit.c
new file mode 100644
index 000000000000..32c25dcf0cb6
--- /dev/null
+++ b/kernel/trace/rv/rv_reactors_kunit.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit tests for RV reactor registration and dispatch.
+ */
+
+#include <kunit/test.h>
+#include <linux/rv.h>
+#include <linux/delay.h>
+#include <linux/atomic.h>
+#include "rv.h"
+
+static struct rv_reactor test_reactor = {
+ .name = "kunit_test_reactor",
+ .description = "KUnit test reactor",
+};
+
+/*
+ * rv_unregister_reactor() on a reactor that was never registered would
+ * list_del() an uninitialized list_head, so track registration and only
+ * unregister in the teardown if it is still on the list.
+ */
+static bool test_reactor_registered;
I'm not too fond of adding more logic than necessary in the test, but if you
really want to keep the dynamic teardown cannot this variable stay somehow in
test-specific memory (test->priv)?
That feels to me a bit cleaner, although the reactor and obviously the reactors
list are still static, so we probably wouldn't gain a huge deal.
What do you think?
Thansk, your suggestion in the next comment -- relying purely on
theteardown action. Since reactor_teardown() is only registered after
rv_register_reactor() succeeds, calling rv_unregister_reactor()
unconditionally inside it is always safe. This lets us drop
test_reactor_registered entirely, along with the
unregister_test_reactor() wrapper. See the updated code below.
+
+static int unregister_test_reactor(void)
+{
+ int ret = 0;
+
+ if (test_reactor_registered) {
+ ret = rv_unregister_reactor(&test_reactor);
+ test_reactor_registered = false;
rv_unregister_reactor() cannot fail and probably never will (I'm not quite sure
it should return int).
But setting test_reactor_registered to false here is assuming it didn't fail
while the rest of the test doesn't make this assumption.
Maybe let's make it void and stop tracking a result that can never change (and
that real reactors already have to ignore).
If we do that you may even stop unregistering manually and rely only on the
teardown, and perhaps get rid of test_reactor_registered. Even
test_register_unregister() would be mostly already covered by the next one.
Just throwing in the idea.
Agreed on all points. In v3:
- unregister_test_reactor() is gone; reactor_teardown() calls
rv_unregister_reactor() directly.
- test_reactor_registered is gone.
- test_register_unregister() is removed; the teardown covers
the unregister path, and test_double_register() already exercises
the register→verify→unregister (via teardown) flow.
The updated teardown is:
static void reactor_teardown(void *arg)
{
rv_unregister_reactor(&test_reactor);
}
static void register_test_reactor(struct kunit *test)
{
KUNIT_ASSERT_EQ(test, rv_register_reactor(&test_reactor), 0);
KUNIT_ASSERT_EQ(test,
kunit_add_action_or_reset(test,reactor_teardown,
NULL), 0);
}
+ }
+
+ return ret;
+}
+
+/*
+ * The teardown action guarantees the reactor is unregistered even if a
+ * test fails mid-way, so a leftover entry cannot corrupt later tests.
+ */
+static void reactor_teardown(void *arg)
+{
+ unregister_test_reactor();
+}
+
+static void register_test_reactor(struct kunit *test)
+{
+ KUNIT_ASSERT_EQ(test, rv_register_reactor(&test_reactor), 0);
+ test_reactor_registered = true;
+ KUNIT_ASSERT_EQ(test,
+ kunit_add_action_or_reset(test, reactor_teardown,
NULL), 0);
+}
+
+static void test_register_unregister(struct kunit *test)
+{
+ register_test_reactor(test);
+
+ KUNIT_EXPECT_EQ(test, unregister_test_reactor(), 0);
+}
+
+static void test_double_register(struct kunit *test)
+{
+ register_test_reactor(test);
+
+ KUNIT_EXPECT_EQ(test, rv_register_reactor(&test_reactor), -EINVAL);
+
+ KUNIT_EXPECT_EQ(test, unregister_test_reactor(), 0);
+}
+
+static void test_name_too_long(struct kunit *test)
+{
+ /* Name length of MAX_RV_REACTOR_NAME_SIZE (32) must be rejected. */
+ static struct rv_reactor long_reactor = {
+ .name = "kunit_reactor_name_too_long_xxx_",
+ };
+
+ KUNIT_ASSERT_EQ(test, (int)strlen(long_reactor.name),
+ MAX_RV_REACTOR_NAME_SIZE);
You probably want KUNIT_ASSERT_GE, or even better
_Static_assert(sizeof(long_name) - 1 >= MAX_RV_REACTOR_NAME_SIZE)
(you'd need to save the name to a static array first for sizeof() to work as
expected)
Good idea. The _Static_assert approach is strictly better: it catches
the mistake at build time rather than test run time, and the assertion
is always evaluated regardless of whether the test runs at all.
In v3 the name is stored in a fixed-size array:
static const char long_reactor_name[] = "kunit_reactor_name_too_long_xxx_";
_Static_assert(sizeof(long_reactor_name) - 1 >= MAX_RV_REACTOR_NAME_SIZE,
"long_reactor_name must be at least
MAX_RV_REACTOR_NAME_SIZE chars");
static void test_name_too_long(struct kunit *test)
{
static struct rv_reactor long_reactor = {
.name = long_reactor_name,
};
KUNIT_EXPECT_EQ(test, rv_register_reactor(&long_reactor), -EINVAL);
}
The KUNIT_ASSERT_EQ(strlen(...)) runtime check is removed.
+ KUNIT_EXPECT_EQ(test, rv_register_reactor(&long_reactor), -EINVAL);
+}
+
+static struct kunit_case rv_reactor_registration_cases[] = {
+ KUNIT_CASE(test_register_unregister),
+ KUNIT_CASE(test_double_register),
+ KUNIT_CASE(test_name_too_long),
+ {}
+};
+
+static struct kunit_suite rv_reactor_registration_suite = {
+ .name = "rv_reactor_registration",
+ .test_cases = rv_reactor_registration_cases,
+};
+
+static atomic_t react_call_count;
+
+__printf(1, 0) static void mock_react(const char *msg, va_list args)
+{
+ atomic_inc(&react_call_count);
+ /*
+ * Hold the CPU for 5 ms so a timer interrupt is likely to fire
+ * inside rv_react()'s lockdep context, exercising the LD_WAIT_SPIN
+ * constraint. mdelay() is a calibrated busy-wait with no scheduler
+ * interaction.
+ */
+ mdelay(5);
+}
+
+static void test_react_no_callback(struct kunit *test)
+{
+ struct rv_monitor monitor = {
+ .name = "kunit_null_react",
+ };
+
+ /* rv_react() must silently return when monitor->react is NULL. */
+ rv_react(&monitor, "no callback");
So what are we testing here? That the kernel doesn't panic? It's fair to expect
a silent return, but we cannot really validate that.
If that's your intent, maybe specify it better, since, in fact, KUnit cannot
validate this. Something like:
"The only possible failure in this test case is a kernel panic"
Good point. And thinking further about it: we actually can do better
than a pure crash guard here. The existing mock_react() already
increments react_call_count, so we can reuse that counter to assert that
the callback was never invoked. That turns the test from "does not
panic" into "callback count stays zero", which catches a broader class
of bugs.
In v3 the test becomes:
static void test_react_no_callback(struct kunit *test)
{
struct rv_monitor monitor = {
.name = "kunit_null_react",
};
atomic_set(&react_call_count, 0);
rv_react(&monitor, "no callback");
/*
* The only possible failure in this test case is a kernel panic.
* NULL react guard: callback must NOT have been invoked
*/
KUNIT_EXPECT_EQ(test, atomic_read(&react_call_count), 0);
}
Thanks,
--
Best wishes,
Wen
+}
+
+static void test_react_callback_invoked(struct kunit *test)
+{
+ struct rv_monitor monitor = {
+ .name = "kunit_dispatch_monitor",
+ .react = mock_react,
+ };
+
+ atomic_set(&react_call_count, 0);
+ rv_react(&monitor, "callback invocation test");
+ KUNIT_EXPECT_EQ(test, atomic_read(&react_call_count), 1);
+}
+
+static struct kunit_case rv_react_dispatch_cases[] = {
+ KUNIT_CASE(test_react_no_callback),
+ KUNIT_CASE(test_react_callback_invoked),
+ {}
+};
+
+static struct kunit_suite rv_react_dispatch_suite = {
+ .name = "rv_react_dispatch",
+ .test_cases = rv_react_dispatch_cases,
+};
+
+kunit_test_suites(&rv_reactor_registration_suite, &rv_react_dispatch_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("KUnit tests for RV reactor registration and dispatch");
