Re: [PATCH 02/10] rv: Let the reactors take care of buffers

[Gabriele Monaco <gmonaco@redhat.com>]

On Thu, 2025-03-13 at 09:16 +0100, Nam Cao wrote:
> Hi Gabriele,
> 
> On Wed, Mar 12, 2025 at 08:58:56AM +0100, Gabriele Monaco wrote:
> > On Tue, 2025-03-11 at 18:05 +0100, Nam Cao wrote:
> > > diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h
> > > index 510c88bfabd4..c55d45544a16 100644
> > > --- a/include/rv/da_monitor.h
> > > +++ b/include/rv/da_monitor.h
> > > @@ -16,58 +16,11 @@
> > >  #include <linux/bug.h>
> > >  #include <linux/sched.h>
> > >  
> > > -#ifdef CONFIG_RV_REACTORS
> > > -
> > > -#define DECLARE_RV_REACTING_HELPERS(name,
> > > type)							\
> > > -static char
> > > REACT_MSG_##name[1024];						
> > > 		\
> > > -
> > > 								
> > > 				\
> > > -static inline char *format_react_msg_##name(type curr_state,
> > > type
> > > event)			\
> > > -
> > > {								
> > > 				\
> > > -	snprintf(REACT_MSG_##name,
> > > 1024,							\
> > > -		 "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));			
> > > 		\
> > > -	return
> > > REACT_MSG_##name;						
> > > 		\
> > > -
> > > }								
> > > 				\
> > > -
> > > 								
> > > 				\
> > > -static void cond_react_##name(char
> > > *msg)							\
> > > -
> > > {								
> > > 				\
> > > -	if
> > > (rv_##name.react)						
> > > 			\
> > > -
> > > 		rv_##name.react(msg);				
> > > 				\
> > > -
> > > }								
> > > 				\
> > > -
> > > 								
> > > 				\
> > > -static bool
> > > rv_reacting_on_##name(void)					
> > > 			\
> > > -
> > > {								
> > > 				\
> > > -	return
> > > rv_reacting_on();						
> > > 		\
> > > -}
> > > -
> > > -#else /* CONFIG_RV_REACTOR */
> > > -
> > > -#define DECLARE_RV_REACTING_HELPERS(name,
> > > type)							\
> > > -static inline char *format_react_msg_##name(type curr_state,
> > > type
> > > event)			\
> > > -
> > > {								
> > > 				\
> > > -	return
> > > NULL;								
> > > 		\
> > > -
> > > }								
> > > 				\
> > > -
> > > 								
> > > 				\
> > > -static void cond_react_##name(char
> > > *msg)							\
> > > -
> > > {								
> > > 				\
> > > -
> > > 	return;						
> > > 					\
> > > -
> > > }								
> > > 				\
> > > -
> > > 								
> > > 				\
> > > -static bool
> > > rv_reacting_on_##name(void)					
> > > 			\
> > > -
> > > {								
> > > 				\
> > > -	return
> > > 0;								
> > > 		\
> > > -}
> > > -#endif
> > > -
> > 
> > I don't think you need to remove those helper functions, why not
> > just
> > having format_react_msg_ prepare the arguments for react?
> 
> I'm not sure what you mean. Making format_react_msg_* a macro that is
> preprocessed into arguments? Then make cond_react_*() a variadic
> function,
> so that we can "pass" format_react_msg_* to it?
> 
> Going that way would also need a vreact() variant, as cond_react_*()
> cannot
> pass on the variadic arguments to react().
> 
> Instead, is it cleaner to do the below?

Hi Nam,

you're right, I got a bit confused, all I meant was to find a way not
to repeat the arguments for implicit and per-task monitors.
What you propose seems perfect to me.

Also for the sake of simplifying things, a bit like you started, we
could have the reacting_on() check inside cond_react and drop the per-
monitor function. I believe the initial idea was to have a reacting_on
toggle for each monitor but since it isn't the case, we don't really
need it.

Thanks,
Gabriele

> 
> diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h
> index 510c88bfabd4..e185ebf894a4 100644
> --- a/include/rv/da_monitor.h
> +++ b/include/rv/da_monitor.h
> @@ -19,22 +19,14 @@
>  #ifdef CONFIG_RV_REACTORS
>  
>  #define DECLARE_RV_REACTING_HELPERS(name,
> type)							\
> -static char
> REACT_MSG_##name[1024];								\
> -
> 												\
> -static inline char *format_react_msg_##name(type curr_state, type
> event)			\
> -
> {												\
> -	snprintf(REACT_MSG_##name,
> 1024,							\
> -		 "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));					\
> -	return
> REACT_MSG_##name;								\
> -
> }												\
> -
> 												\
> -static void cond_react_##name(char
> *msg)							\
> +static void cond_react_##name(type curr_state, type
> event)					\
>  {								
> 				\
> -	if
> (rv_##name.react)									\
> -
> 		rv_##name.react(msg);								\
> +	if
> (!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));
> 				\
>  }								
> 				\
>  								
> 				\
>  static bool
> rv_reacting_on_##name(void)								\
> @@ -45,12 +37,7 @@ static bool
> rv_reacting_on_##name(void)								\
>  #else /* CONFIG_RV_REACTOR */
>  
>  #define DECLARE_RV_REACTING_HELPERS(name,
> type)							\
> -static inline char *format_react_msg_##name(type curr_state, type
> event)			\
> -
> {												\
> -	return
> NULL;										\
> -
> }												\
> -
> 												\
> -static void cond_react_##name(char
> *msg)							\
> +static void cond_react_##name(type curr_state, type
> event)					\
>  {								
> 				\
>  	return;						
> 					\
>  }								
> 				\
> @@ -171,7 +158,7 @@ da_event_##name(struct da_monitor *da_mon, enum
> events_##name event)				\
>  	}							
> 				\
>  								
> 				\
>  	if
> (rv_reacting_on_##name())								\
> -
> 		cond_react_##name(format_react_msg_##name(curr_state, event));			\
> +		cond_react_##name(curr_state,
> event);						\
>  								
> 				\
>  	trace_error_##name(model_get_state_name_##name(curr_state),
> 				\
>  			  
> model_get_event_name_##name(event));					\
> @@ -203,7 +190,7 @@ static inline bool da_event_##name(struct
> da_monitor *da_mon, struct task_struct
>  	}							
> 				\
>  								
> 				\
>  	if
> (rv_reacting_on_##name())								\
> -
> 		cond_react_##name(format_react_msg_##name(curr_state, event));			\
> +		cond_react_##name(curr_state,
> event);						\
>  								
> 				\
>  	trace_error_##name(tsk-
> >pid,								\
>  			  
> model_get_state_name_##name(curr_state),				\
> 
> Best regards,
> Nam
>