On Thu, 2025-08-21 at 10:14 +0200, Nam Cao wrote:

On Thu, Aug 14, 2025 at 05:07:53PM +0200, Gabriele Monaco wrote:

quoted

The da_monitor helper functions are generated from macros of the
type:

DECLARE_DA_FUNCTION(name, type) \
static void da_func_x_##name(type arg) {} \
static void da_func_y_##name(type arg) {} \

This is good to minimise code duplication but the long macros made
of skipped end of lines is rather hard to parse. Since functions
are static, the advantage of naming them differently for each
monitor is minimal.

Refactor the da_monitor.h file to minimise macros, instead of
declaring functions from macros, we simply declare them with the
same name for all monitors (e.g. da_func_x) and for any remaining
reference to the monitor name (e.g. tracepoints, enums, global
variables) we use the CONCATENATE macro.

...

quoted

+#define RV_AUTOMATON_NAME CONCATENATE(automaton_, MONITOR_NAME)
+#define EVENT_MAX CONCATENATE(event_max_, MONITOR_NAME)
+#define STATE_MAX CONCATENATE(state_max_, MONITOR_NAME)
+#define events CONCATENATE(events_, MONITOR_NAME)
+#define states CONCATENATE(states_, MONITOR_NAME)

I think these macros make it harder to read the code. E.g. it is not
obvious what is "events" in "enum events event".

How about renaming these to be the same for all monitors, and get rid
of these 4 macros?

Something like

	enum states_wip -> enum da_states
	state_max_wip   -> da_state_max
	etc

Just my preference, of course. You probably have your reasons for
doing it this way?

I think the reason was mostly laziness / wish to change less things.
This would require to change a bit more in each monitor's header and
rvgen, but since I'm already changing those, it should be no problem.

I assume the compiler would be just fine with the change and I don't see
a use-case where one would grab multiple definitions of those enums to
get a clash.

Good point, I'll look into that.

quoted

+/*
+ * model_get_state_name - return the (string) name of the given
state
+ */
+static char *model_get_state_name(enum states state)
+{
+	if ((state < 0) || (state >= STATE_MAX))
+		return "INVALID";

Just notice that this probably should be
	if (BUG_ON((state < 0) || (state >= STATE_MAX)))

You shouldn't do it in this patch of course. I just want to note it
down.

Mmh, I'm not quite sure about this one, sure it's not a good thing when
we try to get an invalid state/event, but isn't BUG a bit too much here?
We're handling things gracefully so the kernel is not broken (although
rv likely is).

If you really think we should note that, what about just WARN/WARN_ONCE ?

quoted

index 17fa4f6e5ea6..bc02334aa8be 100644

--- a/include/rv/da_monitor.h
+++ b/include/rv/da_monitor.h

@@ -13,97 +13,102 @@

 
 #include <rv/automata.h>
 #include <linux/rv.h>
+#include <linux/stringify.h>
 #include <linux/bug.h>
 #include <linux/sched.h>
 
+#define RV_MONITOR_NAME CONCATENATE(rv_, MONITOR_NAME)
+#define RV_DA_MON_NAME CONCATENATE(da_mon_, MONITOR_NAME)

These macros as well. Should we rename the monitors to be the same
and get rid of the macros?

I see you took this RV_MONITOR_NAME idea from LTL. But I'm starting
to wonder if this is really a good idea.

What do you think?

Same laziness, I'll look into that!
They are static so they won't ever be defined together, nor I see a
reason for them not to be static.

One thing to note is that by using the same names everywhere, the
symbols won't be as accessible from debug tools (gdb/BPF or whatever),
I'm not sure that's really an issue, but it's the only downside coming
to my mind.

quoted

