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 <[email protected]> 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 <[email protected]>"); > 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/[email protected]?part=9
