Re: [PATCH v2 2/3] rv: add per-monitor edge-stat facility and stats file

[Gabriele Monaco <gmonaco@redhat.com>]

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

> +
> +/*
> + * 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"
>