[PATCH 0/3] rv: Add explicit lockdep context for reactors

[PATCH 0/3] rv: Add explicit lockdep context for reactors

[Thomas Weißschuh]

Reactors can be called from any context through tracepoints.
When developing reactors care needs to be taken to only call APIs which
are safe. As the tracepoints used during testing may not actually be
called from restrictive contexts lockdep may not be helpful.

Add explicit overrides to help lockdep find invalid code patterns.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
Thomas Weißschuh (3):
      rv: Pass va_list to reactors
      rv: Make rv_reacting_on() static
      rv: Add explicit lockdep context for reactors

 include/linux/rv.h               | 11 ++++++-----
 include/rv/da_monitor.h          | 35 ++++++++++-------------------------
 include/rv/ltl_monitor.h         | 18 +++++-------------
 kernel/trace/rv/reactor_panic.c  |  6 +-----
 kernel/trace/rv/reactor_printk.c |  6 +-----
 kernel/trace/rv/rv_reactors.c    | 22 ++++++++++++++++++++--
 6 files changed, 43 insertions(+), 55 deletions(-)
---
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
change-id: 20251013-rv-lockdep-6acdc510260a

Best regards,
-- 
Thomas Weißschuh <thomas.weissschuh@linutronix.de>

[PATCH 1/3] rv: Pass va_list to reactors

[Thomas Weißschuh]

The only thing the reactors can do with the passed in varargs is to
convert it into a va_list. Do that in a central helper instead.
It simplifies the reactors, removes some hairy macro-generated code
and introduces a convenient hook point to modify reactor behavior.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 include/linux/rv.h               | 11 +++++++++--
 include/rv/da_monitor.h          | 35 ++++++++++-------------------------
 include/rv/ltl_monitor.h         | 18 +++++-------------
 kernel/trace/rv/reactor_panic.c  |  6 +-----
 kernel/trace/rv/reactor_printk.c |  6 +-----
 kernel/trace/rv/rv_reactors.c    | 16 +++++++++++++++-
 6 files changed, 41 insertions(+), 51 deletions(-)

