Re: [RFC PATCH 2/4] rv/tlob: Add tlob deterministic automaton monitor

[Gabriele Monaco]

Hello,

On Thu, 2026-04-16 at 23:09 +0800, Wen Yang wrote:
> 
> Thanks for the review.  Here's my plan for each point -- let me know if 
> the direction looks right.
> 
> 
> - Timed automata
> 
> The HA framework [1] is a good match when the timeout threshold is 
> global or state-determined, but tlob needs a per-invocation threshold 
> supplied at TRACE_START time -- fitting that into HA would require 
> framework changes.

Not quite, look at the nomiss monitor, the deadline comes directly from the
deadline entity.

What I meant with using per-object monitor is that you can use your custom
struct as a monitor target, that has your per-invocation threshold because you
set instantiate it on start.

Now you can simply do ha_get_target(ha_mon)->threshold and you get your value.

You can define in the dot representation "clk < THRESHOLD_NS()" and rvgen will
do most of the things for you. It's probably better to use nanoseconds so you
avoid conversions when dealing with hrtimers. You can do it transparently when
initialising so the user still passes us.

> My plan is to use da_monitor_init_hook() -- the same mechanism HA 
> monitors use internally -- to arm the per-invocation hrtimer once 
> da_create_storage() has stored the monitor_target.  This gives the same 
> "timer fires => violation" semantics without touching the HA infrastructure.
> 
> If you see a cleaner way to pass per-invocation data through HA I'm 
> happy to go that route.

The above looks cleaner to me, what do you think?

da_monitor_init_hook() isn't really meant to be used by monitors, it's more for
the infrastructure to extend da_monitor.h easily, sure you can use it if there's
no other way, though.

> - Unmonitored state / da_handle_start_event
> 
> Fair point.  I'll drop the explicit unmonitored state and the
> trace_event_tlob() redefinition.  tlob_start_task() will use
> da_handle_start_event() to allocate storage, set initial state to on_cpu,
> and fire the init hook to arm the timer in one shot.  tlob_stop_task()
> calls da_monitor_reset() directly.
> 
> - Per-object monitors
> 
> Will do.  The custom hash table goes away; I'll switch to RV_MON_PER_OBJ
> with:
> 
>      typedef struct tlob_task_state *monitor_target;
> 
> da_get_target_by_id() handles the sched_switch hot path lookup.
> 

Exactly! That should do.

> - RV-way violations
> 
> Agreed.  budget_expired will be declared INVALID in all states so the
> framework calls react() (error_tlob tracepoint + any registered reactor)
> and da_monitor_reset() automatically.  tlob won't emit any tracepoint of
> its own.
> 
> One note on the /dev/tlob ioctl: TLOB_IOCTL_TRACE_STOP returns -EOVERFLOW
> to the caller when the budget was exceeded.  This is just a syscall 
> return code -- not a second reporting path -- to let in-process 
> instrumentation react inline without polling the trace buffer.
> Let me know if you have concerns about keeping this.
> 

I'm not sure how faster can it be compared to attaching to the tracefs, that
should be quite light if you just listen to error events. Sure you'd need a few
more libraries.

I'm a bit concerned in adding new interfaces (ioctl), when we have already
tracepoints and reactors. The reactors themselves are not as flexible as they
should be though, but if required we may definitely create a ioctl reactor just
for this.

For now ignore all this and continue with the TLOB_IOCTL_TRACE_STOP, then we can
think of the details.

> - Generic uprobe helper
> 
> Proposed interface:
> 
>      struct rv_uprobe *rv_uprobe_attach_path(
>              struct path *path, loff_t offset,
>              int (*entry_fn)(struct rv_uprobe *, struct pt_regs *, __u64 *),
>              int (*ret_fn)  (struct rv_uprobe *, unsigned long func,
>                              struct pt_regs *, __u64 *),
>              void *priv);
> 
>      struct rv_uprobe *rv_uprobe_attach(
>              const char *binpath, loff_t offset,
>              int (*entry_fn)(struct rv_uprobe *, struct pt_regs *, __u64 *),
>              int (*ret_fn)  (struct rv_uprobe *, unsigned long func,
>                              struct pt_regs *, __u64 *),
>              void *priv);
> 
>      void rv_uprobe_detach(struct rv_uprobe *p);
> 
> struct rv_uprobe exposes three read-only fields to monitors (offset, 
> priv, path); the uprobe_consumer and callbacks would be kept private to 
> the implementation, so monitors need not include <linux/uprobes.h>.
> 
> rv_uprobe_attach() resolves the path and delegates to 
> rv_uprobe_attach_path(); the latter avoids a redundant kern_path() when 
> registering multiple probes on the same binary:
> 
>      kern_path(binpath, LOOKUP_FOLLOW, &path);
>      b->start = rv_uprobe_attach_path(&path, offset_start, entry_fn, 
> NULL, b);
>      b->stop  = rv_uprobe_attach_path(&path, offset_stop,  stop_fn, 
> NULL, b);
>      path_put(&path);
> 
> Does the interface look reasonable, or did you have a different shape in 
> mind?
> 

Yeah seems reasonable. Then we'd need to keep around the uprobe for
deinitialisation, but probably having it global is the best way without
overengineer anything.

Thanks,
Gabriele