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

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.

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

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()?

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