diff --git a/include/linux/rv.h b/include/linux/rv.h
index 9520aab34bcbe36c730bc6ab20bed71c8eee52bf..b567b0191e67f7dfab74e2aad6de3ed63d94058d 100644
--- a/include/linux/rv.h
+++ b/include/linux/rv.h
@@ -88,7 +88,7 @@ union rv_task_monitor {
 struct rv_reactor {
 	const char		*name;
 	const char		*description;
-	__printf(1, 2) void	(*react)(const char *msg, ...);
+	__printf(1, 0) void	(*react)(const char *msg, va_list args);
 	struct list_head	list;
 };
 #endif
@@ -102,7 +102,7 @@ struct rv_monitor {
 	void			(*reset)(void);
 #ifdef CONFIG_RV_REACTORS
 	struct rv_reactor	*reactor;
-	__printf(1, 2) void	(*react)(const char *msg, ...);
+	__printf(1, 0) void	(*react)(const char *msg, va_list args);
 #endif
 	struct list_head	list;
 	struct rv_monitor	*parent;
@@ -119,11 +119,18 @@ void rv_put_task_monitor_slot(int slot);
 bool rv_reacting_on(void);
 int rv_unregister_reactor(struct rv_reactor *reactor);
 int rv_register_reactor(struct rv_reactor *reactor);
+__printf(2, 3)
+void rv_react(struct rv_monitor *monitor, const char *msg, ...);
 #else
 static inline bool rv_reacting_on(void)
 {
 	return false;
 }
+
+__printf(2, 3)
+static inline void rv_react(struct rv_monitor *monitor, const char *msg, ...)
+{
+}
 #endif /* CONFIG_RV_REACTORS */
 
 #endif /* CONFIG_RV */
diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h
index 17fa4f6e5ea63fad29fd4349c54720944a179e38..0cef64366538c90dab8f76fbf5d2aaacdd61e2e7 100644
--- a/include/rv/da_monitor.h
+++ b/include/rv/da_monitor.h
@@ -16,34 +16,19 @@
 #include <linux/bug.h>
 #include <linux/sched.h>
 
-#ifdef CONFIG_RV_REACTORS
-
-#define DECLARE_RV_REACTING_HELPERS(name, type)							\
-static void cond_react_##name(type curr_state, type event)					\
-{												\
-	if (!rv_reacting_on() || !rv_##name.react)						\
-		return;										\
-	rv_##name.react("rv: monitor %s does not allow event %s on state %s\n",			\
-			#name,									\
-			model_get_event_name_##name(event),					\
-			model_get_state_name_##name(curr_state));				\
-}
-
-#else /* CONFIG_RV_REACTOR */
-
-#define DECLARE_RV_REACTING_HELPERS(name, type)							\
-static void cond_react_##name(type curr_state, type event)					\
-{												\
-	return;											\
-}
-#endif
-
 /*
  * Generic helpers for all types of deterministic automata monitors.
  */
 #define DECLARE_DA_MON_GENERIC_HELPERS(name, type)						\
 												\
-DECLARE_RV_REACTING_HELPERS(name, type)								\
+static void react_##name(type curr_state, type event)						\
+{												\
+	rv_react(&rv_##name,									\
+		 "rv: monitor %s does not allow event %s on state %s\n",			\
+		 #name,										\
+		 model_get_event_name_##name(event),						\
+		 model_get_state_name_##name(curr_state));					\
+}												\
 												\
 /*												\
  * da_monitor_reset_##name - reset a monitor and setting it to init state			\
@@ -126,7 +111,7 @@ da_event_##name(struct da_monitor *da_mon, enum events_##name event)				\
 	for (int i = 0; i < MAX_DA_RETRY_RACING_EVENTS; i++) {					\
 		next_state = model_get_next_state_##name(curr_state, event);			\
 		if (next_state == INVALID_STATE) {						\
-			cond_react_##name(curr_state, event);					\
+			react_##name(curr_state, event);					\
 			trace_error_##name(model_get_state_name_##name(curr_state),		\
 					   model_get_event_name_##name(event));			\
 			return false;								\
@@ -165,7 +150,7 @@ static inline bool da_event_##name(struct da_monitor *da_mon, struct task_struct
 	for (int i = 0; i < MAX_DA_RETRY_RACING_EVENTS; i++) {					\
 		next_state = model_get_next_state_##name(curr_state, event);			\
 		if (next_state == INVALID_STATE) {						\
-			cond_react_##name(curr_state, event);					\
+			react_##name(curr_state, event);					\
 			trace_error_##name(tsk->pid,						\
 					   model_get_state_name_##name(curr_state),		\
 					   model_get_event_name_##name(event));			\
diff --git a/include/rv/ltl_monitor.h b/include/rv/ltl_monitor.h
index 5368cf5fd623e74a5739d2e0b3fc2c27c4bad597..00c42b36f961a00ee473aa58f14b015308523eb0 100644
--- a/include/rv/ltl_monitor.h
+++ b/include/rv/ltl_monitor.h
@@ -16,21 +16,12 @@
 #error "Please include $(MODEL_NAME).h generated by rvgen"
 #endif
 
-#ifdef CONFIG_RV_REACTORS
 #define RV_MONITOR_NAME CONCATENATE(rv_, MONITOR_NAME)
-static struct rv_monitor RV_MONITOR_NAME;
 
-static void rv_cond_react(struct task_struct *task)
-{
-	if (!rv_reacting_on() || !RV_MONITOR_NAME.react)
-		return;
-	RV_MONITOR_NAME.react("rv: "__stringify(MONITOR_NAME)": %s[%d]: violation detected\n",
-			      task->comm, task->pid);
-}
+#ifdef CONFIG_RV_REACTORS
+static struct rv_monitor RV_MONITOR_NAME;
 #else
-static void rv_cond_react(struct task_struct *task)
-{
-}
+extern struct rv_monitor RV_MONITOR_NAME;
 #endif
 
 static int ltl_monitor_slot = RV_PER_TASK_MONITOR_INIT;
@@ -98,7 +89,8 @@ static void ltl_monitor_destroy(void)
 static void ltl_illegal_state(struct task_struct *task, struct ltl_monitor *mon)
 {
 	CONCATENATE(trace_error_, MONITOR_NAME)(task);
-	rv_cond_react(task);
+	rv_react(&RV_MONITOR_NAME, "rv: "__stringify(MONITOR_NAME)": %s[%d]: violation detected\n",
+		 task->comm, task->pid);
 }
 
 static void ltl_attempt_start(struct task_struct *task, struct ltl_monitor *mon)
diff --git a/kernel/trace/rv/reactor_panic.c b/kernel/trace/rv/reactor_panic.c
index 74c6bcc2c7494408770881dda2b0de885c5eb88c..76537b8a4343cbd0d715f60db3cc8868e71c1c0b 100644
--- a/kernel/trace/rv/reactor_panic.c
+++ b/kernel/trace/rv/reactor_panic.c
@@ -13,13 +13,9 @@
 #include <linux/init.h>
 #include <linux/rv.h>
 
-__printf(1, 2) static void rv_panic_reaction(const char *msg, ...)
+__printf(1, 0) static void rv_panic_reaction(const char *msg, va_list args)
 {
-	va_list args;
-
-	va_start(args, msg);
 	vpanic(msg, args);
-	va_end(args);
 }
 
 static struct rv_reactor rv_panic = {
diff --git a/kernel/trace/rv/reactor_printk.c b/kernel/trace/rv/reactor_printk.c
index 2dae2916c05fd17397195e37d9b42d24cea24b9c..48c934e315b31c14d3a5b4fa3ec334ef71f9e390 100644
--- a/kernel/trace/rv/reactor_printk.c
+++ b/kernel/trace/rv/reactor_printk.c
@@ -12,13 +12,9 @@
 #include <linux/init.h>
 #include <linux/rv.h>
 
-__printf(1, 2) static void rv_printk_reaction(const char *msg, ...)
+__printf(1, 0) static void rv_printk_reaction(const char *msg, va_list args)
 {
-	va_list args;
-
-	va_start(args, msg);
 	vprintk_deferred(msg, args);
-	va_end(args);
 }
 
 static struct rv_reactor rv_printk = {
diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index d32859fec238371b5721e08cf6558f0805565f7b..cb1a5968055abb22439a066b4e25dad98f078389 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -438,7 +438,7 @@ int reactor_populate_monitor(struct rv_monitor *mon)
 /*
  * Nop reactor register
  */
-__printf(1, 2) static void rv_nop_reaction(const char *msg, ...)
+__printf(1, 0) static void rv_nop_reaction(const char *msg, va_list args)
 {
 }
 
@@ -477,3 +477,17 @@ int init_rv_reactors(struct dentry *root_dir)
 out_err:
 	return -ENOMEM;
 }
+
+void rv_react(struct rv_monitor *monitor, const char *msg, ...)
+{
+	va_list args;
+
+	if (!rv_reacting_on() || !monitor->react)
+		return;
+
+	va_start(args, msg);
+
+	monitor->react(msg, args);
+
+	va_end(args);
+}

-- 
2.51.0

Re: [PATCH 1/3] rv: Pass va_list to reactors

[Gabriele Monaco]

On Tue, 2025-10-14 at 07:51 +0200, Thomas Weißschuh wrote:
> The only thing the reactors can do with the passed in varargs is to
> convert it into a va_list. Do that in a central helper instead.
> It simplifies the reactors, removes some hairy macro-generated code
> and introduces a convenient hook point to modify reactor behavior.
> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
> --- a/include/rv/da_monitor.h
> +++ b/include/rv/da_monitor.h
> @@ -16,34 +16,19 @@
>  #include <linux/bug.h>
>  #include <linux/sched.h>
>  
> -#ifdef CONFIG_RV_REACTORS
> -
> -#define DECLARE_RV_REACTING_HELPERS(name,
> type)							\
> -static void cond_react_##name(type curr_state, type
> event)					\
> -
> {												\
> -	if (!rv_reacting_on() ||
> !rv_##name.react)						\
> -
> 		return;										\
> -	rv_##name.react("rv: monitor %s does not allow event %s on state
> %s\n",			\
> -
> 			#name,									\
> -
> 			model_get_event_name_##name(event),					\
> -
> 			model_get_state_name_##name(curr_state));				\
> -}
> -

The overall change looks good to me, just keep in mind there is an ongoing
refactor [1] for the da_monitor header to get rid of those macros, basically
making it more similar to the current ltl.
Depending on which gets merged first, you may need some (trivial) adaptation of
this.

Going to test it out more carefully in the next days.

Thanks,
Gabriele

[1] - https://lore.kernel.org/lkml/20250919140954.104920-1-gmonaco@redhat.com

> -#else /* CONFIG_RV_REACTOR */
> -
> -#define DECLARE_RV_REACTING_HELPERS(name,
> type)							\
> -static void cond_react_##name(type curr_state, type
> event)					\
> -
> {												\
> -
> 	return;											\
> -}
> -#endif
> -
>  /*
>   * Generic helpers for all types of deterministic automata monitors.
>   */
>  #define DECLARE_DA_MON_GENERIC_HELPERS(name,
> type)						\
>  									
> 			\
> -DECLARE_RV_REACTING_HELPERS(name,
> type)								\
> +static void react_##name(type curr_state, type
> event)						\
> +{									
> 			\
> +	rv_react(&rv_##name,						
> 			\
> +		 "rv: monitor %s does not allow event %s on state
> %s\n",			\
> +		
> #name,										\
> +		
> model_get_event_name_##name(event),						\
> +		
> model_get_state_name_##name(curr_state));					\
> +}									
> 			\
>  									
> 			\
>  /*									
> 			\
>   * da_monitor_reset_##name - reset a monitor and setting it to init
> state			\
> @@ -126,7 +111,7 @@ da_event_##name(struct da_monitor *da_mon, enum
> events_##name event)				\
>  	for (int i = 0; i < MAX_DA_RETRY_RACING_EVENTS; i++)
> {					\
>  		next_state = model_get_next_state_##name(curr_state,
> event);			\
>  		if (next_state == INVALID_STATE)
> {						\
> -			cond_react_##name(curr_state,
> event);					\
> +			react_##name(curr_state,
> event);					\
>  			trace_error_##name(model_get_state_name_##name(curr_s
> tate),		\
>  					  
> model_get_event_name_##name(event));			\
>  			return
> false;								\
> @@ -165,7 +150,7 @@ static inline bool da_event_##name(struct da_monitor
> *da_mon, struct task_struct
>  	for (int i = 0; i < MAX_DA_RETRY_RACING_EVENTS; i++)
> {					\
>  		next_state = model_get_next_state_##name(curr_state,
> event);			\
>  		if (next_state == INVALID_STATE)
> {						\
> -			cond_react_##name(curr_state,
> event);					\
> +			react_##name(curr_state,
> event);					\
>  			trace_error_##name(tsk-
> >pid,						\
>  					  
> model_get_state_name_##name(curr_state),		\
>  					  
> model_get_event_name_##name(event));			\
> diff --git a/include/rv/ltl_monitor.h b/include/rv/ltl_monitor.h
> index
> 5368cf5fd623e74a5739d2e0b3fc2c27c4bad597..00c42b36f961a00ee473aa58f14b01530852
> 3eb0 100644
> --- a/include/rv/ltl_monitor.h
> +++ b/include/rv/ltl_monitor.h
> @@ -16,21 +16,12 @@
>  #error "Please include $(MODEL_NAME).h generated by rvgen"
>  #endif
>  
> -#ifdef CONFIG_RV_REACTORS
>  #define RV_MONITOR_NAME CONCATENATE(rv_, MONITOR_NAME)
> -static struct rv_monitor RV_MONITOR_NAME;
>  
> -static void rv_cond_react(struct task_struct *task)
> -{
> -	if (!rv_reacting_on() || !RV_MONITOR_NAME.react)
> -		return;
> -	RV_MONITOR_NAME.react("rv: "__stringify(MONITOR_NAME)": %s[%d]:
> violation detected\n",
> -			      task->comm, task->pid);
> -}
> +#ifdef CONFIG_RV_REACTORS
> +static struct rv_monitor RV_MONITOR_NAME;
>  #else
> -static void rv_cond_react(struct task_struct *task)
> -{
> -}
> +extern struct rv_monitor RV_MONITOR_NAME;
>  #endif
>  
>  static int ltl_monitor_slot = RV_PER_TASK_MONITOR_INIT;
> @@ -98,7 +89,8 @@ static void ltl_monitor_destroy(void)
>  static void ltl_illegal_state(struct task_struct *task, struct ltl_monitor
> *mon)
>  {
>  	CONCATENATE(trace_error_, MONITOR_NAME)(task);
> -	rv_cond_react(task);
> +	rv_react(&RV_MONITOR_NAME, "rv: "__stringify(MONITOR_NAME)": %s[%d]:
> violation detected\n",
> +		 task->comm, task->pid);
>  }
>  
>  static void ltl_attempt_start(struct task_struct *task, struct ltl_monitor
> *mon)
> diff --git a/kernel/trace/rv/reactor_panic.c b/kernel/trace/rv/reactor_panic.c
> index
> 74c6bcc2c7494408770881dda2b0de885c5eb88c..76537b8a4343cbd0d715f60db3cc8868e71c
> 1c0b 100644
> --- a/kernel/trace/rv/reactor_panic.c
> +++ b/kernel/trace/rv/reactor_panic.c
> @@ -13,13 +13,9 @@
>  #include <linux/init.h>
>  #include <linux/rv.h>
>  
> -__printf(1, 2) static void rv_panic_reaction(const char *msg, ...)
> +__printf(1, 0) static void rv_panic_reaction(const char *msg, va_list args)
>  {
> -	va_list args;
> -
> -	va_start(args, msg);
>  	vpanic(msg, args);
> -	va_end(args);
>  }
>  
>  static struct rv_reactor rv_panic = {
> diff --git a/kernel/trace/rv/reactor_printk.c
> b/kernel/trace/rv/reactor_printk.c
> index
> 2dae2916c05fd17397195e37d9b42d24cea24b9c..48c934e315b31c14d3a5b4fa3ec334ef71f9
> e390 100644
> --- a/kernel/trace/rv/reactor_printk.c
> +++ b/kernel/trace/rv/reactor_printk.c
> @@ -12,13 +12,9 @@
>  #include <linux/init.h>
>  #include <linux/rv.h>
>  
> -__printf(1, 2) static void rv_printk_reaction(const char *msg, ...)
> +__printf(1, 0) static void rv_printk_reaction(const char *msg, va_list args)
>  {
> -	va_list args;
> -
> -	va_start(args, msg);
>  	vprintk_deferred(msg, args);
> -	va_end(args);
>  }
>  
>  static struct rv_reactor rv_printk = {
> diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
> index
> d32859fec238371b5721e08cf6558f0805565f7b..cb1a5968055abb22439a066b4e25dad98f07
> 8389 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -438,7 +438,7 @@ int reactor_populate_monitor(struct rv_monitor *mon)
>  /*
>   * Nop reactor register
>   */
> -__printf(1, 2) static void rv_nop_reaction(const char *msg, ...)
> +__printf(1, 0) static void rv_nop_reaction(const char *msg, va_list args)
>  {
>  }
>  
> @@ -477,3 +477,17 @@ int init_rv_reactors(struct dentry *root_dir)
>  out_err:
>  	return -ENOMEM;
>  }
> +
> +void rv_react(struct rv_monitor *monitor, const char *msg, ...)
> +{
> +	va_list args;
> +
> +	if (!rv_reacting_on() || !monitor->react)
> +		return;
> +
> +	va_start(args, msg);
> +
> +	monitor->react(msg, args);
> +
> +	va_end(args);
> +}

