Re: [PATCH V4 10/20] rv/monitor: Add the wwnr monitor skeleton created by dot2k

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>,
	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 V4 10/20] rv/monitor: Add the wwnr monitor skeleton created by dot2k
Date: Thu, 7 Jul 2022 04:08:35 +0800	[thread overview]
Message-ID: <YsXrw40DP2ckPHuC@geo.homenetwork> (raw)
In-Reply-To: <7d1b2238aa5c805501ae3dc80b213d06f6588cb3.1655368610.git.bristot@kernel.org>

On Thu, Jun 16, 2022 at 10:44:52AM +0200, Daniel Bristot de Oliveira wrote:

> Per task wakeup while not running (wwnr) monitor, generated by dot2k
> with this command line:
> 
>   $ dot2k -d wwnr.dot -t per_task
> 
> The model is:
>  ----- %< -----
> digraph state_automaton {
> 	center = true;
> 	size = "7,11";
> 	{node [shape = plaintext, style=invis, label=""] "__init_not_running"};
> 	{node [shape = ellipse] "not_running"};
> 	{node [shape = plaintext] "not_running"};
> 	{node [shape = plaintext] "running"};
> 	"__init_not_running" -> "not_running";
> 	"not_running" [label = "not_running", color = green3];
> 	"not_running" -> "not_running" [ label = "wakeup" ];
> 	"not_running" -> "running" [ label = "switch_in" ];
> 	"running" [label = "running"];
> 	"running" -> "not_running" [ label = "switch_out" ];
> 	{ rank = min ;
> 		"__init_not_running";
> 		"not_running";
> 	}
> }
>  ----- >% -----
> 
> This model is broken, the reason is that a task can be running in the
> processor without being set as RUNNABLE. Think about a task about to
> sleep:
> 
> 1:	set_current_state(TASK_UNINTERRUPTIBLE);
> 2:	schedule();
> 
> And then imagine an IRQ happening in between the lines one and two,
> waking the task up. BOOM, the wakeup will happen while the task is
> running.
> 
> Q: Why do we need this model, so?
> A: To test the reactors.
> 
> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marco Elver <elver@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> Cc: Gabriele Paoloni <gpaoloni@redhat.com>
> Cc: Juri Lelli <juri.lelli@redhat.com>
> Cc: Clark Williams <williams@redhat.com>
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-trace-devel@vger.kernel.org
> Signed-off-by: Daniel Bristot de Oliveira <bristot@kernel.org>
> ---
>  kernel/trace/rv/monitors/wwnr/wwnr.c | 115 +++++++++++++++++++++++++++
>  kernel/trace/rv/monitors/wwnr/wwnr.h |  38 +++++++++
>  2 files changed, 153 insertions(+)
>  create mode 100644 kernel/trace/rv/monitors/wwnr/wwnr.c
>  create mode 100644 kernel/trace/rv/monitors/wwnr/wwnr.h
> 
> diff --git a/kernel/trace/rv/monitors/wwnr/wwnr.c b/kernel/trace/rv/monitors/wwnr/wwnr.c
> new file mode 100644
> index 000000000000..8ba01f0f0df8
> --- /dev/null
> +++ b/kernel/trace/rv/monitors/wwnr/wwnr.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/ftrace.h>
> +#include <linux/tracepoint.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/rv.h>
> +#include <rv/instrumentation.h>
> +#include <rv/da_monitor.h>
> +
> +#define MODULE_NAME "wwnr"
> +
> +/*
> + * XXX: include required tracepoint headers, e.g.,
> + * #include <linux/trace/events/sched.h>
> + */
> +#include <trace/events/rv.h>
> +
> +/*
> + * This is the self-generated part of the monitor. Generally, there is no need
> + * to touch this section.
> + */
> +#include "wwnr.h"
> +
> +/*
> + * Declare the deterministic automata monitor.
> + *
> + * The rv monitor reference is needed for the monitor declaration.
> + */
> +struct rv_monitor rv_wwnr;
> +DECLARE_DA_MON_PER_TASK(wwnr, char);
> +
> +/*
> + * This is the instrumentation part of the monitor.
> + *
> + * This is the section where manual work is required. Here the kernel events
> + * are translated into model's event.
> + *
> + */
> +static void handle_switch_in(void *data, /* XXX: fill header */)
> +{
> +	struct task_struct *p = /* XXX: how do I get p? */;
> +	da_handle_event_wwnr(p, switch_in_wwnr);
> +}
> +
> +static void handle_switch_out(void *data, /* XXX: fill header */)
> +{
> +	struct task_struct *p = /* XXX: how do I get p? */;
> +	da_handle_event_wwnr(p, switch_out_wwnr);
> +}
> +
> +static void handle_wakeup(void *data, /* XXX: fill header */)
> +{
> +	struct task_struct *p = /* XXX: how do I get p? */;
> +	da_handle_event_wwnr(p, wakeup_wwnr);
> +}
> +
> +static int start_wwnr(void)
> +{
> +	int retval;
> +
> +	retval = da_monitor_init_wwnr();
> +	if (retval)
> +		return retval;
> +
> +	rv_attach_trace_probe("wwnr", /* XXX: tracepoint */, handle_switch_in);
> +	rv_attach_trace_probe("wwnr", /* XXX: tracepoint */, handle_switch_out);
> +	rv_attach_trace_probe("wwnr", /* XXX: tracepoint */, handle_wakeup);
> +
> +	return 0;
> +}
> +
> +static void stop_wwnr(void)
> +{
> +	rv_wwnr.enabled = 0;
> +
> +	rv_detach_trace_probe("wwnr", /* XXX: tracepoint */, handle_switch_in);
> +	rv_detach_trace_probe("wwnr", /* XXX: tracepoint */, handle_switch_out);
> +	rv_detach_trace_probe("wwnr", /* XXX: tracepoint */, handle_wakeup);
> +
> +	da_monitor_destroy_wwnr();
> +}
> +
> +/*
> + * This is the monitor register section.
> + */
> +struct rv_monitor rv_wwnr = {
> +	.name = "wwnr",
> +	.description = "auto-generated wwnr",
> +	.start = start_wwnr,
> +	.stop = stop_wwnr,
> +	.reset = da_monitor_reset_all_wwnr,
> +	.enabled = 0,
> +};
> +
> +int register_wwnr(void)
> +{
> +	rv_register_monitor(&rv_wwnr);
> +	return 0;
> +}
> +
> +void unregister_wwnr(void)
> +{
> +	if (rv_wwnr.enabled)
> +		stop_wwnr();
> +
> +	rv_unregister_monitor(&rv_wwnr);
> +}
> +
> +module_init(register_wwnr);
> +module_exit(unregister_wwnr);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("dot2k: auto-generated");
> +MODULE_DESCRIPTION("wwnr");
> diff --git a/kernel/trace/rv/monitors/wwnr/wwnr.h b/kernel/trace/rv/monitors/wwnr/wwnr.h
> new file mode 100644
> index 000000000000..f3dc160642bf
> --- /dev/null
> +++ b/kernel/trace/rv/monitors/wwnr/wwnr.h
> @@ -0,0 +1,38 @@
> +enum states_wwnr {
> +	not_running_wwnr = 0,
> +	running_wwnr,
> +	state_max_wwnr
> +};
> +
> +enum events_wwnr {
> +	switch_in_wwnr = 0,
> +	switch_out_wwnr,
> +	wakeup_wwnr,
> +	event_max_wwnr
> +};
> +
> +struct automaton_wwnr {
> +	char *state_names[state_max_wwnr];
> +	char *event_names[event_max_wwnr];
> +	char function[state_max_wwnr][event_max_wwnr];
> +	char initial_state;
> +	char final_states[state_max_wwnr];
> +};
> +
> +struct automaton_wwnr automaton_wwnr = {
> +	.state_names = {
> +		"not_running",
> +		"running"
> +	},
> +	.event_names = {
> +		"switch_in",
> +		"switch_out",
> +		"wakeup"
> +	},

The .state_names and .event_names lack the wwnr postfix.
If they generated aotomatically, something need to fix,
if not, can manually change to the consistent name.

If the state change from running to non-running(initial state),
in the next patch in handle_switch() where there checked
TASK_INTERRUPTABLE to set initial state. Why can not check
TASK_UNINTERURPTABLE to set initial state there, confused.
This model is not completed for me now.

> +	.function = {
> +		{     running_wwnr,               -1, not_running_wwnr },
> +		{               -1, not_running_wwnr,               -1 },
> +	},
> +	.initial_state = not_running_wwnr,
> +	.final_states = { 1, 0 },
> +};
> -- 
> 2.35.1
> 