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

[Daniel Bristot de Oliveira <bristot@kernel.org>]

On 7/27/22 17:29, Tao Zhou wrote:
>> +/*
>> + * 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);						\
> 
> }												\
> 

IOW,

The code is checking twice if the monitor is enabled in these two cases:
	- da_handle_start_run_event_##name()
	- da_handle_start_event_##name()

Because it is checking in these functions first and then again in __da_handle_event_##name().

The function da_handle_event_##name() is not checking if the monitors are enabled because
__da_handle_event_##name() does it.

By adding the check on da_handle_event_##name(), we can remove it in
__da_handle_event_##name(). Making the check run only once for all cases.

This is an optimization, and it makes sense.

(changed return value to bool)

-- Daniel