[PATCH 2/3] rv: Make rv_reacting_on() static

[Thomas Weißschuh]

There are no external users left.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 include/linux/rv.h            | 6 ------
 kernel/trace/rv/rv_reactors.c | 2 +-
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/include/linux/rv.h b/include/linux/rv.h
index b567b0191e67f7dfab74e2aad6de3ed63d94058d..92fd467547e76d8868289b694f28220fc857dbcc 100644
--- a/include/linux/rv.h
+++ b/include/linux/rv.h
@@ -116,17 +116,11 @@ int rv_get_task_monitor_slot(void);
 void rv_put_task_monitor_slot(int slot);
 
 #ifdef CONFIG_RV_REACTORS
-bool rv_reacting_on(void);
 int rv_unregister_reactor(struct rv_reactor *reactor);
 int rv_register_reactor(struct rv_reactor *reactor);
 __printf(2, 3)
 void rv_react(struct rv_monitor *monitor, const char *msg, ...);
 #else
-static inline bool rv_reacting_on(void)
-{
-	return false;
-}
-
 __printf(2, 3)
 static inline void rv_react(struct rv_monitor *monitor, const char *msg, ...)
 {
diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index cb1a5968055abb22439a066b4e25dad98f078389..8c02426bc3bd944265f809e431283d1a20d56a8c 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -347,7 +347,7 @@ static bool __read_mostly reacting_on;
  *
  * Returns 1 if on, 0 otherwise.
  */
-bool rv_reacting_on(void)
+static bool rv_reacting_on(void)
 {
 	/* Ensures that concurrent monitors read consistent reacting_on */
 	smp_rmb();

-- 
2.51.0

[PATCH 3/3] rv: Add explicit lockdep context for reactors

[Thomas Weißschuh]

Reactors can be called from any context through tracepoints.
When developing reactors care needs to be taken to only call APIs which
are safe. As the tracepoints used during testing may not actually be
called from restrictive contexts lockdep may not be helpful.

Add explicit overrides to help lockdep find invalid code patterns.

The usage of LD_WAIT_FREE will trigger lockdep warnings in the panic
reactor. These are indeed valid warnings but they are out of scope for
RV and will instead be fixed by the printk subsystem.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 kernel/trace/rv/rv_reactors.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index 8c02426bc3bd944265f809e431283d1a20d56a8c..d9d335ae9badaa320f1d35dd159a033c3a30eb1a 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -61,6 +61,7 @@
  *      printk
  */
 
+#include <linux/lockdep.h>
 #include <linux/slab.h>
 
 #include "rv.h"
@@ -480,6 +481,7 @@ int init_rv_reactors(struct dentry *root_dir)
 
 void rv_react(struct rv_monitor *monitor, const char *msg, ...)
 {
+	static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map, LD_WAIT_FREE);
 	va_list args;
 
 	if (!rv_reacting_on() || !monitor->react)
@@ -487,7 +489,9 @@ void rv_react(struct rv_monitor *monitor, const char *msg, ...)
 
 	va_start(args, msg);
 
+	lock_map_acquire_try(&rv_react_map);
 	monitor->react(msg, args);
+	lock_map_release(&rv_react_map);
 
 	va_end(args);
 }

-- 
2.51.0

Re: [PATCH 3/3] rv: Add explicit lockdep context for reactors

[Gabriele Monaco]

On Tue, 2025-10-14 at 07:51 +0200, Thomas Weißschuh wrote:
> Reactors can be called from any context through tracepoints.
> When developing reactors care needs to be taken to only call APIs which
> are safe. As the tracepoints used during testing may not actually be
> called from restrictive contexts lockdep may not be helpful.
> 
> Add explicit overrides to help lockdep find invalid code patterns.
> 
> The usage of LD_WAIT_FREE will trigger lockdep warnings in the panic
> reactor. These are indeed valid warnings but they are out of scope for
> RV and will instead be fixed by the printk subsystem.

Looks like a nice addition!
If I get it correctly, this patch does trigger a lockdep warning with the
current state of the kernel. Is there a plan of fixing the warning in printk?
I assume this series would need to wait for that or did you have other ideas?

Thanks,
Gabriele

> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
>  kernel/trace/rv/rv_reactors.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
> index
> 8c02426bc3bd944265f809e431283d1a20d56a8c..d9d335ae9badaa320f1d35dd159a033c3a30
> eb1a 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -61,6 +61,7 @@
>   *      printk
>   */
>  
> +#include <linux/lockdep.h>
>  #include <linux/slab.h>
>  
>  #include "rv.h"
> @@ -480,6 +481,7 @@ int init_rv_reactors(struct dentry *root_dir)
>  
>  void rv_react(struct rv_monitor *monitor, const char *msg, ...)
>  {
> +	static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map, LD_WAIT_FREE);
>  	va_list args;
>  
>  	if (!rv_reacting_on() || !monitor->react)
> @@ -487,7 +489,9 @@ void rv_react(struct rv_monitor *monitor, const char *msg,
> ...)
>  
>  	va_start(args, msg);
>  
> +	lock_map_acquire_try(&rv_react_map);
>  	monitor->react(msg, args);
> +	lock_map_release(&rv_react_map);
>  
>  	va_end(args);
>  }