+#if RV_MON_TYPE == RV_MON_GLOBAL || RV_MON_TYPE == RV_MON_PER_CPU
+
+static inline bool
+da_event(struct da_monitor *da_mon, enum events event)
+{
+	enum states curr_state, next_state;
+
+	curr_state = READ_ONCE(da_mon->curr_state);
+	for (int i = 0; i < MAX_DA_RETRY_RACING_EVENTS; i++) {
+		next_state = model_get_next_state(curr_state,
event);
+		if (next_state == INVALID_STATE) {
+			cond_react(curr_state, event);
+			CONCATENATE(trace_error_,
MONITOR_NAME)(model_get_state_name(curr_state),
+					  
model_get_event_name(event));
+			return false;
+		}
+		if (likely(try_cmpxchg(&da_mon->curr_state,
&curr_state, next_state))) {
+			CONCATENATE(trace_event_,
MONITOR_NAME)(model_get_state_name(curr_state),
+					  
model_get_event_name(event),
+					  
model_get_state_name(next_state),
+					  
model_is_final_state(next_state));
+			return true;
+		}
+	}
+
+	trace_rv_retries_error(__stringify(MONITOR_NAME),
model_get_event_name(event));
+	pr_warn("rv: " __stringify(MAX_DA_RETRY_RACING_EVENTS)
+		" retries reached for event %s, resetting monitor
%s",
+		model_get_event_name(event),
__stringify(MONITOR_NAME));
+	return false;
+}

You have two da_event(), which is mostly similar, except the
function's signature and the trace event. Would it be sane to unify
them, and only putting the differences in #ifdef?

Perhaps something like:

#if RV_MON_TYPE == RV_MON_GLOBAL || RV_MON_TYPE == RV_MON_PER_CPU

typedef struct {} monitor_target;

static void da_trace_event(struct da_monitor *da_mon, monitor_target
target,
			   enum events event, enum states curr, enum
states next)
{
	CONCATENATE(trace_event_,
MONITOR_NAME)(model_get_state_name(curr_state),
		    model_get_event_name(event),
		    model_get_state_name(next_state),
		    model_is_final_state(next_state));
}

#elif RV_MON_TYPE == RV_MON_PER_TASK

typedef struct task_struct *monitor_target;

static void da_trace_event(struct da_monitor *da_mon, struct
task_struct *task,
			   enum events event, enum states curr, enum
states next)
{
	CONCATENATE(trace_event_, MONITOR_NAME)(task->pid,
		   model_get_state_name(curr_state),
		   model_get_event_name(event),
		   model_get_state_name(next_state),
		   model_is_final_state(next_state));
}

#endif

static inline bool
da_event(struct da_monitor *da_mon, monitor_target target, enum
events event)
{
	enum states curr_state, next_state;

	curr_state = READ_ONCE(da_mon->curr_state);
	for (int i = 0; i < MAX_DA_RETRY_RACING_EVENTS; i++) {
		next_state = model_get_next_state(curr_state,
event);
		if (next_state == INVALID_STATE) {
			cond_react(curr_state, event);
			da_trace_event(da_mon, event, target,
curr_state, next_state);
			return false;
		}
		if (likely(try_cmpxchg(&da_mon->curr_state,
&curr_state, next_state))) {
			da_trace_error(...);
			return true;
		}
	}

	trace_rv_retries_error(__stringify(MONITOR_NAME),
model_get_event_name(event));
	pr_warn("rv: " __stringify(MAX_DA_RETRY_RACING_EVENTS)
		" retries reached for event %s, resetting monitor
%s",
		model_get_event_name(event),
__stringify(MONITOR_NAME));
	return false;
}

Same for the other functions.

Mmh, that's been bugging me throughout every change, but I'm not sure
it's that simple, implicit monitors don't have a target at all, so all
function prototypes are different because that needs to propagate.
Now that I think about it, it may not be a big deal not to pass the
target at all and retrieve it from the da_mon (that's just pointer
arithmetic the compiler might even optimise out).

I'm not sure that would be as simple if per-cpu monitors stopped being
implicit (like they are in your patch: they do have a target), but that
may never happen.

I'll give it a shot, thanks for pointing that out!

Thanks again,
Gabriele