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?
> +
> +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.
> + }
> +
> + 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)
> + 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"
Thanks,
Gabriele
> +}
> +
> +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");