Re: [PATCH 3/3] rv: Add explicit lockdep context for reactors

[Thomas Weißschuh]

On Tue, Oct 14, 2025 at 08:55:44AM +0200, Gabriele Monaco wrote:
> On Tue, 2025-10-14 at 07:51 +0200, Thomas Weißschuh wrote:
> > Reactors can be called from any context through tracepoints.
> > When developing reactors care needs to be taken to only call APIs which
> > are safe. As the tracepoints used during testing may not actually be
> > called from restrictive contexts lockdep may not be helpful.
> > 
> > Add explicit overrides to help lockdep find invalid code patterns.
> > 
> > The usage of LD_WAIT_FREE will trigger lockdep warnings in the panic
> > reactor. These are indeed valid warnings but they are out of scope for
> > RV and will instead be fixed by the printk subsystem.
> 
> Looks like a nice addition!

Thanks!

> If I get it correctly, this patch does trigger a lockdep warning with the
> current state of the kernel. Is there a plan of fixing the warning in printk?

This will be fixed as soon as the console drivers are converted to the
nonblocking APIs. And there are a few other lockdep warnings that will pop up
after that in other kernel subsystems which are currently hidden by the printk
one. None of which should be the responsibility of RV to fix.

> I assume this series would need to wait for that or did you have other ideas?

