All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tao Zhou <tao.zhou@linux.dev>
To: Daniel Bristot de Oliveira <bristot@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Jonathan Corbet <corbet@lwn.net>, Ingo Molnar <mingo@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Marco Elver <elver@google.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Shuah Khan <skhan@linuxfoundation.org>,
	Gabriele Paoloni <gpaoloni@redhat.com>,
	Juri Lelli <juri.lelli@redhat.com>,
	Clark Williams <williams@redhat.com>,
	Randy Dunlap <rdunlap@infradead.org>,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-trace-devel@vger.kernel.org, Tao Zhou <tao.zhou@linux.dev>
Subject: Re: [PATCH V7 04/16] rv/include: Add deterministic automata monitor definition via C macros
Date: Wed, 27 Jul 2022 23:29:30 +0800	[thread overview]
Message-ID: <YuFZ2scVb658mhoq@geo.homenetwork> (raw)
In-Reply-To: <75d14829c5234c2ff43aff744ac41f246b970ed8.1658778484.git.bristot@kernel.org>

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;										\
> +}

  reply	other threads:[~2022-07-27 15:29 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-25 20:11 [PATCH V7 00/16] The Runtime Verification (RV) interface Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 01/16] rv: Add " Daniel Bristot de Oliveira
2022-07-26 16:22   ` Steven Rostedt
2022-07-26 20:00     ` Daniel Bristot de Oliveira
2022-07-26 20:59       ` Steven Rostedt
2022-07-27 13:42         ` Daniel Bristot de Oliveira
2022-07-27 16:10   ` Tao Zhou
2022-07-25 20:11 ` [PATCH V7 02/16] rv: Add runtime reactors interface Daniel Bristot de Oliveira
2022-07-26 16:26   ` Steven Rostedt
2022-07-26 20:06     ` Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 03/16] rv/include: Add helper functions for deterministic automata Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 04/16] rv/include: Add deterministic automata monitor definition via C macros Daniel Bristot de Oliveira
2022-07-27 15:29   ` Tao Zhou [this message]
2022-07-27 16:06     ` Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 05/16] rv/include: Add instrumentation helper functions Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 06/16] Documentation/rv: Add a basic documentation Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 07/16] tools/rv: Add dot2c Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 08/16] Documentation/rv: Add deterministic automaton documentation Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 09/16] tools/rv: Add dot2k Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 10/16] Documentation/rv: Add deterministic automata monitor synthesis documentation Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 11/16] Documentation/rv: Add deterministic automata instrumentation documentation Daniel Bristot de Oliveira
2022-07-26 16:31   ` Steven Rostedt
2022-07-25 20:11 ` [PATCH V7 12/16] rv/monitor: Add the wip monitor skeleton created by dot2k Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 13/16] rv/monitor: Add the wip monitor Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 14/16] rv/monitor: Add the wwnr monitor Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 15/16] rv/reactor: Add the printk reactor Daniel Bristot de Oliveira
2022-07-25 20:11 ` [PATCH V7 16/16] rv/reactor: Add the panic reactor Daniel Bristot de Oliveira

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YuFZ2scVb658mhoq@geo.homenetwork \
    --to=tao.zhou@linux.dev \
    --cc=bristot@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=corbet@lwn.net \
    --cc=dvyukov@google.com \
    --cc=elver@google.com \
    --cc=gpaoloni@redhat.com \
    --cc=juri.lelli@redhat.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=mingo@redhat.com \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rdunlap@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=skhan@linuxfoundation.org \
    --cc=tglx@linutronix.de \
    --cc=will@kernel.org \
    --cc=williams@redhat.com \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.