Re: [PATCH 1/3] rv: Pass va_list to reactors

[Gabriele Monaco <gmonaco@redhat.com>]

On Tue, 2025-10-14 at 07:51 +0200, Thomas Weißschuh wrote:
> The only thing the reactors can do with the passed in varargs is to
> convert it into a va_list. Do that in a central helper instead.
> It simplifies the reactors, removes some hairy macro-generated code
> and introduces a convenient hook point to modify reactor behavior.
> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
> --- a/include/rv/da_monitor.h
> +++ b/include/rv/da_monitor.h
> @@ -16,34 +16,19 @@
>  #include <linux/bug.h>
>  #include <linux/sched.h>
>  
> -#ifdef CONFIG_RV_REACTORS
> -
> -#define DECLARE_RV_REACTING_HELPERS(name,
> type)							\
> -static void cond_react_##name(type curr_state, type
> event)					\
> -
> {												\
> -	if (!rv_reacting_on() ||
> !rv_##name.react)						\
> -
> 		return;										\
> -	rv_##name.react("rv: monitor %s does not allow event %s on state
> %s\n",			\
> -
> 			#name,									\
> -
> 			model_get_event_name_##name(event),					\
> -
> 			model_get_state_name_##name(curr_state));				\
> -}
> -

The overall change looks good to me, just keep in mind there is an ongoing
refactor [1] for the da_monitor header to get rid of those macros, basically
making it more similar to the current ltl.
Depending on which gets merged first, you may need some (trivial) adaptation of
this.

Going to test it out more carefully in the next days.

Thanks,
Gabriele

[1] - https://lore.kernel.org/lkml/20250919140954.104920-1-gmonaco@redhat.com

> -#else /* CONFIG_RV_REACTOR */
> -
> -#define DECLARE_RV_REACTING_HELPERS(name,
> type)							\
> -static void cond_react_##name(type curr_state, type
> event)					\
> -
> {												\
> -
> 	return;											\
> -}
> -#endif
> -
>  /*
>   * Generic helpers for all types of deterministic automata monitors.
>   */
>  #define DECLARE_DA_MON_GENERIC_HELPERS(name,
> type)						\
>  									
> 			\
> -DECLARE_RV_REACTING_HELPERS(name,
> type)								\
> +static void react_##name(type curr_state, type
> event)						\
> +{									
> 			\
> +	rv_react(&rv_##name,						
> 			\
> +		 "rv: monitor %s does not allow event %s on state
> %s\n",			\
> +		
> #name,										\
> +		
> model_get_event_name_##name(event),						\
> +		
> model_get_state_name_##name(curr_state));					\
> +}									
> 			\
>  									
> 			\
>  /*									
> 			\
>   * da_monitor_reset_##name - reset a monitor and setting it to init
> state			\
> @@ -126,7 +111,7 @@ da_event_##name(struct da_monitor *da_mon, enum
> events_##name event)				\
>  	for (int i = 0; i < MAX_DA_RETRY_RACING_EVENTS; i++)
> {					\
>  		next_state = model_get_next_state_##name(curr_state,
> event);			\
>  		if (next_state == INVALID_STATE)
> {						\
> -			cond_react_##name(curr_state,
> event);					\
> +			react_##name(curr_state,
> event);					\
>  			trace_error_##name(model_get_state_name_##name(curr_s
> tate),		\
>  					  
> model_get_event_name_##name(event));			\
>  			return
> false;								\
> @@ -165,7 +150,7 @@ static inline bool da_event_##name(struct da_monitor
> *da_mon, struct task_struct
>  	for (int i = 0; i < MAX_DA_RETRY_RACING_EVENTS; i++)
> {					\
>  		next_state = model_get_next_state_##name(curr_state,
> event);			\
>  		if (next_state == INVALID_STATE)
> {						\
> -			cond_react_##name(curr_state,
> event);					\
> +			react_##name(curr_state,
> event);					\
>  			trace_error_##name(tsk-
> >pid,						\
>  					  
> model_get_state_name_##name(curr_state),		\
>  					  
> model_get_event_name_##name(event));			\
> diff --git a/include/rv/ltl_monitor.h b/include/rv/ltl_monitor.h
> index
> 5368cf5fd623e74a5739d2e0b3fc2c27c4bad597..00c42b36f961a00ee473aa58f14b01530852
> 3eb0 100644
> --- a/include/rv/ltl_monitor.h
> +++ b/include/rv/ltl_monitor.h
> @@ -16,21 +16,12 @@
>  #error "Please include $(MODEL_NAME).h generated by rvgen"
>  #endif
>  
> -#ifdef CONFIG_RV_REACTORS
>  #define RV_MONITOR_NAME CONCATENATE(rv_, MONITOR_NAME)
> -static struct rv_monitor RV_MONITOR_NAME;
>  
> -static void rv_cond_react(struct task_struct *task)
> -{
> -	if (!rv_reacting_on() || !RV_MONITOR_NAME.react)
> -		return;
> -	RV_MONITOR_NAME.react("rv: "__stringify(MONITOR_NAME)": %s[%d]:
> violation detected\n",
> -			      task->comm, task->pid);
> -}
> +#ifdef CONFIG_RV_REACTORS
> +static struct rv_monitor RV_MONITOR_NAME;
>  #else
> -static void rv_cond_react(struct task_struct *task)
> -{
> -}
> +extern struct rv_monitor RV_MONITOR_NAME;
>  #endif
>  
>  static int ltl_monitor_slot = RV_PER_TASK_MONITOR_INIT;
> @@ -98,7 +89,8 @@ static void ltl_monitor_destroy(void)
>  static void ltl_illegal_state(struct task_struct *task, struct ltl_monitor
> *mon)
>  {
>  	CONCATENATE(trace_error_, MONITOR_NAME)(task);
> -	rv_cond_react(task);
> +	rv_react(&RV_MONITOR_NAME, "rv: "__stringify(MONITOR_NAME)": %s[%d]:
> violation detected\n",
> +		 task->comm, task->pid);
>  }
>  
>  static void ltl_attempt_start(struct task_struct *task, struct ltl_monitor
> *mon)
> diff --git a/kernel/trace/rv/reactor_panic.c b/kernel/trace/rv/reactor_panic.c
> index
> 74c6bcc2c7494408770881dda2b0de885c5eb88c..76537b8a4343cbd0d715f60db3cc8868e71c
> 1c0b 100644
> --- a/kernel/trace/rv/reactor_panic.c
> +++ b/kernel/trace/rv/reactor_panic.c
> @@ -13,13 +13,9 @@
>  #include <linux/init.h>
>  #include <linux/rv.h>
>  
> -__printf(1, 2) static void rv_panic_reaction(const char *msg, ...)
> +__printf(1, 0) static void rv_panic_reaction(const char *msg, va_list args)
>  {
> -	va_list args;
> -
> -	va_start(args, msg);
>  	vpanic(msg, args);
> -	va_end(args);
>  }
>  
>  static struct rv_reactor rv_panic = {
> diff --git a/kernel/trace/rv/reactor_printk.c
> b/kernel/trace/rv/reactor_printk.c
> index
> 2dae2916c05fd17397195e37d9b42d24cea24b9c..48c934e315b31c14d3a5b4fa3ec334ef71f9
> e390 100644
> --- a/kernel/trace/rv/reactor_printk.c
> +++ b/kernel/trace/rv/reactor_printk.c
> @@ -12,13 +12,9 @@
>  #include <linux/init.h>
>  #include <linux/rv.h>
>  
> -__printf(1, 2) static void rv_printk_reaction(const char *msg, ...)
> +__printf(1, 0) static void rv_printk_reaction(const char *msg, va_list args)
>  {
> -	va_list args;
> -
> -	va_start(args, msg);
>  	vprintk_deferred(msg, args);
> -	va_end(args);
>  }
>  
>  static struct rv_reactor rv_printk = {
> diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
> index
> d32859fec238371b5721e08cf6558f0805565f7b..cb1a5968055abb22439a066b4e25dad98f07
> 8389 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -438,7 +438,7 @@ int reactor_populate_monitor(struct rv_monitor *mon)
>  /*
>   * Nop reactor register
>   */
> -__printf(1, 2) static void rv_nop_reaction(const char *msg, ...)
> +__printf(1, 0) static void rv_nop_reaction(const char *msg, va_list args)
>  {
>  }
>  
> @@ -477,3 +477,17 @@ int init_rv_reactors(struct dentry *root_dir)
>  out_err:
>  	return -ENOMEM;
>  }
> +
> +void rv_react(struct rv_monitor *monitor, const char *msg, ...)
> +{
> +	va_list args;
> +
> +	if (!rv_reacting_on() || !monitor->react)
> +		return;
> +
> +	va_start(args, msg);
> +
> +	monitor->react(msg, args);
> +
> +	va_end(args);
> +}