Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] rv: bogus initial dwell time in __da_handle_start_run_event() --
commit 296c211b8b28be395fee98f1ad59896cd58a9336 Author: Tobias Schaffner <[email protected]> rv: collect per-edge dwell time for per-cpu DA/HA monitors This commit hooks the edge-stat facility into the DA and HA monitor layers to record per-edge state dwell times for per-CPU automaton monitors and documents the resulting stats file in runtime-verification.rst. > diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h > index 6fc5ef8142aca..c8f7497800d07 100644 > --- a/include/rv/da_monitor.h > +++ b/include/rv/da_monitor.h [ ... ] > @@ -113,7 +114,7 @@ static inline void da_monitor_reset(struct da_monitor > *da_mon) > */ > static inline void da_monitor_start(struct da_monitor *da_mon) > { > - da_mon->curr_state = model_get_initial_state(); > + da_mon->curr_state = da_state_entered(model_get_initial_state()); > da_monitor_init_hook(da_mon); > /* Pairs with smp_load_acquire in da_monitoring(). */ > smp_store_release(&da_mon->monitoring, 1); [ ... ] > @@ -689,9 +696,11 @@ static inline void da_trace_error(struct da_monitor > *da_mon, > static inline bool da_event(struct da_monitor *da_mon, enum events event, > da_id_type id) > { > enum states curr_state, next_state; > + da_state_t old; > > - curr_state = READ_ONCE(da_mon->curr_state); > + old = READ_ONCE(da_mon->curr_state); > for (int i = 0; i < MAX_DA_RETRY_RACING_EVENTS; i++) { > + curr_state = da_state_of(old); > next_state = model_get_next_state(curr_state, event); > if (next_state == INVALID_STATE) { > react(curr_state, event); > @@ -699,7 +708,8 @@ static inline bool da_event(struct da_monitor *da_mon, > enum events event, da_id_ > model_get_event_name(event), id); > return false; > } > - if (likely(da_state_try_commit(&da_mon->curr_state, &old, > - next_state, event))) { > + if (likely(da_state_try_commit(&da_mon->curr_state, &old, > + next_state, event))) { > if (!da_monitor_event_hook(da_mon, curr_state, event, > next_state, id)) > return false; > da_trace_event(da_mon, model_get_state_name(curr_state), [Severity: Medium] Does stamping da_mon->curr_state with da_state_entered() in da_monitor_start() record a bogus dwell time when starting a monitor via __da_handle_start_run_event()? Unlike __da_handle_start_event(), which calls da_monitor_start() and returns immediately when entering the initial state, __da_handle_start_run_event() is used when the system is already in the initial state for an unknown duration and an outgoing transition occurs: include/rv/da_monitor.h:__da_handle_start_run_event() { if (!da_monitor_enabled()) return 0; if (unlikely(!da_monitoring(da_mon))) da_monitor_start(da_mon); __da_handle_event_common(da_mon, event, id); return 1; } When !da_monitoring(da_mon) is true (such as in the opid monitor on the first handle_sched_need_resched() or handle_sched_waking() event after enabling or resetting), da_monitor_start() stamps curr_state with a non-zero local_clock() timestamp via da_state_entered(). Immediately afterwards, __da_handle_event_common() calls da_event(), where da_state_try_commit() takes a second local_clock() timestamp and calls rv_edge_account(). Because the timestamp in old is non-zero, won't rv_edge_account() record the few nanoseconds spent between da_monitor_start() and da_state_try_commit() as the dwell time for the initial state's outgoing edge? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
