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

[Wen Yang <wen.yang@linux.dev>]

On 8/3/26 20:49, Gabriele Monaco wrote:
> On Mon, 2026-08-03 at 02:43 +0800, wen.yang@linux.dev wrote:
>> From: Wen Yang <wen.yang@linux.dev>
>>
>> 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 <wen.yang@linux.dev>
>> ---
>>   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");
>