I think this series can go in as-is. Only the panic reactor is affected and the
panic will still go through anyways. *After* the panic will be a lockdep splat.


Thomas

Re: [PATCH 3/3] rv: Add explicit lockdep context for reactors

[Nam Cao]

Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes:
> Reactors can be called from any context through tracepoints.
> When developing reactors care needs to be taken to only call APIs which
> are safe. As the tracepoints used during testing may not actually be
> called from restrictive contexts lockdep may not be helpful.
>
> Add explicit overrides to help lockdep find invalid code patterns.
>
> The usage of LD_WAIT_FREE will trigger lockdep warnings in the panic
> reactor. These are indeed valid warnings but they are out of scope for
> RV and will instead be fixed by the printk subsystem.
>
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
...
>  void rv_react(struct rv_monitor *monitor, const char *msg, ...)
>  {
> +	static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map, LD_WAIT_FREE);
>  	va_list args;
>  
>  	if (!rv_reacting_on() || !monitor->react)
> @@ -487,7 +489,9 @@ void rv_react(struct rv_monitor *monitor, const char *msg, ...)
>  
>  	va_start(args, msg);
>  
> +	lock_map_acquire_try(&rv_react_map);
>  	monitor->react(msg, args);
> +	lock_map_release(&rv_react_map);
>  
>  	va_end(args);
>  }

