On Fri, 2026-09-11 at 14:34 +0200, Tobias Schaffner wrote:
> Add CONFIG_RV_EDGE_STAT, an optional feature that records how long a
> monitor's automaton dwells in a state and exposes it per edge through a
> per-monitor "stats" tracefs file.
>
> The facility uses a static per-CPU matrix and reports, per edge and per
> CPU, the count, summed dwell time and maximum dwell time. Only the owning
> CPU writes the counters, so a reader snapshots them with local64_read()
> with no IPI and no locking on the accounting path.
>
> Signed-off-by: Tobias Schaffner <[email protected]>
> ---
...
> +
> +/*
> + * Static storage avoids allocation lifetime races. Only the owning CPU
> writes,
> + * so readers can snapshot counters with local64_read().
> + */
> +#define RV_THIS_NR_EDGES (STATE_MAX * EVENT_MAX)
> +
> +struct rv_this_edges {
> + struct rv_edge_stat edge[RV_THIS_NR_EDGES];
> +};
Does this really need to be a new struct? Cannot you just define it as:
DEFINE_PER_CPU(struct rv_edge_stat[RV_THIS_NR_EDGES], DA_MON_EDGES);
Then use it as:
per_cpu_ptr(DA_MON_EDGES, cpu), sizeof(DA_MON_EDGES)
struct rv_edge_stat *edges = per_cpu_ptr(DA_MON_EDGES, cpu);
&edges[e].count
> +
> +static DEFINE_PER_CPU(struct rv_this_edges, DA_MON_EDGES);
> +static struct dentry *rv_this_stats_file;
> +
> +/* Called before tracepoints are registered, so memset cannot race an update.
> */
> +static void rv_edge_stats_reset(void)
> +{
> + int cpu;
> +
> + for_each_possible_cpu(cpu)
> + memset(per_cpu_ptr(&DA_MON_EDGES, cpu), 0,
> + sizeof(struct rv_this_edges));
> +}
> +
> +static int rv_edge_stats_show(struct seq_file *seq, void *v)
> +{
> + unsigned int e;
> + int cpu;
> +
> + seq_puts(seq, "# cpu edge label count sum_ns max_ns\n");
> + /* Include offline CPUs and keep output stable across CPU hotplug. */
> + for_each_possible_cpu(cpu) {
> + struct rv_this_edges *m = per_cpu_ptr(&DA_MON_EDGES, cpu);
> +
> + for (e = 0; e < RV_THIS_NR_EDGES; e++)
> + seq_printf(seq, "%d %u %s:%s %llu %llu %llu\n",
> + cpu, e,
> + model_get_state_name(e / EVENT_MAX),
> + model_get_event_name(e % EVENT_MAX),
Perhaps you could have macros like DA_EDGE(s, e) or DA_EDGE_TO_EVENT(e)
DA_EDGE_TO_STATE(s) to make the matrix selection a bit more readable,
but that's a matter of taste.
> + (u64)local64_read(&m->edge[e].count),
> + (u64)local64_read(&m->edge[e].sum_ns),
> + (u64)local64_read(&m->edge[e].max_ns));
> + }
I would like to see a way to tell if an edge is invalid or not, we
expect zero on invalid edges.
Perhaps that can be a new column so you also show the count in case they
are not 0.
You can just use model_get_next_state() == INVALID_STATE
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(rv_edge_stats);
> +
> +static int rv_edge_stats_create(struct dentry *parent)
> +{
> + rv_edge_stats_reset();
> + /* RV_MODE_READ is private to kernel/trace/rv/. */
I suggested you to use the rv helpers for this, you can obviously also
use RV_MODE_READ. Now monitors are in kernel/trace/rv/, so it won't be
an issue.
If we add support for modular monitors, moving the definitions to
linux/rv.h is the easiest thing to do anyway (you can do it too if you
really want and/or your agents complain, in a separate commit).
> + rv_this_stats_file = tracefs_create_file("stats", 0440, parent,
> + NULL, &rv_edge_stats_fops);
> + return rv_this_stats_file ? 0 : -ENOMEM;
> +}
> +
> +static void rv_edge_stats_remove(void)
> +{
> + tracefs_remove(rv_this_stats_file);
> + rv_this_stats_file = NULL;
> +}
> +
> +/* A zero timestamp marks resets, which can run on a different CPU. */
> +#define da_state_entered(s) da_state_pack((s), local_clock() &
> RV_TS_MASK)
> +
> +static __always_inline void
> +rv_edge_account(da_state_t old, da_state_t new, enum states curr, enum events
> ev)
> +{
> + struct rv_this_edges *m;
> + u64 prev = da_ts_of(old);
> +
> + if (!prev)
> + return;
This happens only the first time after reset, isn't it unlikely()?
> +
> + m = this_cpu_ptr(&DA_MON_EDGES);
> + rv_edge_stat_account(&m->edge[curr * EVENT_MAX + ev],
> + (da_ts_of(new) - prev) & RV_TS_MASK);
If you decide to go for the macro, this would be DA_EDGE(s, e) or whatever
looks clear to you.
> +}
> +
> +#else /* per-cpu accounting off for this monitor type */
> +
> +#define da_state_entered(s) ((da_state_t)(s))
> +
> +static __always_inline void
> +rv_edge_account(da_state_t old, da_state_t new, enum states curr, enum events
> ev) { }
> +
> +#endif /* RV_MON_TYPE == RV_MON_PER_CPU */
> +
> +static __always_inline bool
> +da_state_try_commit(da_state_t *word, da_state_t *old, enum states next,
> + enum events event)
> +{
> + da_state_t prev = *old;
> + da_state_t new = da_state_entered(next);
> +
> + if (!try_cmpxchg(word, old, new))
> + return false;
> +
> + rv_edge_account(prev, new, da_state_of(prev), event);
> + return true;
> +}
> +
> +#else /* !CONFIG_RV_EDGE_STAT */
> +
> +/*
> + * Feature off: only the hooks the DA/HA layer calls are provided, as trivial
> + * pass-throughs. da_state_t is a plain state word with no packed timestamp.
> + */
> +#define da_state_of(w) ((unsigned int)(w))
Is this cast necessary? We assign it to an enum, the sign isn't really
important. I guess it's the same for the other implementation of
da_state_of() (which is now in another patch).
Thanks,
Gabriele
> +#define da_state_entered(s) (s)
> +
> +static __always_inline bool
> +da_state_try_commit(da_state_t *word, da_state_t *old, enum states next,
> + enum events event)
> +{
> + return try_cmpxchg(word, old, next);
> +}
> +
> +#if RV_MON_TYPE == RV_MON_PER_CPU
> +static inline int rv_edge_stats_create(struct dentry *parent) { return 0; }
> +static inline void rv_edge_stats_remove(void) { }
> +#endif
> +
> #endif /* CONFIG_RV_EDGE_STAT */
>
> #endif /* _RV_EDGE_STAT_H */
> diff --git a/kernel/trace/rv/Kconfig b/kernel/trace/rv/Kconfig
> index 3884b14df375..3045037fb36a 100644
> --- a/kernel/trace/rv/Kconfig
> +++ b/kernel/trace/rv/Kconfig
> @@ -59,6 +59,18 @@ config RV_PER_TASK_MONITORS
> This option configures the maximum number of per-task RV monitors
> that can run
> simultaneously.
>
> +config RV_EDGE_STAT
> + bool "Per-edge dwell-time statistics"
> + depends on RV && 64BIT && DA_MON_EVENTS_IMPLICIT
> + help
> + Record per-edge dwell-time statistics for per-cpu deterministic and
> + hybrid automaton monitors and expose them through a per-monitor
> + "stats" tracefs file. This times each monitored automaton
> transition
> + with local_clock(), so leave it off if you do not need the
> + statistics.
> +
> + If unsure, say N.
> +
> source "kernel/trace/rv/monitors/wip/Kconfig"
> source "kernel/trace/rv/monitors/wwnr/Kconfig"
>
