Re: [PATCH v2 4/4] rv/reactors: add KUnit tests for reactor registration and dispatch

On Mon, 2026-08-03 at 02:43 +0800, [email protected] wrote:
> From: Wen Yang <[email protected]>
>=20
> 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_SP=
IN
> lockdep context).=C2=A0 The Kconfig entry is tristate so the tests can be=
 built
> as a module when CONFIG_KUNIT=3Dm; only RV_REACTORS is required.
>=20
> Signed-off-by: Wen Yang <[email protected]>
> ---
> =C2=A0kernel/trace/rv/Kconfig=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 12 +++
> =C2=A0kernel/trace/rv/Makefile=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 1 +
> =C2=A0kernel/trace/rv/rv_reactors_kunit.c | 143 +++++++++++++++++++++++++=
+++
> =C2=A03 files changed, 156 insertions(+)
> =C2=A0create mode 100644 kernel/trace/rv/rv_reactors_kunit.c
>=20
> 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
> =C2=A0=09=C2=A0 Enables the panic reactor. The panic reactor emits a prin=
tk()
> =C2=A0=09=C2=A0 message if an exception is found and panic()s the system.
> =C2=A0
> +config RV_REACTORS_KUNIT
> +=09tristate "KUnit tests for RV reactors" if !KUNIT_ALL_TESTS
> +=09depends on KUNIT
> +=09depends on RV_REACTORS
> +=09default KUNIT_ALL_TESTS
> +=09help
> +=09=C2=A0 Enable KUnit tests for RV reactor registration and dispatch.
> +=09=C2=A0 These tests verify the register/unregister lifecycle, duplicat=
e
> +=09=C2=A0 rejection, and that rv_react() correctly invokes callbacks.
> +
> +=09=C2=A0 If unsure, say N.
> +
> =C2=A0config RV_MONITORS_KUNIT_TEST
> =C2=A0=09tristate "KUnit tests for RV monitors" if !KUNIT_ALL_TESTS
> =C2=A0=09depends 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) +=3D monitors/wakeup/wakeup=
.o
> =C2=A0obj-$(CONFIG_RV_REACTORS) +=3D rv_reactors.o
> =C2=A0obj-$(CONFIG_RV_REACT_PRINTK) +=3D reactor_printk.o
> =C2=A0obj-$(CONFIG_RV_REACT_PANIC) +=3D reactor_panic.o
> +obj-$(CONFIG_RV_REACTORS_KUNIT) +=3D rv_reactors_kunit.o
> =C2=A0obj-$(CONFIG_RV_MONITORS_KUNIT_TEST) +=3D 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 =3D {
> +=09.name=09=09=3D "kunit_test_reactor",
> +=09.description=09=3D "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 yo=
u
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 reac=
tors
list are still static, so we probably wouldn't gain a huge deal.

What do you think?

> +
> +static int unregister_test_reactor(void)
> +{
> +=09int ret =3D 0;
> +
> +=09if (test_reactor_registered) {
> +=09=09ret =3D rv_unregister_reactor(&test_reactor);
> +=09=09test_reactor_registered =3D 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 fai=
l
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.

> +=09}
> +
> +=09return 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)
> +{
> +=09unregister_test_reactor();
> +}
> +
> +static void register_test_reactor(struct kunit *test)
> +{
> +=09KUNIT_ASSERT_EQ(test, rv_register_reactor(&test_reactor), 0);
> +=09test_reactor_registered =3D true;
> +=09KUNIT_ASSERT_EQ(test,
> +=09=09=09kunit_add_action_or_reset(test, reactor_teardown,
> NULL), 0);
> +}
> +
> +static void test_register_unregister(struct kunit *test)
> +{
> +=09register_test_reactor(test);
> +
> +=09KUNIT_EXPECT_EQ(test, unregister_test_reactor(), 0);
> +}
> +
> +static void test_double_register(struct kunit *test)
> +{
> +=09register_test_reactor(test);
> +
> +=09KUNIT_EXPECT_EQ(test, rv_register_reactor(&test_reactor), -EINVAL);
> +
> +=09KUNIT_EXPECT_EQ(test, unregister_test_reactor(), 0);
> +}
> +
> +static void test_name_too_long(struct kunit *test)
> +{
> +=09/* Name length of MAX_RV_REACTOR_NAME_SIZE (32) must be rejected. */
> +=09static struct rv_reactor long_reactor =3D {
> +=09=09.name =3D "kunit_reactor_name_too_long_xxx_",
> +=09};
> +
> +=09KUNIT_ASSERT_EQ(test, (int)strlen(long_reactor.name),
> +=09=09=09MAX_RV_REACTOR_NAME_SIZE);

You probably want KUNIT_ASSERT_GE, or even better
_Static_assert(sizeof(long_name) - 1 >=3D MAX_RV_REACTOR_NAME_SIZE)

(you'd need to save the name to a static array first for sizeof() to work a=
s
expected)

> +=09KUNIT_EXPECT_EQ(test, rv_register_reactor(&long_reactor), -EINVAL);
> +}
> +
> +static struct kunit_case rv_reactor_registration_cases[] =3D {
> +=09KUNIT_CASE(test_register_unregister),
> +=09KUNIT_CASE(test_double_register),
> +=09KUNIT_CASE(test_name_too_long),
> +=09{}
> +};
> +
> +static struct kunit_suite rv_reactor_registration_suite =3D {
> +=09.name=09=09=3D "rv_reactor_registration",
> +=09.test_cases=09=3D rv_reactor_registration_cases,
> +};
> +
> +static atomic_t react_call_count;
> +
> +__printf(1, 0) static void mock_react(const char *msg, va_list args)
> +{
> +=09atomic_inc(&react_call_count);
> +=09/*
> +=09 * Hold the CPU for 5 ms so a timer interrupt is likely to fire
> +=09 * inside rv_react()'s lockdep context, exercising the LD_WAIT_SPIN
> +=09 * constraint.=C2=A0 mdelay() is a calibrated busy-wait with no sched=
uler
> +=09 * interaction.
> +=09 */
> +=09mdelay(5);
> +}
> +
> +static void test_react_no_callback(struct kunit *test)
> +{
> +=09struct rv_monitor monitor =3D {
> +=09=09.name =3D "kunit_null_react",
> +=09};
> +
> +=09/* rv_react() must silently return when monitor->react is NULL. */
> +=09rv_react(&monitor, "no callback");

So what are we testing here? That the kernel doesn't panic? It's fair to ex=
pect
a silent return, but we cannot really validate that.

If that's your intent, maybe specify it better, since, in fact, KUnit canno=
t
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)
> +{
> +=09struct rv_monitor monitor =3D {
> +=09=09.name=09=3D "kunit_dispatch_monitor",
> +=09=09.react=09=3D mock_react,
> +=09};
> +
> +=09atomic_set(&react_call_count, 0);
> +=09rv_react(&monitor, "callback invocation test");
> +=09KUNIT_EXPECT_EQ(test, atomic_read(&react_call_count), 1);
> +}
> +
> +static struct kunit_case rv_react_dispatch_cases[] =3D {
> +=09KUNIT_CASE(test_react_no_callback),
> +=09KUNIT_CASE(test_react_callback_invoked),
> +=09{}
> +};
> +
> +static struct kunit_suite rv_react_dispatch_suite =3D {
> +=09.name=09=09=3D "rv_react_dispatch",
> +=09.test_cases=09=3D rv_react_dispatch_cases,
> +};
> +
> +kunit_test_suites(&rv_reactor_registration_suite, &rv_react_dispatch_sui=
te);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("KUnit tests for RV reactor registration and dispatch=
");