[RFC PATCH v2 07/12] rv: Adapt the sco monitor to the new set_state

Gabriele Monaco posted 12 patches 9 months ago
There is a newer version of this series
[RFC PATCH v2 07/12] rv: Adapt the sco monitor to the new set_state
Posted by Gabriele Monaco 9 months ago
The sched_set_state tracepoint changed prototype adding a new argument,
this argument can differentiate between an explicit set_state called by
a task and a set state to runnable by the scheduler due to a pending
signal.

Adapt the handlers prototypes for the sco monitor.
Expand the model to handle the new set_state flavour, the monitor was
making sure set state happens only outside of the scheduler, if the
event occurs with the new argument (from_signal) set to true, we instead
expect it to be inside the scheduler.

Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>
---
 Documentation/trace/rv/monitor_sched.rst | 35 +++++++++++++-----------
 kernel/trace/rv/monitors/sco/sco.c       |  8 ++++--
 kernel/trace/rv/monitors/sco/sco.h       |  6 ++--
 tools/verification/models/sched/sco.dot  |  1 +
 4 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/Documentation/trace/rv/monitor_sched.rst b/Documentation/trace/rv/monitor_sched.rst
index 24b2c62a3bc2..6f76bba94d9f 100644
--- a/Documentation/trace/rv/monitor_sched.rst
+++ b/Documentation/trace/rv/monitor_sched.rst
@@ -64,22 +64,25 @@ Monitor sco
 ~~~~~~~~~~~
 
 The scheduling context operations (sco) monitor ensures changes in a task state
-happen only in thread context::
-
-
-                        |
-                        |
-                        v
-    sched_set_state   +------------------+
-  +------------------ |                  |
-  |                   |  thread_context  |
-  +-----------------> |                  | <+
-                      +------------------+  |
-                        |                   |
-                        | schedule_entry    | schedule_exit
-                        v                   |
-                                            |
-                       scheduling_context  -+
+happen only in thread context, the only exception is a special kind of set
+state that occurs if a task about to sleep has a pending signal. This set state
+is not called by the thread but by the scheduler itself::
+
+                                        |
+                                        |
+                                        v
+    sched_set_state                   +------------------+
+  +---------------------------------- |                  |
+  |                                   |  thread_context  |
+  +---------------------------------> |                  | <+
+                                      +------------------+  |
+                                        |                   |
+                                        | schedule_entry    | schedule_exit
+                                        v                   |
+    sched_set_state_runnable_signal                         |
+  +----------------------------------                       |
+  |                                    scheduling_context   |
+  +--------------------------------->                      -+
 
 Monitor snroc
 ~~~~~~~~~~~~~
diff --git a/kernel/trace/rv/monitors/sco/sco.c b/kernel/trace/rv/monitors/sco/sco.c
index 66f4639d46ac..6457ff2469d0 100644
--- a/kernel/trace/rv/monitors/sco/sco.c
+++ b/kernel/trace/rv/monitors/sco/sco.c
@@ -19,9 +19,13 @@
 static struct rv_monitor rv_sco;
 DECLARE_DA_MON_PER_CPU(sco, unsigned char);
 
-static void handle_sched_set_state(void *data, struct task_struct *tsk, int state)
+static void handle_sched_set_state(void *data, struct task_struct *tsk,
+				   int state, bool from_signal)
 {
-	da_handle_start_event_sco(sched_set_state_sco);
+	if (from_signal)
+		da_handle_event_sco(sched_set_state_runnable_signal_sco);
+	else
+		da_handle_start_event_sco(sched_set_state_sco);
 }
 
 static void handle_schedule_entry(void *data, bool preempt, unsigned long ip)
