Re: [PATCH V7 04/16] rv/include: Add deterministic automata monitor definition via C macros

[Tao Zhou <tao.zhou@linux.dev>]

On Mon, Jul 25, 2022 at 10:11:16PM +0200, Daniel Bristot de Oliveira wrote:

> +/*
> + * Event handler for per_task monitors.
> + */
> +#define DECLARE_DA_MON_MODEL_HANDLER_PER_TASK(name, type)					\
> +												\
> +static inline type da_event_##name(struct da_monitor *da_mon, struct task_struct *tsk,		\

Not sure here the type `type` why not the bool. The return value is ture/false.
I checked the caller of this function, the return value is stored on `int`.

> +				   enum events_##name event)					\
> +{												\
> +	type curr_state = da_monitor_curr_state_##name(da_mon);					\
> +	type next_state = model_get_next_state_##name(curr_state, event);			\
> +												\
> +	if (next_state != INVALID_STATE) {							\
> +		da_monitor_set_state_##name(da_mon, next_state);				\
> +												\
> +		trace_event_##name(tsk->pid,							\
> +				   model_get_state_name_##name(curr_state),			\
> +				   model_get_event_name_##name(event),				\
> +				   model_get_state_name_##name(next_state),			\
> +				   model_is_final_state_##name(next_state));			\
> +												\
> +		return true;									\
> +	}											\
> +												\
> +	if (rv_reacting_on_##name())								\
> +		cond_react_##name(format_react_msg_##name(curr_state, event));			\
> +												\
> +	trace_error_##name(tsk->pid,								\
> +			   model_get_state_name_##name(curr_state),				\
> +			   model_get_event_name_##name(event));					\
> +												\
> +	return false;										\
> +}

[snip]

> +/*
> + * Handle event for implicit monitor: da_get_monitor_##name() will figure out
> + * the monitor.
> + */
> +#define DECLARE_DA_MON_MONITOR_HANDLER_IMPLICIT(name, type)					\
> +												\
> +static inline void __da_handle_event_##name(struct da_monitor *da_mon,				\
> +					    enum events_##name event)				\
> +{												\
> +	int retval;										\
> +												\
> +	retval = da_monitor_handling_event_##name(da_mon);					\
> +	if (!retval)										\
> +		return;										\

I checked the callers of __da_handle_event_##name():
da_handle_event_##name() for all cases need the above check.
da_handle_start_event_##name() for all cases may not need this check.
(this function checked the enable first and the da_monitoring later and if
it is not monitoring it will start monitoring and return, the later event
handler will not be called. Otherwise enable is enabled, da_monitoring is
monitoring)
da_handle_start_run_event_##name() for implicit case may not need this check.
(almost the same with the above, the difference is if da-monitor is not
monitoring, it will start monitoring and not return and do the event handler,
here enable is enabled and da_monitoring is monitoring, if I am not wrong)
So after another(v7) looking at this patch, I realized that this check can
be omited in two cases(all three cases). Just in fuction da_handle_event_##name()
we need to do da_monitor_handling_event_##name().
So I'd write like this:
static inline void __da_handle_event_##name(struct da_monitor *da_mon,				\
					    enum events_##name event)				\
{												\
	int retval;										\
                                                    \
    retval = da_event_##name(da_mon, event);						\
    if (!retval)										\
        da_monitor_reset_##name(da_mon);						\
}												\

static inline void da_handle_event_##name(enum events_##name event)				\
{												\
    struct da_monitor *da_mon = da_get_monitor_##name();					\
	int retval;										\
                                                    \
    retval = da_monitor_handling_event_##name(da_mon);					\
    if (!retval)										\
        return;										\
                                                    \
    __da_handle_event_##name(da_mon, event);						\

}												\

> +												\
> +	retval = da_event_##name(da_mon, event);						\
> +	if (!retval)										\
> +		da_monitor_reset_##name(da_mon);						\
> +}												\
> +												\
> +/*												\
> + * da_handle_event_##name - handle an event							\
> + */												\
> +static inline void da_handle_event_##name(enum events_##name event)				\
> +{												\
> +	struct da_monitor *da_mon = da_get_monitor_##name();					\
> +	__da_handle_event_##name(da_mon, event);						\
> +}												\
> +												\
> +/*												\
> + * da_handle_start_event_##name - start monitoring or handle event				\
> + *												\
> + * This function is used notify the monitor that the system is returning			\

/used/used to/ :-) My wording is not well, sorry for not convenience, Thanks,

> + * to the initial state, so the monitor can start monitoring in the next event.			\
> + * Thus:											\
> + *												\
> + * If the monitor already started, handle the event.						\
> + * If the monitor did not start yet, start the monitor but skip the event.			\
> + */												\
> +static inline bool da_handle_start_event_##name(enum events_##name event)			\
> +{												\
> +	struct da_monitor *da_mon;								\
> +												\
> +	if (!da_monitor_enabled_##name())							\
> +		return 0;									\
> +												\
> +	da_mon = da_get_monitor_##name();							\
> +												\
> +	if (unlikely(!da_monitoring_##name(da_mon))) {						\
> +		da_monitor_start_##name(da_mon);						\
> +		return 0;									\
> +	}											\
> +												\
> +	__da_handle_event_##name(da_mon, event);						\
> +												\
> +	return 1;										\
> +}												\
> +												\
> +/*												\
> + * da_handle_start_run_event_##name - start monitoring and handle event				\
> + *												\
> + * This function is used notify the monitor that the system is in the				\
> + * initial state, so the monitor can start monitoring and handling event.			\
> + */												\
> +static inline bool da_handle_start_run_event_##name(enum events_##name event)			\
> +{												\
> +	struct da_monitor *da_mon;								\
> +												\
> +	if (!da_monitor_enabled_##name())							\
> +		return 0;									\
> +												\
> +	da_mon = da_get_monitor_##name();							\
> +												\
> +	if (unlikely(!da_monitoring_##name(da_mon)))						\
> +		da_monitor_start_##name(da_mon);						\
> +												\
> +	__da_handle_event_##name(da_mon, event);						\
> +												\
> +	return 1;										\
> +}
> +
> +/*
> + * Handle event for per task.
> + */
> +#define DECLARE_DA_MON_MONITOR_HANDLER_PER_TASK(name, type)					\
> +												\
> +static inline void										\
> +__da_handle_event_##name(struct da_monitor *da_mon, struct task_struct *tsk,			\
> +			 enum events_##name event)						\
> +{												\
> +	int retval;										\
> +												\
> +	retval = da_monitor_handling_event_##name(da_mon);					\
> +	if (!retval)										\
> +		return;										\
> +												\
> +	retval = da_event_##name(da_mon, tsk, event);						\
> +	if (!retval)										\
> +		da_monitor_reset_##name(da_mon);						\
> +}												\
> +												\
> +/*												\
> + * da_handle_event_##name - handle an event							\
> + */												\
> +static inline void										\
> +da_handle_event_##name(struct task_struct *tsk, enum events_##name event)			\
> +{												\
> +	struct da_monitor *da_mon = da_get_monitor_##name(tsk);					\
> +	__da_handle_event_##name(da_mon, tsk, event);						\
> +}												\
> +												\
> +/*												\
> + * da_handle_start_event_##name - start monitoring or handle event				\
> + *												\
> + * This function is used notify the monitor that the system is returning			\
> + * to the initial state, so the monitor can start monitoring in the next event.			\
> + * Thus:											\
> + *												\
> + * If the monitor already started, handle the event.						\
> + * If the monitor did not start yet, start the monitor but skip the event.			\
> + */												\
> +static inline bool										\
> +da_handle_start_event_##name(struct task_struct *tsk, enum events_##name event)			\
> +{												\
> +	struct da_monitor *da_mon;								\
> +												\
> +	if (!da_monitor_enabled_##name())							\
> +		return 0;									\
> +												\
> +	da_mon = da_get_monitor_##name(tsk);							\
> +												\
> +	if (unlikely(!da_monitoring_##name(da_mon))) {						\
> +		da_monitor_start_##name(da_mon);						\
> +		return 0;									\
> +	}											\
> +												\
> +	__da_handle_event_##name(da_mon, tsk, event);						\
> +												\
> +	return 1;										\
> +}