Re: [RFC PATCH 01/17] rv: Refactor da_monitor to minimise macros

[Gabriele Monaco <gmonaco@redhat.com>]

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:
> > 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.

...
> > +#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.

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

> > 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.

> > +#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