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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 27 Jul 2022 18:06:26 +0200
From:   Daniel Bristot de Oliveira <bristot@...nel.org>
To:     Tao Zhou <tao.zhou@...ux.dev>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Jonathan Corbet <corbet@....net>,
        Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Will Deacon <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Marco Elver <elver@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Gabriele Paoloni <gpaoloni@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Clark Williams <williams@...hat.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-trace-devel@...r.kernel.org
Subject: Re: [PATCH V7 04/16] rv/include: Add deterministic automata monitor
 definition via C macros

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