diff --git a/kernel/trace/rv/monitors/sco/sco.h b/kernel/trace/rv/monitors/sco/sco.h
index 7a4c1f2d5ca1..302750687f9c 100644
--- a/kernel/trace/rv/monitors/sco/sco.h
+++ b/kernel/trace/rv/monitors/sco/sco.h
@@ -15,6 +15,7 @@ enum states_sco {
 
 enum events_sco {
 	sched_set_state_sco = 0,
+	sched_set_state_runnable_signal_sco,
 	schedule_entry_sco,
 	schedule_exit_sco,
 	event_max_sco
@@ -35,12 +36,13 @@ static const struct automaton_sco automaton_sco = {
 	},
 	.event_names = {
 		"sched_set_state",
+		"sched_set_state_runnable_signal",
 		"schedule_entry",
 		"schedule_exit"
 	},
 	.function = {
-		{     thread_context_sco, scheduling_context_sco,          INVALID_STATE },
-		{          INVALID_STATE,          INVALID_STATE,     thread_context_sco },
+		{     thread_context_sco,          INVALID_STATE, scheduling_context_sco,          INVALID_STATE },
+		{          INVALID_STATE, scheduling_context_sco,          INVALID_STATE,     thread_context_sco },
 	},
 	.initial_state = thread_context_sco,
 	.final_states = { 1, 0 },
diff --git a/tools/verification/models/sched/sco.dot b/tools/verification/models/sched/sco.dot
index 20b0e3b449a6..4e44ed58c62a 100644
--- a/tools/verification/models/sched/sco.dot
+++ b/tools/verification/models/sched/sco.dot
@@ -7,6 +7,7 @@ digraph state_automaton {
 	{node [shape = plaintext] "thread_context"};
 	"__init_thread_context" -> "thread_context";
 	"scheduling_context" [label = "scheduling_context"];
+	"scheduling_context" -> "scheduling_context" [ label = "sched_set_state_runnable_signal" ];
 	"scheduling_context" -> "thread_context" [ label = "schedule_exit" ];
 	"thread_context" [label = "thread_context", color = green3];
 	"thread_context" -> "scheduling_context" [ label = "schedule_entry" ];
-- 
2.49.0
Re: [RFC PATCH v2 07/12] rv: Adapt the sco monitor to the new set_state
Posted by Nam Cao 8 months, 3 weeks ago
On Wed, May 14, 2025 at 10:43:09AM +0200, Gabriele Monaco wrote:
>  	.function = {
> -		{     thread_context_sco, scheduling_context_sco,          INVALID_STATE },
> -		{          INVALID_STATE,          INVALID_STATE,     thread_context_sco },
> +		{     thread_context_sco,          INVALID_STATE, scheduling_context_sco,          INVALID_STATE },
> +		{          INVALID_STATE, scheduling_context_sco,          INVALID_STATE,     thread_context_sco },

This is over the 100 column limit.

I know it is not your fault, this is generated. Back when I was playing
with DA monitor, I made a patch to fix this. Maybe you could include it in
your series?

From b4fb648398a29a9c0d8e08bd12394978d3948a5e Mon Sep 17 00:00:00 2001
From: Nam Cao <namcao@linutronix.de>
Date: Fri, 15 Nov 2024 14:56:33 +0100
Subject: [PATCH] tools/rv/dot2c: Fix generated files going over 100 column
 limit

The dot2c.py script generates all states in a single line. This breaks the
100 column limit when the state machines are non-trivial.

Change dot2c.py to generate the states in separate lines.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 tools/verification/rvgen/rvgen/dot2c.py | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/tools/verification/rvgen/rvgen/dot2c.py b/tools/verification/rvgen/rvgen/dot2c.py
index 6009caf568d9..abc0ee569b34 100644
--- a/tools/verification/rvgen/rvgen/dot2c.py
+++ b/tools/verification/rvgen/rvgen/dot2c.py
@@ -152,29 +152,22 @@ class Dot2c(Automata):
         max_state_name = max(self.states, key = len).__len__()
         return max(max_state_name, self.invalid_state_str.__len__())
 
-    def __get_state_string_length(self):
-        maxlen = self.__get_max_strlen_of_states() + self.enum_suffix.__len__()
-        return "%" + str(maxlen) + "s"
-
     def get_aut_init_function(self):
         nr_states = self.states.__len__()
         nr_events = self.events.__len__()
         buff = []
 
-        strformat = self.__get_state_string_length()
-
         for x in range(nr_states):
-            line = "\t\t{ "
+            buff.append("\t\t{")
             for y in range(nr_events):
                 next_state = self.function[x][y]
                 if next_state != self.invalid_state_str:
                     next_state = self.function[x][y] + self.enum_suffix
 
                 if y != nr_events-1:
-                    line = line + strformat % next_state + ", "
+                    buff.append(''.join(("\t\t\t", next_state, ",")))
                 else:
-                    line = line + strformat % next_state + " },"
-            buff.append(line)
+                    buff.append(''.join(("\t\t\t", next_state, "\n\t\t},")))
 
         return self.__buff_to_string(buff)
 
-- 
2.39.5


Best regards,
Nam
Re: [RFC PATCH v2 07/12] rv: Adapt the sco monitor to the new set_state
Posted by Gabriele Monaco 8 months, 3 weeks ago

On Mon, 2025-05-19 at 10:42 +0200, Nam Cao wrote:
> On Wed, May 14, 2025 at 10:43:09AM +0200, Gabriele Monaco wrote:
> >  	.function = {
> > -		{     thread_context_sco,
> > scheduling_context_sco,          INVALID_STATE },
> > -		{          INVALID_STATE,         
> > INVALID_STATE,     thread_context_sco },
> > +		{     thread_context_sco,          INVALID_STATE,
> > scheduling_context_sco,          INVALID_STATE },
> > +		{          INVALID_STATE,
> > scheduling_context_sco,          INVALID_STATE,    
> > thread_context_sco },
> 
> This is over the 100 column limit.
> 
> I know it is not your fault, this is generated. Back when I was
> playing
> with DA monitor, I made a patch to fix this. Maybe you could include
> it in
> your series?
> 
> From b4fb648398a29a9c0d8e08bd12394978d3948a5e Mon Sep 17 00:00:00
> 2001
> From: Nam Cao <namcao@linutronix.de>
> Date: Fri, 15 Nov 2024 14:56:33 +0100
> Subject: [PATCH] tools/rv/dot2c: Fix generated files going over 100
> column
>  limit
> 
> The dot2c.py script generates all states in a single line. This
> breaks the
> 100 column limit when the state machines are non-trivial.
> 
> Change dot2c.py to generate the states in separate lines.
> 
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> ---
>  tools/verification/rvgen/rvgen/dot2c.py | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/verification/rvgen/rvgen/dot2c.py
> b/tools/verification/rvgen/rvgen/dot2c.py
> index 6009caf568d9..abc0ee569b34 100644
> --- a/tools/verification/rvgen/rvgen/dot2c.py
> +++ b/tools/verification/rvgen/rvgen/dot2c.py
> @@ -152,29 +152,22 @@ class Dot2c(Automata):
>          max_state_name = max(self.states, key = len).__len__()
>          return max(max_state_name, self.invalid_state_str.__len__())
>  
> -    def __get_state_string_length(self):
> -        maxlen = self.__get_max_strlen_of_states() +
> self.enum_suffix.__len__()
> -        return "%" + str(maxlen) + "s"
> -
>      def get_aut_init_function(self):
>          nr_states = self.states.__len__()
>          nr_events = self.events.__len__()
>          buff = []
>  
> -        strformat = self.__get_state_string_length()
> -
>          for x in range(nr_states):
> -            line = "\t\t{ "
> +            buff.append("\t\t{")
>              for y in range(nr_events):
>                  next_state = self.function[x][y]
>                  if next_state != self.invalid_state_str:
>                      next_state = self.function[x][y] +
> self.enum_suffix
>  
>                  if y != nr_events-1:
> -                    line = line + strformat % next_state + ", "
> +                    buff.append(''.join(("\t\t\t", next_state,
> ",")))
>                  else:
> -                    line = line + strformat % next_state + " },"
> -            buff.append(line)
> +                    buff.append(''.join(("\t\t\t", next_state,
> "\n\t\t},")))
>  
>          return self.__buff_to_string(buff)
>  

Thanks for bringing this up, I'm a bit undecided on this one..

The nice thing of the current representation is that it shows a matrix
and it's relatively easy to see what each event does.
On the other hand it's true larger models do exceed quite a bit the
size limits and considering you aren't really supposed to touch this
file directly (as the script does it for you), perhaps cleaner C code
should be the priority.

I'll play with your patch and see if it negatively affects the workflow
in any way. If not, I'd include it and adapt the monitors (perhaps only
those with long lines, not really need to change all).

Thanks,
Gabriele