The reactors are invoked in tracepoints' handlers, thus they must not
trigger another tracepoint, otherwise we may be stuck in an infinite loop.
(this is why preempt_enable_notrace() exists alongside preempt_enable()).

I'm not familiar with the internal lockdep. But I think these would
trigger trace_lock_acquire() and trace_lock_release().

Nam

Re: [PATCH 3/3] rv: Add explicit lockdep context for reactors

[Thomas Weißschuh]

On Tue, Oct 14, 2025 at 09:38:09AM +0200, Nam Cao wrote:
> Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes:
> > Reactors can be called from any context through tracepoints.
> > When developing reactors care needs to be taken to only call APIs which
> > are safe. As the tracepoints used during testing may not actually be
> > called from restrictive contexts lockdep may not be helpful.
> >
> > Add explicit overrides to help lockdep find invalid code patterns.
> >
> > The usage of LD_WAIT_FREE will trigger lockdep warnings in the panic
> > reactor. These are indeed valid warnings but they are out of scope for
> > RV and will instead be fixed by the printk subsystem.
> >
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > ---
> ...
> >  void rv_react(struct rv_monitor *monitor, const char *msg, ...)
> >  {
> > +	static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map, LD_WAIT_FREE);
> >  	va_list args;
> >  
> >  	if (!rv_reacting_on() || !monitor->react)
> > @@ -487,7 +489,9 @@ void rv_react(struct rv_monitor *monitor, const char *msg, ...)
> >  
> >  	va_start(args, msg);
> >  
> > +	lock_map_acquire_try(&rv_react_map);
> >  	monitor->react(msg, args);
> > +	lock_map_release(&rv_react_map);
> >  
> >  	va_end(args);
> >  }
> 
> The reactors are invoked in tracepoints' handlers, thus they must not
> trigger another tracepoint, otherwise we may be stuck in an infinite loop.
> (this is why preempt_enable_notrace() exists alongside preempt_enable()).

Sounds reasonable. However today not even the printk reactor satisfies this
rule as it transitively calls trace_console().

> I'm not familiar with the internal lockdep. But I think these would
> trigger trace_lock_acquire() and trace_lock_release().

Indeed. Right now no monitor attaches to those tracepoints. We could
prevent monitors from attaching to certain "well-known" tracepoints.
But then we still need to manually track which those are, which is ugly.
Or we move the invocation of the reactor to a workqueue/task_work.


Thomas

Re: [PATCH 3/3] rv: Add explicit lockdep context for reactors

[Gabriele Monaco]

On Tue, 2025-10-14 at 11:46 +0200, Thomas Weißschuh wrote:
> On Tue, Oct 14, 2025 at 09:38:09AM +0200, Nam Cao wrote:
> > The reactors are invoked in tracepoints' handlers, thus they must not
> > trigger another tracepoint, otherwise we may be stuck in an infinite loop.
> > (this is why preempt_enable_notrace() exists alongside preempt_enable()).
> 
> Sounds reasonable. However today not even the printk reactor satisfies this
> rule as it transitively calls trace_console().

That's a valid concern, I assume it would become a problem also if we wanted to
use locks inside event handlers, as it was discussed some time ago to better
handle concurrency.

> > I'm not familiar with the internal lockdep. But I think these would
> > trigger trace_lock_acquire() and trace_lock_release().
> 
> Indeed. Right now no monitor attaches to those tracepoints. We could
> prevent monitors from attaching to certain "well-known" tracepoints.
> But then we still need to manually track which those are, which is ugly.
> Or we move the invocation of the reactor to a workqueue/task_work.

