Re: [PATCH v6 6/9] rv: Add tlob hybrid automaton monitor

[Gabriele Monaco]

Hi Wen,

thanks for the series, I'm going with the first chunk, I still need to
review the unbind_reap logic here.

On Fri, 2026-08-21 at 00:45 +0800, wen.yang@xxxxxxxxx wrote:
> From: Wen Yang <wen.yang@xxxxxxxxx>
> +static inline void tlob_reset_notify(struct da_monitor *da_mon)
> +{
> +	struct ha_monitor *ha_mon = to_ha_monitor(da_mon);
> +	struct tlob_task_state *ws;
> +
> +	ha_monitor_reset_env(da_mon);
> +
> +	ws = ha_get_target(ha_mon);
> +	if (!ws)
> +		return;
> +
> +	/*
> +	 * stopping==1 means tlob_stop_task() ended this window already.
> +	 * acquire pairs with the _release clear in ha_setup_invariants().
> +	 */
> +	if (atomic_read_acquire(&ws->stopping))
> +		return;

Maybe I'm getting confused by the "acquire" in here, but are you aiming
to "acquire" the stopping here by reading and then setting it later?
That's obviously not atomic so you could read 0 and a concurring handler
could set it to 1 before you do. Wouldn't it be better to cmpxchg?

> +
> +	/*
> +	 * Monitor disable (ha_mon_destroying set) is not a violation: the
> +	 * teardown paths free ws regardless.  Couples to an HA-layer flag
> +	 * with no public contract; a framework-level equivalent would be
> +	 * cleaner.
> +	 */
> +	if (unlikely(READ_ONCE(ha_mon_destroying)))
> +		return;

This should be the first check in this function, there's no need to do
anything else and you cannot trust the da_mon pointer.
Put it before ha_monitor_reset_env() (to_ha_monitor is pointer
arithmetic, it can stay where it is for better readability).

> +
> +	/* Genuine expiry: end the window so a later start takes the restart
> path. */
> +	atomic_set(&ws->stopping, 1);
> +
> +	/* Stamped regardless of the tracepoint; tlob_stop_task() reads it.
> */
> +	WRITE_ONCE(ws->budget_exceeded, true);
> +
> +	if (!trace_detail_env_tlob_enabled())
> +		return;
> +
> +	unsigned int curr_state = READ_ONCE(da_mon->curr_state);
> +	u64 accs[TLOB_ACC_MAX], partial_ns;
> +	unsigned long flags;
> +
> +	/* Snapshot accumulators; partial_ns covers curr_state time not yet
> folded in. */
> +	raw_spin_lock_irqsave(&ws->entry_lock, flags);
> +	partial_ns = ktime_get_ns() - ktime_to_ns(ws->last_ts);
> +	accs[TLOB_ACC_RUNNING]  = ws->accs_ns[TLOB_ACC_RUNNING]  +
> +				  (curr_state == running_tlob  ? partial_ns :
> 0);
> +	accs[TLOB_ACC_WAITING]  = ws->accs_ns[TLOB_ACC_WAITING]  +
> +				  (curr_state == waiting_tlob  ? partial_ns :
> 0);
> +	accs[TLOB_ACC_SLEEPING] = ws->accs_ns[TLOB_ACC_SLEEPING] +
> +				  (curr_state == sleeping_tlob ? partial_ns :
> 0);
> +	raw_spin_unlock_irqrestore(&ws->entry_lock, flags);
> +
> +	trace_detail_env_tlob(da_get_id(da_mon), ws->threshold_ns,
> +			      accs[TLOB_ACC_RUNNING],
> +			      accs[TLOB_ACC_WAITING],
> +			      accs[TLOB_ACC_SLEEPING]);
> +}
> +
> +#define BUDGET_NS(ha_mon) (ha_get_target(ha_mon)->threshold_ns)
> +
> +/* HA constraint functions (called by ha_monitor_handle_constraint) */
> +
> +static u64 ha_get_env(struct ha_monitor *ha_mon, enum envs_tlob env,
> +		      u64 time_ns)
> +{
> +	if (env == clk_elapsed_tlob)
> +		return ha_get_clk_ns(ha_mon, env, time_ns);
> +	return ENV_INVALID_VALUE;
> +}
> +
> +/*
> + * Invariant: clk_elapsed < BUDGET_NS in running/waiting/sleeping.  "stopped"
> + * is exempt: the parked period must not be measured against the old window's
> + * clock anchor (restart from "stopped" would otherwise spuriously overrun).
> + */
> +static inline bool ha_verify_invariants(struct ha_monitor *ha_mon,
> +					enum states curr_state, enum events
> event,
> +					enum states next_state, u64 time_ns)
> +{
> +	if (curr_state == stopped_tlob)
> +		return true;
> +	return ha_check_invariant_ns(ha_mon, clk_elapsed_tlob, time_ns,
> BUDGET_NS(ha_mon));
> +}
> +
> +/*
> + * The clock stays in guard (anchor) representation all window: env_store
> + * holds the window-start timestamp, re-anchored on start/restart.
> + * ha_invariant_passed_ns() never stores the deadline representation (the
> + * framework dropped ha_set_invariant_ns(), commit ab2900ae252b), so calling
> + * ha_inv_to_guard() here would subtract BUDGET_NS from the anchor and skew
> + * every check by one budget.  nomiss likewise never converts.
> + */
> +
> +/* No per-event guard conditions for tlob; invariants suffice. */
> +static inline bool ha_verify_guards(struct ha_monitor *ha_mon,
> +				    enum states curr_state, enum events
> event,
> +				    enum states next_state, u64 time_ns)
> +{

Mmh, I just realised you don't use the guards for resets, is the order
of actions a problem for this monitor?

> +	return true;
> +}
> +
> +/*
> + * Guard on stopping: a sched_switch after ha_cancel_timer_sync() would
> + * re-arm the timer (ODEBUG splat).  _acquire pairs with cmpxchg_release in
> + * tlob_stop_task.
> + *
> + * Entering stopped_tlob also resets env_store to the invalid sentinel, so a
> + * restart re-anchors the clock; a stale anchor would wrap the restart's
> + * timer delay to ~U64_MAX.
> + */
> +static inline void ha_setup_invariants(struct ha_monitor *ha_mon,
> +				       enum states curr_state, enum events
> event,
> +				       enum states next_state, u64 time_ns)
> +{
> +	if (next_state == stopped_tlob) {
> +		/*
> +		 * Window ending: reset env_store to the invalid sentinel so
> +		 * the next window gets a fresh clock anchor.  Keep
> stopping==1
> +		 * so __tlob_acc() continues to block sched events while
> parked.
> +		 */
> +		ha_monitor_reset_all_stored(ha_mon);

Is this really necessary since you have a reset() in the start edge?
Isn't that anyway equivalent to reset() on the stop edge too?

> +		return;
> +	}
> +
> +	if (atomic_read_acquire(&ha_get_target(ha_mon)->stopping)) {

Same as above, do you mean to cmpxchg?

> +		/*
> +		 * Restart (stopped -> running): arm the timer, then clear
> +		 * stopping so __tlob_acc() admits sched events only once the
> +		 * state is already running_tlob.  _release pairs with the
> +		 * acquires in __tlob_acc/tlob_reset_notify.
> +		 */
> +		if (next_state < state_max_tlob)
> +			ha_start_timer_ns(ha_mon, clk_elapsed_tlob,
> BUDGET_NS(ha_mon), time_ns);
> +		atomic_set_release(&ha_get_target(ha_mon)->stopping, 0);
> +		return;
> +	}
...
> +
> +/*
> + * Accumulate elapsed ns into accs_ns[idx] since last_ts and advance it.
> + * Returns true if monitored with an active window.  The stopping gate is
> + * what keeps scheduler events from reaching a parked task (no "stopped"
> + * self-loops, see tlob.h) and keeps accs_ns[] from growing while parked.
> + */
> +static inline bool __tlob_acc(struct task_struct *task, ktime_t now,
> +			       enum tlob_acc_idx idx)
> +{
> +	struct tlob_task_state *ws;
> +	unsigned long flags;
> +
> +	guard(rcu)();
> +	ws = da_get_target_by_id(task->pid);
> +	/* acquire pairs with the _release clear in ha_setup_invariants(). */
> +	if (!ws || atomic_read_acquire(&ws->stopping))
> +		return false;

Mmh, returning false on !ws is kind of a shortcut to avoid another
hashtable lookup that would return nothing, but skipping events on
stopping is actually changing how the model behaves.

It is usually clearer to allow all events in a stopped state as
self-loops (the /task/ can go through those events also when stopped).
I usually prefer to keep logic in the model rather than in the source
file.

It wouldn't be too wrong to say that the task can do anything but the
tlob instance is stopped and other events cannot occur there, like
you're doing, but that should be well documented.
You're vaguely mentioning this in some changelog, if you want to keep it
this way, please explain it in the documentation file.
Word it like a description of the model rather than how you implement
it, something like (adapt it accordingly): "although tasks can enter the
scheduler when tlob is in the stopped state, those events are explicitly
ignored by the tlob instance, hence there is no self-loop in the stopped
state".

Thanks,
Gabriele