Re: [PATCH 09/10] rv: Add KUnit test for throttle monitor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] rv: flaky KUnit test in throttle monitor due to tick granularity
- [Low] rv: missing handle_exit callback in rv_throttle_ops API
--

commit 393b9dd1a461f953ed6e0221a5c4e2b0850aa791
Author: Gabriele Monaco <gmonaco@redhat.com>

rv: Add KUnit test for throttle monitor

Add a test case for the throttle monitor to validate reactions occur
when a task gets throttled on time but switched back in without replenish
or runs longer than its runtime.

> diff --git a/kernel/trace/rv/monitors/throttle/throttle.c b/kernel/trace/rv/monitors/throttle/throttle.c
> index b22a2abe59fe0..2b181c46b7b65 100644
> --- a/kernel/trace/rv/monitors/throttle/throttle.c
> +++ b/kernel/trace/rv/monitors/throttle/throttle.c
[ ... ]
> @@ -283,3 +283,20 @@ module_exit(unregister_throttle);
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Gabriele Monaco <gmonaco@redhat.com>");
>  MODULE_DESCRIPTION("throttle: throttle dl entities when they use up their runtime.");
> +
> +#if IS_ENABLED(CONFIG_RV_MONITORS_KUNIT_TEST)
> +#include <kunit/visibility.h>
> +#include "throttle_kunit.h"
> +
> +const struct rv_throttle_ops rv_throttle_ops = {
> +	.mon = RV_MON_OPS_INIT(),
> +	.handle_dl_replenish = handle_dl_replenish,
> +	.handle_dl_throttle = handle_dl_throttle,
> +	.handle_sched_enqueue = handle_sched_enqueue,
> +	.handle_sched_switch = handle_sched_switch,
> +	.handle_sys_enter = handle_sys_enter,
> +	.handle_newtask = handle_newtask,
> +	.handle_dl_server_stop = handle_dl_server_stop,
> +};

[Severity: Low]
Did you intentionally omit the handle_exit callback from the rv_throttle_ops
struct?

While teardown_test cleans up the hash table and prevents memory leaks during
tests, the missing function pointer prevents KUnit tests from manually
simulating the task exit cleanup lifecycle to verify state destruction when
a task dies.

> +EXPORT_SYMBOL_IF_KUNIT(rv_throttle_ops);
> +#endif
> diff --git a/kernel/trace/rv/monitors/throttle/throttle_kunit.c b/kernel/trace/rv/monitors/throttle/throttle_kunit.c
> new file mode 100644
> index 0000000000000..8194095dbab81
> --- /dev/null
> +++ b/kernel/trace/rv/monitors/throttle/throttle_kunit.c
[ ... ]
> +	/* Task runs longer than runtime */
> +	rv_throttle_ops.handle_sched_switch(NULL, 0, other, target, TASK_RUNNING);
> +	rv_throttle_ops.handle_dl_replenish(NULL, &target->dl, 0, DL_TASK);
> +	udelay(10 + TICK_USEC);
> +	rv_throttle_ops.handle_dl_throttle(NULL, &target->dl, 0, DL_TASK);
> +	RV_KUNIT_EXPECT_REACTION(test, ctx);

[Severity: Medium]
Will this delay calculation spuriously fail on specific kernel tick rates?

The monitor expiration threshold is calculated as 10000 + TICK_NSEC
nanoseconds.

Due to integer truncation in jiffies math (e.g. at HZ=300, TICK_USEC is 3333
and TICK_NSEC is 3333333), the injected delay (10 + TICK_USEC, or 3343000 ns)
would fall 333 ns short of the mathematical threshold (3343333 ns).

Depending on execution overhead, could the constraint check 
time_ns - reset_time <= ha_mon->expire evaluate to true, generating no
reaction and causing this RV_KUNIT_EXPECT_REACTION assertion to fail?

> +}
> +
> +#else
> +#define rv_test_throttle rv_test_stub
> +#endif
> diff --git a/kernel/trace/rv/monitors/throttle/throttle_kunit.h b/kernel/trace/rv/monitors/throttle/throttle_kunit.h
> new file mode 100644
> index 0000000000000..d730497deca4d
> --- /dev/null
> +++ b/kernel/trace/rv/monitors/throttle/throttle_kunit.h
[ ... ]
> +extern const struct rv_throttle_ops {
> +	struct rv_kunit_mon mon;
> +	void (*handle_dl_replenish)(void *data, struct sched_dl_entity *dl_se,
> +				int cpu, u8 type);
> +	void (*handle_dl_throttle)(void *data, struct sched_dl_entity *dl_se,
> +			       int cpu, u8 type);
> +	void (*handle_sched_enqueue)(void *data, struct task_struct *tsk, int cpu);
> +	void (*handle_sched_switch)(void *data, bool preempt,
> +				struct task_struct *prev,
> +				struct task_struct *next,
> +				unsigned int prev_state);
> +	void (*handle_sys_enter)(void *data, struct pt_regs *regs, long id);
> +	void (*handle_newtask)(void *data, struct task_struct *task, u64 flags);
> +	void (*handle_dl_server_stop)(void *data, struct sched_dl_entity *dl_se,
> +				  int cpu, u8 type);
> +} rv_throttle_ops;

[Severity: Low]
Does this struct definition also need the handle_exit callback added to
properly export the API for the test environment?

> +#endif
> +
> +#endif /* __THROTTLE_KUNIT_H */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929124908.177676-1-gmonaco@redhat.com?part=9