I'm afraid also workqueues might open a rabbit-hole (waking up a task fights
with locks in many scheduling tracepoints).
At a quick glance task_works also do some IPI/wakeups that are traced.
If I get it correctly we are looking for something absolutely lock-free/trace-
free, I can't really think of much at the moment, maybe abusing RCU callbacks
but those would have their set of problems too.

As much as it might be interesting to write monitors on lockdep tracepoints,
this seems challenging.

We could opt for a foolproof Kconfig solution and prevent reactors if lockdep is
active (leaving only the error tracepoints that are hopefully still safe).

Gabriele

Re: [PATCH 3/3] rv: Add explicit lockdep context for reactors

[Thomas Weißschuh]

On Tue, Oct 14, 2025 at 12:22:06PM +0200, Gabriele Monaco wrote:
> On Tue, 2025-10-14 at 11:46 +0200, Thomas Weißschuh wrote:
> > On Tue, Oct 14, 2025 at 09:38:09AM +0200, Nam Cao wrote:
> > > The reactors are invoked in tracepoints' handlers, thus they must not
> > > trigger another tracepoint, otherwise we may be stuck in an infinite loop.
> > > (this is why preempt_enable_notrace() exists alongside preempt_enable()).

(...)

> > > I'm not familiar with the internal lockdep. But I think these would
> > > trigger trace_lock_acquire() and trace_lock_release().
> > 
> > Indeed. Right now no monitor attaches to those tracepoints. We could
> > prevent monitors from attaching to certain "well-known" tracepoints.
> > But then we still need to manually track which those are, which is ugly.
> > Or we move the invocation of the reactor to a workqueue/task_work.
> 
> I'm afraid also workqueues might open a rabbit-hole (waking up a task fights
> with locks in many scheduling tracepoints).
> At a quick glance task_works also do some IPI/wakeups that are traced.
> If I get it correctly we are looking for something absolutely lock-free/trace-
> free, I can't really think of much at the moment, maybe abusing RCU callbacks
> but those would have their set of problems too.

Agreed.

> As much as it might be interesting to write monitors on lockdep tracepoints,
> this seems challenging.

> We could opt for a foolproof Kconfig solution and prevent reactors if lockdep is
> active (leaving only the error tracepoints that are hopefully still safe).

I can't follow here. lockdep can indicate problems, but it should not introduce
problems on its own. So preventing the usage together with lockdep would be the
proverbial head in the sand. If the tracepoints called by lockdep are an issue
then we would just not call into lockdep in the first place. lockdep triggering
these tracepoints should not be an issue in practice. I don't see a bulletproof
way to prevent a tracepoint handler from calling another tracepoint, except
maybe extending lockdep to also track that.


Thomas

Re: [PATCH 3/3] rv: Add explicit lockdep context for reactors

[Gabriele Monaco]

On Tue, 2025-10-14 at 14:51 +0200, Thomas Weißschuh wrote:
> I can't follow here. lockdep can indicate problems, but it should not
> introduce
> problems on its own. So preventing the usage together with lockdep would be
> the
> proverbial head in the sand. If the tracepoints called by lockdep are an issue
> then we would just not call into lockdep in the first place. lockdep
> triggering
> these tracepoints should not be an issue in practice. I don't see a
> bulletproof
> way to prevent a tracepoint handler from calling another tracepoint, except
> maybe extending lockdep to also track that.

