Re: [PATCH v3 3/4] rv: collect per-edge dwell time for per-cpu DA/HA monitors

[sashiko-bot@kernel.org]

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 <tobias.schaffner@siemens.com>

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/20261020095512.863754-1-tobias.schaffner@siemens.com?part=3