Re: [PATCH 5/5] rv: Add rts monitor

[Nam Cao <namcao@linutronix.de>]

On Wed, Aug 06, 2025 at 10:15:48AM +0200, Gabriele Monaco wrote:
> I didn't make it on time before your V2, I assume you solved already so
> you might ignore this.
> 
> You kinda have something like the da_monitor_enabled: the
> rv_ltl_all_atoms_known
> 
> I wonder if you could define LTL_RT_TASK_ENQUEUED only when you
> actually know it (or are reasonably sure based on your internal
> counter). Or at least not set all atoms until the monitor is fully set
> up.

The rv_ltl_all_atoms_known() thingy is for situation where relevant
tracepoints have not been hit yet.

This case is slightly different, the tracepoint has been hit. And it is not
clear how to implement the "reasonably sure based on your internal counter"
part.

> Anyway reordering the tracepoints registration is likely necessary
> whatever you do, but I'm afraid a problem like this can occur pretty
> often with this type of monitors.

What I have in v2 is a workaround only, by reordering the tracepoint
registrations.

The root problem is not specific to this monitor, but all LTL monitors. My
idea for the real fix is the untested patch below. I will send it
separately. It is not urgent, so I can wait for your DA macro removal patch
to be merged first.

As I'm sending the patch to you, I realized that the patch effectively
nullifies ltl_atoms_init(). So I will need to fix that up..

Nam

commit 7fbb9a99f1a95e5149d476fa3d83a60be1a9a579
Author: Nam Cao <namcao@linutronix.de>
Date:   Tue Aug 5 22:47:49 2025 +0200

    rv: Share the da_monitor_enabled_##name() function with LTL
    
    The LTL monitors also need the functionality that
    da_monitor_enabled_##name() offers.
    
    This is useful to prevent the automaton from being executed before the
    monitor is completely enabled, preventing the situation where the
    monitors run before all tracepoints are registered. This situation can
    cause a false positive error, because the monitors do not see some
    events and do not validate properly.
    
    Pull da_monitor_enabled_##name() to be in the common header, and use
    it for both LTL and DA.
    
    Signed-off-by: Nam Cao <namcao@linutronix.de>

diff --git a/include/linux/rv.h b/include/linux/rv.h
index 1aa01d98e390..8a885b3665a8 100644
--- a/include/linux/rv.h
+++ b/include/linux/rv.h
@@ -119,6 +119,14 @@ int rv_register_monitor(struct rv_monitor *monitor, struct rv_monitor *parent);
 int rv_get_task_monitor_slot(void);
 void rv_put_task_monitor_slot(int slot);
 
+static inline bool rv_monitor_enabled(struct rv_monitor *monitor)
+{
+	if (unlikely(!rv_monitoring_on()))
+		return 0;
+
+	return likely(monitor->enabled);
+}
+
 #ifdef CONFIG_RV_REACTORS
 bool rv_reacting_on(void);
 int rv_unregister_reactor(struct rv_reactor *reactor);
diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h
index 17fa4f6e5ea6..92b8a8c0b9b7 100644
--- a/include/rv/da_monitor.h
+++ b/include/rv/da_monitor.h
@@ -74,29 +74,12 @@ static inline bool da_monitoring_##name(struct da_monitor *da_mon)				\
 	return da_mon->monitoring;								\
 }												\
 												\
-/*												\
- * da_monitor_enabled_##name - checks if the monitor is enabled					\
- */												\
-static inline bool da_monitor_enabled_##name(void)						\
-{												\
-	/* global switch */									\
-	if (unlikely(!rv_monitoring_on()))							\
-		return 0;									\
-												\
-	/* monitor enabled */									\
-	if (unlikely(!rv_##name.enabled))							\
-		return 0;									\
-												\
-	return 1;										\
-}												\
-												\
 /*												\
  * da_monitor_handling_event_##name - checks if the monitor is ready to handle events		\
  */												\
 static inline bool da_monitor_handling_event_##name(struct da_monitor *da_mon)			\
 {												\
-												\
-	if (!da_monitor_enabled_##name())							\
+	if (!rv_monitor_enabled(&rv_##name))							\
 		return 0;									\
 												\
 	/* monitor is actually monitoring */							\
@@ -390,7 +373,7 @@ static inline bool da_handle_start_event_##name(enum events_##name event)			\
 {												\
 	struct da_monitor *da_mon;								\
 												\
-	if (!da_monitor_enabled_##name())							\
+	if (!rv_monitor_enabled(&rv_##name))							\
 		return 0;									\
 												\
 	da_mon = da_get_monitor_##name();							\
@@ -415,7 +398,7 @@ static inline bool da_handle_start_run_event_##name(enum events_##name event)
 {												\
 	struct da_monitor *da_mon;								\
 												\
-	if (!da_monitor_enabled_##name())							\
+	if (!rv_monitor_enabled(&rv_##name))				\
 		return 0;									\
 												\
 	da_mon = da_get_monitor_##name();							\
@@ -475,7 +458,7 @@ da_handle_start_event_##name(struct task_struct *tsk, enum events_##name event)
 {												\
 	struct da_monitor *da_mon;								\
 												\
-	if (!da_monitor_enabled_##name())							\
+	if (!rv_monitor_enabled(&rv_##name))							\
 		return 0;									\
 												\
 	da_mon = da_get_monitor_##name(tsk);							\
@@ -501,7 +484,7 @@ da_handle_start_run_event_##name(struct task_struct *tsk, enum events_##name eve
 {												\
 	struct da_monitor *da_mon;								\
 												\
-	if (!da_monitor_enabled_##name())							\
+	if (!rv_monitor_enabled(&rv_##name))							\
 		return 0;									\
 												\
 	da_mon = da_get_monitor_##name(tsk);							\
diff --git a/include/rv/ltl_monitor.h b/include/rv/ltl_monitor.h
index 29bbf86d1a52..85a3d07a0303 100644
--- a/include/rv/ltl_monitor.h
+++ b/include/rv/ltl_monitor.h
@@ -16,6 +16,8 @@
 #error "Please include $(MODEL_NAME).h generated by rvgen"
 #endif
 
+#define RV_MONITOR_NAME CONCATENATE(rv_, MONITOR_NAME)
+
 #if LTL_MONITOR_TYPE == LTL_TASK_MONITOR
 
 #define TARGET_PRINT_FORMAT "%s[%d]"
@@ -33,7 +35,6 @@ typedef unsigned int monitor_target;
 #endif
 
 #ifdef CONFIG_RV_REACTORS
-#define RV_MONITOR_NAME CONCATENATE(rv_, MONITOR_NAME)
 static struct rv_monitor RV_MONITOR_NAME;
 
 static struct ltl_monitor *ltl_get_monitor(monitor_target target);
@@ -156,6 +157,9 @@ static void ltl_attempt_start(monitor_target target, struct ltl_monitor *mon)
 
 static inline void ltl_atom_set(struct ltl_monitor *mon, enum ltl_atom atom, bool value)
 {
+	if (!rv_monitor_enabled(&RV_MONITOR_NAME))
+		return;
+
 	__clear_bit(atom, mon->unknown_atoms);
 	if (value)
 		__set_bit(atom, mon->atoms);