Forget about it, you're right. This leads to not using lockdep inside reactors
in the first place. We could even have notrace versions of the lockdep calls
(I'm not sure lockdep itself needs them), but that's getting horrid.

Leaving for a moment concurrency quirks aside, a monitor that is reacting should
be done for a while and can be marked as not monitoring before reacting, instead
of after.
Trace handlers triggered in the same tracepoints should, in principle, be able
to tell they are not supposed to run. This at least stands for DA monitors, but
the same idea could work on LTL as well.

Of course this gets more complicated in practice, but perhaps suspending
monitors during reaction can be enough to allow these lockdep calls without
risking infinite loops.

Thoughts?

Gabriele

Re: [PATCH 3/3] rv: Add explicit lockdep context for reactors

[Thomas Weißschuh]

On Tue, Oct 14, 2025 at 03:45:39PM +0200, Gabriele Monaco wrote:
> On Tue, 2025-10-14 at 14:51 +0200, Thomas Weißschuh wrote:
> > I can't follow here. lockdep can indicate problems, but it should not
> > introduce
> > problems on its own. So preventing the usage together with lockdep would be
> > the
> > proverbial head in the sand. If the tracepoints called by lockdep are an issue
> > then we would just not call into lockdep in the first place. lockdep
> > triggering
> > these tracepoints should not be an issue in practice. I don't see a
> > bulletproof
> > way to prevent a tracepoint handler from calling another tracepoint, except
> > maybe extending lockdep to also track that.
> 
> Forget about it, you're right. This leads to not using lockdep inside reactors
> in the first place. We could even have notrace versions of the lockdep calls
> (I'm not sure lockdep itself needs them), but that's getting horrid.

I still don't understand why the tracepoints called from lockdep are worse then
the ones called from the reactors themselves? Any solution should also apply to
those. Especially as even the simplest printk reactor runs into the same issue.

> Leaving for a moment concurrency quirks aside, a monitor that is reacting should
> be done for a while and can be marked as not monitoring before reacting, instead
> of after.
> Trace handlers triggered in the same tracepoints should, in principle, be able
> to tell they are not supposed to run. This at least stands for DA monitors, but
> the same idea could work on LTL as well.
> 
> Of course this gets more complicated in practice, but perhaps suspending
> monitors during reaction can be enough to allow these lockdep calls without
> risking infinite loops.

What would it mean to suspend a monitor? In my opinion we shouldn't sacrifice
the accuracy of the monitors or the reliability of the reactors while trying to
mitigate a theoretical problem.


Thomas

Re: [PATCH 3/3] rv: Add explicit lockdep context for reactors

[Gabriele Monaco]

On Tue, 2025-10-14 at 16:18 +0200, Thomas Weißschuh wrote:
> On Tue, Oct 14, 2025 at 03:45:39PM +0200, Gabriele Monaco wrote:
> > On Tue, 2025-10-14 at 14:51 +0200, Thomas Weißschuh wrote:
> > > I can't follow here. lockdep can indicate problems, but it should not
> > > introduce
> > > problems on its own. So preventing the usage together with lockdep would
> > > be
> > > the
> > > proverbial head in the sand. If the tracepoints called by lockdep are an
> > > issue
> > > then we would just not call into lockdep in the first place. lockdep
> > > triggering
> > > these tracepoints should not be an issue in practice. I don't see a
> > > bulletproof
> > > way to prevent a tracepoint handler from calling another tracepoint,
> > > except
> > > maybe extending lockdep to also track that.
> > 
> > Forget about it, you're right. This leads to not using lockdep inside
> > reactors
> > in the first place. We could even have notrace versions of the lockdep calls
> > (I'm not sure lockdep itself needs them), but that's getting horrid.
> 
> I still don't understand why the tracepoints called from lockdep are worse
> then
> the ones called from the reactors themselves? Any solution should also apply
> to
> those. Especially as even the simplest printk reactor runs into the same
> issue.

They aren't in fact, so yes, we already had this problem without knowing about
it.

> > Leaving for a moment concurrency quirks aside, a monitor that is reacting
> > should be done for a while and can be marked as not monitoring before
> > reacting, instead of after.
> > Trace handlers triggered in the same tracepoints should, in principle, be
> > able to tell they are not supposed to run. This at least stands for DA
> > monitors, but the same idea could work on LTL as well.
> > 
> > Of course this gets more complicated in practice, but perhaps suspending
> > monitors during reaction can be enough to allow these lockdep calls without
> > risking infinite loops.
> 
> What would it mean to suspend a monitor? In my opinion we shouldn't sacrifice
> the accuracy of the monitors or the reliability of the reactors while trying
> to mitigate a theoretical problem.

I don't mean to really sacrifice accuracy, DA monitors are disabled after a
reaction. This comes from the assumption that the model becomes invalid, so
whatever comes after might be meaningless. Monitors restart as soon as we are
sure we reached the initial state.
In this case, it already doesn't make sense to monitor events triggered by
reactors.

LTL is a bit more complex, so it might make sense to continue monitoring just
after a reaction, but I'm not sure how useful that is.

Re: [PATCH 3/3] rv: Add explicit lockdep context for reactors

[Thomas Weißschuh]

On Tue, Oct 14, 2025 at 04:50:18PM +0200, Gabriele Monaco wrote:
> On Tue, 2025-10-14 at 16:18 +0200, Thomas Weißschuh wrote:
> > On Tue, Oct 14, 2025 at 03:45:39PM +0200, Gabriele Monaco wrote:
> > > On Tue, 2025-10-14 at 14:51 +0200, Thomas Weißschuh wrote:

(...)

> > > Leaving for a moment concurrency quirks aside, a monitor that is reacting
> > > should be done for a while and can be marked as not monitoring before
> > > reacting, instead of after.
> > > Trace handlers triggered in the same tracepoints should, in principle, be
> > > able to tell they are not supposed to run. This at least stands for DA
> > > monitors, but the same idea could work on LTL as well.
> > > 
> > > Of course this gets more complicated in practice, but perhaps suspending
> > > monitors during reaction can be enough to allow these lockdep calls without
> > > risking infinite loops.
> > 
> > What would it mean to suspend a monitor? In my opinion we shouldn't sacrifice
> > the accuracy of the monitors or the reliability of the reactors while trying
> > to mitigate a theoretical problem.
> 
> I don't mean to really sacrifice accuracy, DA monitors are disabled after a
> reaction. This comes from the assumption that the model becomes invalid, so
> whatever comes after might be meaningless. Monitors restart as soon as we are
> sure we reached the initial state.
> In this case, it already doesn't make sense to monitor events triggered by
> reactors.
>
> LTL is a bit more complex, so it might make sense to continue monitoring just
> after a reaction, but I'm not sure how useful that is.

Ack.

It is still possible to manually re-enable all monitors through sysfs, correct?
That is needed for the kind of testing I have in mind.

Do we still consider these hypothetical tracepoint loops a blocker for this
patch series? In my opinion the usage of lockdep does not exacerbate the risk.


Thomas