[PATCH 0/6] rv: Clean up & simplify

[PATCH 0/6] rv: Clean up & simplify

[Nam Cao]

Hi,

This series removes some redundant code and simply RV.

Nam Cao (6):
  rv: Remove unused field in struct rv_monitor_def
  rv: Merge struct rv_monitor_def into struct rv_monitor
  rv: Merge struct rv_reactor_def into struct rv_reactor
  rv: Remove rv_reactor's reference counter
  rv: Remove the nop reactor
  rv: Remove struct rv_monitor::reacting

 include/linux/rv.h            |   8 ++
 kernel/trace/rv/rv.c          | 210 +++++++++++++++-------------------
 kernel/trace/rv/rv.h          |  39 +------
 kernel/trace/rv/rv_reactors.c | 178 ++++++++--------------------
 4 files changed, 152 insertions(+), 283 deletions(-)

-- 
2.39.5

[PATCH 1/6] rv: Remove unused field in struct rv_monitor_def

[Nam Cao]

rv_monitor_def::task_monitor is not used. Delete it.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 kernel/trace/rv/rv.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/trace/rv/rv.h b/kernel/trace/rv/rv.h
index 98fca0a1adbc..873364094402 100644
--- a/kernel/trace/rv/rv.h
+++ b/kernel/trace/rv/rv.h
@@ -41,7 +41,6 @@ struct rv_monitor_def {
 	struct rv_reactor_def	*rdef;
 	bool			reacting;
 #endif
-	bool			task_monitor;
 };
 
 struct dentry *get_monitors_root(void);
-- 
2.39.5

Re: [PATCH 1/6] rv: Remove unused field in struct rv_monitor_def

[Gabriele Monaco]

On Mon, 2025-07-21 at 11:47 +0200, Nam Cao wrote:
> rv_monitor_def::task_monitor is not used. Delete it.
> 
> Signed-off-by: Nam Cao <namcao@linutronix.de>

Reviewed-by: Gabriele Monaco <gmonaco@redhat.com>

Thanks,
Gabriele

> ---
>  kernel/trace/rv/rv.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/kernel/trace/rv/rv.h b/kernel/trace/rv/rv.h
> index 98fca0a1adbc..873364094402 100644
> --- a/kernel/trace/rv/rv.h
> +++ b/kernel/trace/rv/rv.h
> @@ -41,7 +41,6 @@ struct rv_monitor_def {
>  	struct rv_reactor_def	*rdef;
>  	bool			reacting;
>  #endif
> -	bool			task_monitor;
>  };
>  
>  struct dentry *get_monitors_root(void);

[PATCH 2/6] rv: Merge struct rv_monitor_def into struct rv_monitor

[Nam Cao]

Each struct rv_monitor has a unique struct rv_monitor_def associated with
it. struct rv_monitor is statically allocated, while struct rv_monitor_def
is dynamically allocated.

This makes the code more complicated than it should be:

  - Lookup is required to get the associated rv_monitor_def from rv_monitor

  - Dynamic memory allocation is required for rv_monitor_def. This is
    harder to get right compared to static memory. For instance, there is
    an existing mistake: rv_unregister_monitor() does not free the memory
    allocated by rv_register_monitor(). This is fortunately not a real
    memory leak problem, as rv_unregister_monitor() is never called.

Simplify and merge rv_monitor_def into rv_monitor.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 include/linux/rv.h            |   8 ++
 kernel/trace/rv/rv.c          | 211 +++++++++++++++-------------------
 kernel/trace/rv/rv.h          |  27 ++---
 kernel/trace/rv/rv_reactors.c |  62 +++++-----
 4 files changed, 140 insertions(+), 168 deletions(-)

diff --git a/include/linux/rv.h b/include/linux/rv.h
index 97baf58d88b2..dba53aecdfab 100644
--- a/include/linux/rv.h
+++ b/include/linux/rv.h
@@ -7,6 +7,9 @@
 #ifndef _LINUX_RV_H
 #define _LINUX_RV_H
 
+#include <linux/types.h>
+#include <linux/list.h>
+
 #define MAX_DA_NAME_LEN	32
 
 #ifdef CONFIG_RV
@@ -98,8 +101,13 @@ struct rv_monitor {
 	void			(*disable)(void);
 	void			(*reset)(void);
 #ifdef CONFIG_RV_REACTORS
+	struct rv_reactor_def	*rdef;
 	__printf(1, 2) void	(*react)(const char *msg, ...);
+	bool			reacting;
 #endif
+	struct list_head	list;
+	struct rv_monitor	*parent;
+	struct dentry		*root_d;
 };
 
 bool rv_monitoring_on(void);
diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
index 108429d16ec1..6c0be2fdc52d 100644
--- a/kernel/trace/rv/rv.c
+++ b/kernel/trace/rv/rv.c
@@ -210,9 +210,9 @@ void rv_put_task_monitor_slot(int slot)
  * Monitors with a parent are nested,
  * Monitors without a parent could be standalone or containers.
  */
-bool rv_is_nested_monitor(struct rv_monitor_def *mdef)
+bool rv_is_nested_monitor(struct rv_monitor *mon)
 {
-	return mdef->parent != NULL;
+	return mon->parent != NULL;
 }
 
 /*
@@ -223,16 +223,16 @@ bool rv_is_nested_monitor(struct rv_monitor_def *mdef)
  * for enable()/disable(). Use this condition to find empty containers.
  * Keep both conditions in case we have some non-compliant containers.
  */
-bool rv_is_container_monitor(struct rv_monitor_def *mdef)
+bool rv_is_container_monitor(struct rv_monitor *mon)
 {
-	struct rv_monitor_def *next;
+	struct rv_monitor *next;
 
-	if (list_is_last(&mdef->list, &rv_monitors_list))
+	if (list_is_last(&mon->list, &rv_monitors_list))
 		return false;
 
-	next = list_next_entry(mdef, list);
+	next = list_next_entry(mon, list);
 
-	return next->parent == mdef->monitor || !mdef->monitor->enable;
+	return next->parent == mon || !mon->enable;
 }
 
 /*
@@ -241,10 +241,10 @@ bool rv_is_container_monitor(struct rv_monitor_def *mdef)
 static ssize_t monitor_enable_read_data(struct file *filp, char __user *user_buf, size_t count,
 					loff_t *ppos)
 {
-	struct rv_monitor_def *mdef = filp->private_data;
+	struct rv_monitor *mon = filp->private_data;
 	const char *buff;
 
-	buff = mdef->monitor->enabled ? "1\n" : "0\n";
+	buff = mon->enabled ? "1\n" : "0\n";
 
 	return simple_read_from_buffer(user_buf, count, ppos, buff, strlen(buff)+1);
 }
@@ -252,14 +252,14 @@ static ssize_t monitor_enable_read_data(struct file *filp, char __user *user_buf
 /*
  * __rv_disable_monitor - disabled an enabled monitor
  */
-static int __rv_disable_monitor(struct rv_monitor_def *mdef, bool sync)
+static int __rv_disable_monitor(struct rv_monitor *mon, bool sync)
 {
 	lockdep_assert_held(&rv_interface_lock);
 
-	if (mdef->monitor->enabled) {
-		mdef->monitor->enabled = 0;
-		if (mdef->monitor->disable)
-			mdef->monitor->disable();
+	if (mon->enabled) {
+		mon->enabled = 0;
+		if (mon->disable)
+			mon->disable();
 
 		/*
 		 * Wait for the execution of all events to finish.
@@ -273,90 +273,90 @@ static int __rv_disable_monitor(struct rv_monitor_def *mdef, bool sync)
 	return 0;
 }
 
-static void rv_disable_single(struct rv_monitor_def *mdef)
+static void rv_disable_single(struct rv_monitor *mon)
 {
-	__rv_disable_monitor(mdef, true);
+	__rv_disable_monitor(mon, true);
 }
 
-static int rv_enable_single(struct rv_monitor_def *mdef)
+static int rv_enable_single(struct rv_monitor *mon)
 {
 	int retval;
 
 	lockdep_assert_held(&rv_interface_lock);
 
-	if (mdef->monitor->enabled)
+	if (mon->enabled)
 		return 0;
 
-	retval = mdef->monitor->enable();
+	retval = mon->enable();
 
 	if (!retval)
-		mdef->monitor->enabled = 1;
+		mon->enabled = 1;
 
 	return retval;
 }
 
-static void rv_disable_container(struct rv_monitor_def *mdef)
+static void rv_disable_container(struct rv_monitor *mon)
 {
-	struct rv_monitor_def *p = mdef;
+	struct rv_monitor *p = mon;
 	int enabled = 0;
 
 	list_for_each_entry_continue(p, &rv_monitors_list, list) {
-		if (p->parent != mdef->monitor)
+		if (p->parent != mon)
 			break;
 		enabled += __rv_disable_monitor(p, false);
 	}
 	if (enabled)
 		tracepoint_synchronize_unregister();
-	mdef->monitor->enabled = 0;
+	mon->enabled = 0;
 }
 
-static int rv_enable_container(struct rv_monitor_def *mdef)
+static int rv_enable_container(struct rv_monitor *mon)
 {
-	struct rv_monitor_def *p = mdef;
+	struct rv_monitor *p = mon;
 	int retval = 0;
 
 	list_for_each_entry_continue(p, &rv_monitors_list, list) {
-		if (retval || p->parent != mdef->monitor)
+		if (retval || p->parent != mon)
 			break;
 		retval = rv_enable_single(p);
 	}
 	if (retval)
-		rv_disable_container(mdef);
+		rv_disable_container(mon);
 	else
-		mdef->monitor->enabled = 1;
+		mon->enabled = 1;
 	return retval;
 }
 
 /**
  * rv_disable_monitor - disable a given runtime monitor
- * @mdef: Pointer to the monitor definition structure.
+ * @mon: Pointer to the monitor definition structure.
  *
  * Returns 0 on success.
  */
-int rv_disable_monitor(struct rv_monitor_def *mdef)
+int rv_disable_monitor(struct rv_monitor *mon)
 {
-	if (rv_is_container_monitor(mdef))
-		rv_disable_container(mdef);
+	if (rv_is_container_monitor(mon))
+		rv_disable_container(mon);
 	else
-		rv_disable_single(mdef);
+		rv_disable_single(mon);
 
 	return 0;
 }
 
 /**
  * rv_enable_monitor - enable a given runtime monitor
- * @mdef: Pointer to the monitor definition structure.
+ * @mon: Pointer to the monitor definition structure.
  *
  * Returns 0 on success, error otherwise.
  */
-int rv_enable_monitor(struct rv_monitor_def *mdef)
+int rv_enable_monitor(struct rv_monitor *mon)
 {
 	int retval;
 
-	if (rv_is_container_monitor(mdef))
-		retval = rv_enable_container(mdef);
+	if (rv_is_container_monitor(mon))
+		retval = rv_enable_container(mon);
 	else
-		retval = rv_enable_single(mdef);
+		retval = rv_enable_single(mon);
 
 	return retval;
 }
@@ -367,7 +367,7 @@ int rv_enable_monitor(struct rv_monitor_def *mdef)
 static ssize_t monitor_enable_write_data(struct file *filp, const char __user *user_buf,
 					 size_t count, loff_t *ppos)
 {
-	struct rv_monitor_def *mdef = filp->private_data;
+	struct rv_monitor *mon = filp->private_data;
 	int retval;
 	bool val;
 
@@ -378,9 +378,9 @@ static ssize_t monitor_enable_write_data(struct file *filp, const char __user *u
 	mutex_lock(&rv_interface_lock);
 
 	if (val)
-		retval = rv_enable_monitor(mdef);
+		retval = rv_enable_monitor(mon);
 	else
-		retval = rv_disable_monitor(mdef);
+		retval = rv_disable_monitor(mon);
 
 	mutex_unlock(&rv_interface_lock);
 
@@ -399,12 +399,12 @@ static const struct file_operations interface_enable_fops = {
 static ssize_t monitor_desc_read_data(struct file *filp, char __user *user_buf, size_t count,
 				      loff_t *ppos)
 {
-	struct rv_monitor_def *mdef = filp->private_data;
+	struct rv_monitor *mon = filp->private_data;
 	char buff[256];
 
 	memset(buff, 0, sizeof(buff));
 
-	snprintf(buff, sizeof(buff), "%s\n", mdef->monitor->description);
+	snprintf(buff, sizeof(buff), "%s\n", mon->description);
 
 	return simple_read_from_buffer(user_buf, count, ppos, buff, strlen(buff) + 1);
 }
@@ -419,37 +419,37 @@ static const struct file_operations interface_desc_fops = {
  * the monitor dir, where the specific options of the monitor
  * are exposed.
  */
-static int create_monitor_dir(struct rv_monitor_def *mdef, struct rv_monitor_def *parent)
+static int create_monitor_dir(struct rv_monitor *mon, struct rv_monitor *parent)
 {
 	struct dentry *root = parent ? parent->root_d : get_monitors_root();
-	const char *name = mdef->monitor->name;
+	const char *name = mon->name;
 	struct dentry *tmp;
 	int retval;
 
-	mdef->root_d = rv_create_dir(name, root);
-	if (!mdef->root_d)
+	mon->root_d = rv_create_dir(name, root);
+	if (!mon->root_d)
 		return -ENOMEM;
 
-	tmp = rv_create_file("enable", RV_MODE_WRITE, mdef->root_d, mdef, &interface_enable_fops);
+	tmp = rv_create_file("enable", RV_MODE_WRITE, mon->root_d, mon, &interface_enable_fops);
 	if (!tmp) {
 		retval = -ENOMEM;
 		goto out_remove_root;
 	}
 
-	tmp = rv_create_file("desc", RV_MODE_READ, mdef->root_d, mdef, &interface_desc_fops);
+	tmp = rv_create_file("desc", RV_MODE_READ, mon->root_d, mon, &interface_desc_fops);
 	if (!tmp) {
 		retval = -ENOMEM;
 		goto out_remove_root;
 	}
 
-	retval = reactor_populate_monitor(mdef);
+	retval = reactor_populate_monitor(mon);
 	if (retval)
 		goto out_remove_root;
 
 	return 0;
 
 out_remove_root:
-	rv_remove(mdef->root_d);
+	rv_remove(mon->root_d);
 	return retval;
 }
 
@@ -458,13 +458,12 @@ static int create_monitor_dir(struct rv_monitor_def *mdef, struct rv_monitor_def
  */
 static int monitors_show(struct seq_file *m, void *p)
 {
-	struct rv_monitor_def *mon_def = p;
+	struct rv_monitor *mon = p;
 
-	if (mon_def->parent)
-		seq_printf(m, "%s:%s\n", mon_def->parent->name,
-			   mon_def->monitor->name);
+	if (mon->parent)
+		seq_printf(m, "%s:%s\n", mon->parent->name, mon->name);
 	else
-		seq_printf(m, "%s\n", mon_def->monitor->name);
+		seq_printf(m, "%s\n", mon->name);
 	return 0;
 }
 
@@ -496,13 +495,13 @@ static void *available_monitors_next(struct seq_file *m, void *p, loff_t *pos)
  */
 static void *enabled_monitors_next(struct seq_file *m, void *p, loff_t *pos)
 {
-	struct rv_monitor_def *m_def = p;
+	struct rv_monitor *mon = p;
 
 	(*pos)++;
 
-	list_for_each_entry_continue(m_def, &rv_monitors_list, list) {
-		if (m_def->monitor->enabled)
-			return m_def;
+	list_for_each_entry_continue(mon, &rv_monitors_list, list) {
+		if (mon->enabled)
+			return mon;
 	}
 
 	return NULL;
@@ -510,7 +509,7 @@ static void *enabled_monitors_next(struct seq_file *m, void *p, loff_t *pos)
 
 static void *enabled_monitors_start(struct seq_file *m, loff_t *pos)
 {
-	struct rv_monitor_def *m_def;
+	struct rv_monitor *mon;
 	loff_t l;
 
 	mutex_lock(&rv_interface_lock);
@@ -518,15 +517,15 @@ static void *enabled_monitors_start(struct seq_file *m, loff_t *pos)
 	if (list_empty(&rv_monitors_list))
 		return NULL;
 
-	m_def = list_entry(&rv_monitors_list, struct rv_monitor_def, list);
+	mon = list_entry(&rv_monitors_list, struct rv_monitor, list);
 
 	for (l = 0; l <= *pos; ) {
-		m_def = enabled_monitors_next(m, m_def, &l);
-		if (!m_def)
+		mon = enabled_monitors_next(m, mon, &l);
+		if (!mon)
 			break;
 	}
 
-	return m_def;
+	return mon;
 }
 
 /*
@@ -566,13 +565,13 @@ static const struct file_operations available_monitors_ops = {
  */
 static void disable_all_monitors(void)
 {
-	struct rv_monitor_def *mdef;
+	struct rv_monitor *mon;
 	int enabled = 0;
 
 	mutex_lock(&rv_interface_lock);
 
-	list_for_each_entry(mdef, &rv_monitors_list, list)
-		enabled += __rv_disable_monitor(mdef, false);
+	list_for_each_entry(mon, &rv_monitors_list, list)
+		enabled += __rv_disable_monitor(mon, false);
 
 	if (enabled) {
 		/*
@@ -598,7 +597,7 @@ static ssize_t enabled_monitors_write(struct file *filp, const char __user *user
 				      size_t count, loff_t *ppos)
 {
 	char buff[MAX_RV_MONITOR_NAME_SIZE + 2];
-	struct rv_monitor_def *mdef;
+	struct rv_monitor *mon;
 	int retval = -EINVAL;
 	bool enable = true;
 	char *ptr, *tmp;
@@ -633,17 +632,17 @@ static ssize_t enabled_monitors_write(struct file *filp, const char __user *user
 	if (tmp)
 		ptr = tmp+1;
 
-	list_for_each_entry(mdef, &rv_monitors_list, list) {
-		if (strcmp(ptr, mdef->monitor->name) != 0)
+	list_for_each_entry(mon, &rv_monitors_list, list) {
+		if (strcmp(ptr, mon->name) != 0)
 			continue;
 
 		/*
 		 * Monitor found!
 		 */
 		if (enable)
-			retval = rv_enable_monitor(mdef);
+			retval = rv_enable_monitor(mon);
 		else
-			retval = rv_disable_monitor(mdef);
+			retval = rv_disable_monitor(mon);
 
 		if (!retval)
 			retval = count;
@@ -702,11 +701,11 @@ static void turn_monitoring_off(void)
 
 static void reset_all_monitors(void)
 {
-	struct rv_monitor_def *mdef;
+	struct rv_monitor *mon;
 
-	list_for_each_entry(mdef, &rv_monitors_list, list) {
-		if (mdef->monitor->enabled && mdef->monitor->reset)
-			mdef->monitor->reset();
+	list_for_each_entry(mon, &rv_monitors_list, list) {
+		if (mon->enabled && mon->reset)
+			mon->reset();
 	}
 }
 
@@ -768,10 +767,10 @@ static const struct file_operations monitoring_on_fops = {
 	.read   = monitoring_on_read_data,
 };
 
-static void destroy_monitor_dir(struct rv_monitor_def *mdef)
+static void destroy_monitor_dir(struct rv_monitor *mon)
 {
-	reactor_cleanup_monitor(mdef);
-	rv_remove(mdef->root_d);
+	reactor_cleanup_monitor(mon);
+	rv_remove(mon->root_d);
 }
 
 /**
@@ -783,7 +782,7 @@ static void destroy_monitor_dir(struct rv_monitor_def *mdef)
  */
 int rv_register_monitor(struct rv_monitor *monitor, struct rv_monitor *parent)
 {
-	struct rv_monitor_def *r, *p = NULL;
+	struct rv_monitor *r;
 	int retval = 0;
 
 	if (strlen(monitor->name) >= MAX_RV_MONITOR_NAME_SIZE) {
@@ -795,49 +794,31 @@ int rv_register_monitor(struct rv_monitor *monitor, struct rv_monitor *parent)
 	mutex_lock(&rv_interface_lock);
 
 	list_for_each_entry(r, &rv_monitors_list, list) {
-		if (strcmp(monitor->name, r->monitor->name) == 0) {
+		if (strcmp(monitor->name, r->name) == 0) {
 			pr_info("Monitor %s is already registered\n", monitor->name);
 			retval = -EEXIST;
 			goto out_unlock;
 		}
 	}
 
-	if (parent) {
-		list_for_each_entry(r, &rv_monitors_list, list) {
-			if (strcmp(parent->name, r->monitor->name) == 0) {
-				p = r;
-				break;
-			}
-		}
-	}
-
-	if (p && rv_is_nested_monitor(p)) {
+	if (parent && rv_is_nested_monitor(parent)) {
 		pr_info("Parent monitor %s is already nested, cannot nest further\n",
 			parent->name);
 		retval = -EINVAL;
 		goto out_unlock;
 	}
 
-	r = kzalloc(sizeof(struct rv_monitor_def), GFP_KERNEL);
-	if (!r) {
-		retval = -ENOMEM;
-		goto out_unlock;
-	}
-
-	r->monitor = monitor;
-	r->parent = parent;
+	monitor->parent = parent;
 
-	retval = create_monitor_dir(r, p);
-	if (retval) {
-		kfree(r);
-		goto out_unlock;
-	}
+	retval = create_monitor_dir(monitor, parent);
+	if (retval)
+		return retval;
 
 	/* keep children close to the parent for easier visualisation */
-	if (p)
-		list_add(&r->list, &p->list);
+	if (parent)
+		list_add(&monitor->list, &parent->list);
 	else
-		list_add_tail(&r->list, &rv_monitors_list);
+		list_add_tail(&monitor->list, &rv_monitors_list);
 
 out_unlock:
 	mutex_unlock(&rv_interface_lock);
@@ -852,17 +833,11 @@ int rv_register_monitor(struct rv_monitor *monitor, struct rv_monitor *parent)
  */
 int rv_unregister_monitor(struct rv_monitor *monitor)
 {
-	struct rv_monitor_def *ptr, *next;
-
 	mutex_lock(&rv_interface_lock);
 
-	list_for_each_entry_safe(ptr, next, &rv_monitors_list, list) {
-		if (strcmp(monitor->name, ptr->monitor->name) == 0) {
-			rv_disable_monitor(ptr);
-			list_del(&ptr->list);
-			destroy_monitor_dir(ptr);
-		}
-	}
+	rv_disable_monitor(monitor);
+	list_del(&monitor->list);
+	destroy_monitor_dir(monitor);
 
 	mutex_unlock(&rv_interface_lock);
 	return 0;
diff --git a/kernel/trace/rv/rv.h b/kernel/trace/rv/rv.h
index 873364094402..f039ec1c9156 100644
--- a/kernel/trace/rv/rv.h
+++ b/kernel/trace/rv/rv.h
@@ -32,34 +32,23 @@ struct rv_reactor_def {
 };
 #endif
 
-struct rv_monitor_def {
-	struct list_head	list;
-	struct rv_monitor	*monitor;
-	struct rv_monitor	*parent;
-	struct dentry		*root_d;
-#ifdef CONFIG_RV_REACTORS
-	struct rv_reactor_def	*rdef;
-	bool			reacting;
-#endif
-};
-
 struct dentry *get_monitors_root(void);
-int rv_disable_monitor(struct rv_monitor_def *mdef);
-int rv_enable_monitor(struct rv_monitor_def *mdef);
-bool rv_is_container_monitor(struct rv_monitor_def *mdef);
-bool rv_is_nested_monitor(struct rv_monitor_def *mdef);
+int rv_disable_monitor(struct rv_monitor *mon);
+int rv_enable_monitor(struct rv_monitor *mon);
+bool rv_is_container_monitor(struct rv_monitor *mon);
+bool rv_is_nested_monitor(struct rv_monitor *mon);
 
 #ifdef CONFIG_RV_REACTORS
-int reactor_populate_monitor(struct rv_monitor_def *mdef);
-void reactor_cleanup_monitor(struct rv_monitor_def *mdef);
+int reactor_populate_monitor(struct rv_monitor *mon);
+void reactor_cleanup_monitor(struct rv_monitor *mon);
 int init_rv_reactors(struct dentry *root_dir);
 #else
-static inline int reactor_populate_monitor(struct rv_monitor_def *mdef)
+static inline int reactor_populate_monitor(struct rv_monitor *mon)
 {
 	return 0;
 }
 
-static inline void reactor_cleanup_monitor(struct rv_monitor_def *mdef)
+static inline void reactor_cleanup_monitor(struct rv_monitor *mon)
 {
 	return;
 }
diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index 740603670dd1..7cc620a1be1a 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -138,10 +138,10 @@ static const struct file_operations available_reactors_ops = {
  */
 static int monitor_reactor_show(struct seq_file *m, void *p)
 {
-	struct rv_monitor_def *mdef = m->private;
+	struct rv_monitor *mon = m->private;
 	struct rv_reactor_def *rdef = p;
 
-	if (mdef->rdef == rdef)
+	if (mon->rdef == rdef)
 		seq_printf(m, "[%s]\n", rdef->reactor->name);
 	else
 		seq_printf(m, "%s\n", rdef->reactor->name);
@@ -158,41 +158,41 @@ static const struct seq_operations monitor_reactors_seq_ops = {
 	.show	= monitor_reactor_show
 };
 
-static void monitor_swap_reactors_single(struct rv_monitor_def *mdef,
+static void monitor_swap_reactors_single(struct rv_monitor *mon,
 					 struct rv_reactor_def *rdef,
 					 bool reacting, bool nested)
 {
 	bool monitor_enabled;
 
 	/* nothing to do */
-	if (mdef->rdef == rdef)
+	if (mon->rdef == rdef)
 		return;
 
-	monitor_enabled = mdef->monitor->enabled;
+	monitor_enabled = mon->enabled;
 	if (monitor_enabled)
-		rv_disable_monitor(mdef);
+		rv_disable_monitor(mon);
 
 	/* swap reactor's usage */
-	mdef->rdef->counter--;
+	mon->rdef->counter--;
 	rdef->counter++;
 
-	mdef->rdef = rdef;
-	mdef->reacting = reacting;
-	mdef->monitor->react = rdef->reactor->react;
+	mon->rdef = rdef;
+	mon->reacting = reacting;
+	mon->react = rdef->reactor->react;
 
 	/* enable only once if iterating through a container */
 	if (monitor_enabled && !nested)
-		rv_enable_monitor(mdef);
+		rv_enable_monitor(mon);
 }
 
-static void monitor_swap_reactors(struct rv_monitor_def *mdef,
+static void monitor_swap_reactors(struct rv_monitor *mon,
 				  struct rv_reactor_def *rdef, bool reacting)
 {
-	struct rv_monitor_def *p = mdef;
+	struct rv_monitor *p = mon;
 
-	if (rv_is_container_monitor(mdef))
+	if (rv_is_container_monitor(mon))
 		list_for_each_entry_continue(p, &rv_monitors_list, list) {
-			if (p->parent != mdef->monitor)
+			if (p->parent != mon)
 				break;
 			monitor_swap_reactors_single(p, rdef, reacting, true);
 		}
@@ -202,7 +202,7 @@ static void monitor_swap_reactors(struct rv_monitor_def *mdef,
 	 * All nested monitors are enabled also if they were off, we may refine
 	 * this logic in the future.
 	 */
-	monitor_swap_reactors_single(mdef, rdef, reacting, false);
+	monitor_swap_reactors_single(mon, rdef, reacting, false);
 }
 
 static ssize_t
@@ -210,7 +210,7 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
 		      size_t count, loff_t *ppos)
 {
 	char buff[MAX_RV_REACTOR_NAME_SIZE + 2];
-	struct rv_monitor_def *mdef;
+	struct rv_monitor *mon;
 	struct rv_reactor_def *rdef;
 	struct seq_file *seq_f;
 	int retval = -EINVAL;
@@ -237,7 +237,7 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
 	 * See monitor_reactors_open()
 	 */
 	seq_f = file->private_data;
-	mdef = seq_f->private;
+	mon = seq_f->private;
 
 	mutex_lock(&rv_interface_lock);
 
@@ -252,7 +252,7 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
 		else
 			enable = true;
 
-		monitor_swap_reactors(mdef, rdef, enable);
+		monitor_swap_reactors(mon, rdef, enable);
 
 		retval = count;
 		break;
@@ -268,7 +268,7 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
  */
 static int monitor_reactors_open(struct inode *inode, struct file *file)
 {
-	struct rv_monitor_def *mdef = inode->i_private;
+	struct rv_monitor *mon = inode->i_private;
 	struct seq_file *seq_f;
 	int ret;
 
@@ -284,7 +284,7 @@ static int monitor_reactors_open(struct inode *inode, struct file *file)
 	/*
 	 * Copy the create file "private" data to the seq_file private data.
 	 */
-	seq_f->private = mdef;
+	seq_f->private = mon;
 
 	return 0;
 };
@@ -454,37 +454,37 @@ static const struct file_operations reacting_on_fops = {
 
 /**
  * reactor_populate_monitor - creates per monitor reactors file
- * @mdef:	monitor's definition.
+ * @mon:	The monitor.
  *
  * Returns 0 if successful, error otherwise.
  */
-int reactor_populate_monitor(struct rv_monitor_def *mdef)
+int reactor_populate_monitor(struct rv_monitor *mon)
 {
 	struct dentry *tmp;
 
-	tmp = rv_create_file("reactors", RV_MODE_WRITE, mdef->root_d, mdef, &monitor_reactors_ops);
+	tmp = rv_create_file("reactors", RV_MODE_WRITE, mon->root_d, mon, &monitor_reactors_ops);
 	if (!tmp)
 		return -ENOMEM;
 
 	/*
 	 * Configure as the rv_nop reactor.
 	 */
-	mdef->rdef = get_reactor_rdef_by_name("nop");
-	mdef->rdef->counter++;
-	mdef->reacting = false;
+	mon->rdef = get_reactor_rdef_by_name("nop");
+	mon->rdef->counter++;
+	mon->reacting = false;
 
 	return 0;
 }
 
 /**
  * reactor_cleanup_monitor - cleanup a monitor reference
- * @mdef:       monitor's definition.
+ * @mon:       the monitor.
  */
-void reactor_cleanup_monitor(struct rv_monitor_def *mdef)
+void reactor_cleanup_monitor(struct rv_monitor *mon)
 {
 	lockdep_assert_held(&rv_interface_lock);
-	mdef->rdef->counter--;
-	WARN_ON_ONCE(mdef->rdef->counter < 0);
+	mon->rdef->counter--;
+	WARN_ON_ONCE(mon->rdef->counter < 0);
 }
 
 /*
-- 
2.39.5

Re: [PATCH 2/6] rv: Merge struct rv_monitor_def into struct rv_monitor

[Gabriele Monaco]

On Mon, 2025-07-21 at 11:47 +0200, Nam Cao wrote:
> Each struct rv_monitor has a unique struct rv_monitor_def associated
> with
> it. struct rv_monitor is statically allocated, while struct
> rv_monitor_def
> is dynamically allocated.
> 
> This makes the code more complicated than it should be:
> 
>   - Lookup is required to get the associated rv_monitor_def from
> rv_monitor
> 
>   - Dynamic memory allocation is required for rv_monitor_def. This is
>     harder to get right compared to static memory. For instance,
> there is an existing mistake: rv_unregister_monitor() does not free
> the memory allocated by rv_register_monitor(). This is fortunately
> not a real memory leak problem, as rv_unregister_monitor() is never
> called.
> 
> Simplify and merge rv_monitor_def into rv_monitor.
> 
> Signed-off-by: Nam Cao <namcao@linutronix.de>

Right, I don't see a valid reason for keeping it separate, considering
that's also simplifying things.

Reviewed-by: Gabriele Monaco <gmonaco@redhat.com>

Thanks,
Gabriele

> ---
>  include/linux/rv.h            |   8 ++
>  kernel/trace/rv/rv.c          | 211 +++++++++++++++-----------------
> --
>  kernel/trace/rv/rv.h          |  27 ++---
>  kernel/trace/rv/rv_reactors.c |  62 +++++-----
>  4 files changed, 140 insertions(+), 168 deletions(-)
> 
> diff --git a/include/linux/rv.h b/include/linux/rv.h
> index 97baf58d88b2..dba53aecdfab 100644
> --- a/include/linux/rv.h
> +++ b/include/linux/rv.h
> @@ -7,6 +7,9 @@
>  #ifndef _LINUX_RV_H
>  #define _LINUX_RV_H
>  
> +#include <linux/types.h>
> +#include <linux/list.h>
> +
>  #define MAX_DA_NAME_LEN	32
>  
>  #ifdef CONFIG_RV
> @@ -98,8 +101,13 @@ struct rv_monitor {
>  	void			(*disable)(void);
>  	void			(*reset)(void);
>  #ifdef CONFIG_RV_REACTORS
> +	struct rv_reactor_def	*rdef;
>  	__printf(1, 2) void	(*react)(const char *msg, ...);
> +	bool			reacting;
>  #endif
> +	struct list_head	list;
> +	struct rv_monitor	*parent;
> +	struct dentry		*root_d;
>  };
>  
>  bool rv_monitoring_on(void);
> diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
> index 108429d16ec1..6c0be2fdc52d 100644
> --- a/kernel/trace/rv/rv.c
> +++ b/kernel/trace/rv/rv.c
> @@ -210,9 +210,9 @@ void rv_put_task_monitor_slot(int slot)
>   * Monitors with a parent are nested,
>   * Monitors without a parent could be standalone or containers.
>   */
> -bool rv_is_nested_monitor(struct rv_monitor_def *mdef)
> +bool rv_is_nested_monitor(struct rv_monitor *mon)
>  {
> -	return mdef->parent != NULL;
> +	return mon->parent != NULL;
>  }
>  
>  /*
> @@ -223,16 +223,16 @@ bool rv_is_nested_monitor(struct rv_monitor_def
> *mdef)
>   * for enable()/disable(). Use this condition to find empty
> containers.
>   * Keep both conditions in case we have some non-compliant
> containers.
>   */
> -bool rv_is_container_monitor(struct rv_monitor_def *mdef)
> +bool rv_is_container_monitor(struct rv_monitor *mon)
>  {
> -	struct rv_monitor_def *next;
> +	struct rv_monitor *next;
>  
> -	if (list_is_last(&mdef->list, &rv_monitors_list))
> +	if (list_is_last(&mon->list, &rv_monitors_list))
>  		return false;
>  
> -	next = list_next_entry(mdef, list);
> +	next = list_next_entry(mon, list);
>  
> -	return next->parent == mdef->monitor || !mdef->monitor-
> >enable;
> +	return next->parent == mon || !mon->enable;
>  }
>  
>  /*
> @@ -241,10 +241,10 @@ bool rv_is_container_monitor(struct
> rv_monitor_def *mdef)
>  static ssize_t monitor_enable_read_data(struct file *filp, char
> __user *user_buf, size_t count,
>  					loff_t *ppos)
>  {
> -	struct rv_monitor_def *mdef = filp->private_data;
> +	struct rv_monitor *mon = filp->private_data;
>  	const char *buff;
>  
> -	buff = mdef->monitor->enabled ? "1\n" : "0\n";
> +	buff = mon->enabled ? "1\n" : "0\n";
>  
>  	return simple_read_from_buffer(user_buf, count, ppos, buff,
> strlen(buff)+1);
>  }
> @@ -252,14 +252,14 @@ static ssize_t monitor_enable_read_data(struct
> file *filp, char __user *user_buf
>  /*
>   * __rv_disable_monitor - disabled an enabled monitor
>   */
> -static int __rv_disable_monitor(struct rv_monitor_def *mdef, bool
> sync)
> +static int __rv_disable_monitor(struct rv_monitor *mon, bool sync)
>  {
>  	lockdep_assert_held(&rv_interface_lock);
>  
> -	if (mdef->monitor->enabled) {
> -		mdef->monitor->enabled = 0;
> -		if (mdef->monitor->disable)
> -			mdef->monitor->disable();
> +	if (mon->enabled) {
> +		mon->enabled = 0;
> +		if (mon->disable)
> +			mon->disable();
>  
>  		/*
>  		 * Wait for the execution of all events to finish.
> @@ -273,90 +273,90 @@ static int __rv_disable_monitor(struct
> rv_monitor_def *mdef, bool sync)
>  	return 0;
>  }
>  
> -static void rv_disable_single(struct rv_monitor_def *mdef)
> +static void rv_disable_single(struct rv_monitor *mon)
>  {
> -	__rv_disable_monitor(mdef, true);
> +	__rv_disable_monitor(mon, true);
>  }
>  
> -static int rv_enable_single(struct rv_monitor_def *mdef)
> +static int rv_enable_single(struct rv_monitor *mon)
>  {
>  	int retval;
>  
>  	lockdep_assert_held(&rv_interface_lock);
>  
> -	if (mdef->monitor->enabled)
> +	if (mon->enabled)
>  		return 0;
>  
> -	retval = mdef->monitor->enable();
> +	retval = mon->enable();
>  
>  	if (!retval)
> -		mdef->monitor->enabled = 1;
> +		mon->enabled = 1;
>  
>  	return retval;
>  }
>  
> -static void rv_disable_container(struct rv_monitor_def *mdef)
> +static void rv_disable_container(struct rv_monitor *mon)
>  {
> -	struct rv_monitor_def *p = mdef;
> +	struct rv_monitor *p = mon;
>  	int enabled = 0;
>  
>  	list_for_each_entry_continue(p, &rv_monitors_list, list) {
> -		if (p->parent != mdef->monitor)
> +		if (p->parent != mon)
>  			break;
>  		enabled += __rv_disable_monitor(p, false);
>  	}
>  	if (enabled)
>  		tracepoint_synchronize_unregister();
> -	mdef->monitor->enabled = 0;
> +	mon->enabled = 0;
>  }
>  
> -static int rv_enable_container(struct rv_monitor_def *mdef)
> +static int rv_enable_container(struct rv_monitor *mon)
>  {
> -	struct rv_monitor_def *p = mdef;
> +	struct rv_monitor *p = mon;
>  	int retval = 0;
>  
>  	list_for_each_entry_continue(p, &rv_monitors_list, list) {
> -		if (retval || p->parent != mdef->monitor)
> +		if (retval || p->parent != mon)
>  			break;
>  		retval = rv_enable_single(p);
>  	}
>  	if (retval)
> -		rv_disable_container(mdef);
> +		rv_disable_container(mon);
>  	else
> -		mdef->monitor->enabled = 1;
> +		mon->enabled = 1;
>  	return retval;
>  }
>  
>  /**
>   * rv_disable_monitor - disable a given runtime monitor
> - * @mdef: Pointer to the monitor definition structure.
> + * @mon: Pointer to the monitor definition structure.
>   *
>   * Returns 0 on success.
>   */
> -int rv_disable_monitor(struct rv_monitor_def *mdef)
> +int rv_disable_monitor(struct rv_monitor *mon)
>  {
> -	if (rv_is_container_monitor(mdef))
> -		rv_disable_container(mdef);
> +	if (rv_is_container_monitor(mon))
> +		rv_disable_container(mon);
>  	else
> -		rv_disable_single(mdef);
> +		rv_disable_single(mon);
>  
>  	return 0;
>  }
>  
>  /**
>   * rv_enable_monitor - enable a given runtime monitor
> - * @mdef: Pointer to the monitor definition structure.
> + * @mon: Pointer to the monitor definition structure.
>   *
>   * Returns 0 on success, error otherwise.
>   */
> -int rv_enable_monitor(struct rv_monitor_def *mdef)
> +int rv_enable_monitor(struct rv_monitor *mon)
>  {
>  	int retval;
>  
> -	if (rv_is_container_monitor(mdef))
> -		retval = rv_enable_container(mdef);
> +	if (rv_is_container_monitor(mon))
> +		retval = rv_enable_container(mon);
>  	else
> -		retval = rv_enable_single(mdef);
> +		retval = rv_enable_single(mon);
>  
>  	return retval;
>  }
> @@ -367,7 +367,7 @@ int rv_enable_monitor(struct rv_monitor_def
> *mdef)
>  static ssize_t monitor_enable_write_data(struct file *filp, const
> char __user *user_buf,
>  					 size_t count, loff_t *ppos)
>  {
> -	struct rv_monitor_def *mdef = filp->private_data;
> +	struct rv_monitor *mon = filp->private_data;
>  	int retval;
>  	bool val;
>  
> @@ -378,9 +378,9 @@ static ssize_t monitor_enable_write_data(struct
> file *filp, const char __user *u
>  	mutex_lock(&rv_interface_lock);
>  
>  	if (val)
> -		retval = rv_enable_monitor(mdef);
> +		retval = rv_enable_monitor(mon);
>  	else
> -		retval = rv_disable_monitor(mdef);
> +		retval = rv_disable_monitor(mon);
>  
>  	mutex_unlock(&rv_interface_lock);
>  
> @@ -399,12 +399,12 @@ static const struct file_operations
> interface_enable_fops = {
>  static ssize_t monitor_desc_read_data(struct file *filp, char __user
> *user_buf, size_t count,
>  				      loff_t *ppos)
>  {
> -	struct rv_monitor_def *mdef = filp->private_data;
> +	struct rv_monitor *mon = filp->private_data;
>  	char buff[256];
>  
>  	memset(buff, 0, sizeof(buff));
>  
> -	snprintf(buff, sizeof(buff), "%s\n", mdef->monitor-
> >description);
> +	snprintf(buff, sizeof(buff), "%s\n", mon->description);
>  
>  	return simple_read_from_buffer(user_buf, count, ppos, buff,
> strlen(buff) + 1);
>  }
> @@ -419,37 +419,37 @@ static const struct file_operations
> interface_desc_fops = {
>   * the monitor dir, where the specific options of the monitor
>   * are exposed.
>   */
> -static int create_monitor_dir(struct rv_monitor_def *mdef, struct
> rv_monitor_def *parent)
> +static int create_monitor_dir(struct rv_monitor *mon, struct
> rv_monitor *parent)
>  {
>  	struct dentry *root = parent ? parent->root_d :
> get_monitors_root();
> -	const char *name = mdef->monitor->name;
> +	const char *name = mon->name;
>  	struct dentry *tmp;
>  	int retval;
>  
> -	mdef->root_d = rv_create_dir(name, root);
> -	if (!mdef->root_d)
> +	mon->root_d = rv_create_dir(name, root);
> +	if (!mon->root_d)
>  		return -ENOMEM;
>  
> -	tmp = rv_create_file("enable", RV_MODE_WRITE, mdef->root_d,
> mdef, &interface_enable_fops);
> +	tmp = rv_create_file("enable", RV_MODE_WRITE, mon->root_d,
> mon, &interface_enable_fops);
>  	if (!tmp) {
>  		retval = -ENOMEM;
>  		goto out_remove_root;
>  	}
>  
> -	tmp = rv_create_file("desc", RV_MODE_READ, mdef->root_d,
> mdef, &interface_desc_fops);
> +	tmp = rv_create_file("desc", RV_MODE_READ, mon->root_d, mon,
> &interface_desc_fops);
>  	if (!tmp) {
>  		retval = -ENOMEM;
>  		goto out_remove_root;
>  	}
>  
> -	retval = reactor_populate_monitor(mdef);
> +	retval = reactor_populate_monitor(mon);
>  	if (retval)
>  		goto out_remove_root;
>  
>  	return 0;
>  
>  out_remove_root:
> -	rv_remove(mdef->root_d);
> +	rv_remove(mon->root_d);
>  	return retval;
>  }
>  
> @@ -458,13 +458,12 @@ static int create_monitor_dir(struct
> rv_monitor_def *mdef, struct rv_monitor_def
>   */
>  static int monitors_show(struct seq_file *m, void *p)
>  {
> -	struct rv_monitor_def *mon_def = p;
> +	struct rv_monitor *mon = p;
>  
> -	if (mon_def->parent)
> -		seq_printf(m, "%s:%s\n", mon_def->parent->name,
> -			   mon_def->monitor->name);
> +	if (mon->parent)
> +		seq_printf(m, "%s:%s\n", mon->parent->name, mon-
> >name);
>  	else
> -		seq_printf(m, "%s\n", mon_def->monitor->name);
> +		seq_printf(m, "%s\n", mon->name);
>  	return 0;
>  }
>  
> @@ -496,13 +495,13 @@ static void *available_monitors_next(struct
> seq_file *m, void *p, loff_t *pos)
>   */
>  static void *enabled_monitors_next(struct seq_file *m, void *p,
> loff_t *pos)
>  {
> -	struct rv_monitor_def *m_def = p;
> +	struct rv_monitor *mon = p;
>  
>  	(*pos)++;
>  
> -	list_for_each_entry_continue(m_def, &rv_monitors_list, list)
> {
> -		if (m_def->monitor->enabled)
> -			return m_def;
> +	list_for_each_entry_continue(mon, &rv_monitors_list, list) {
> +		if (mon->enabled)
> +			return mon;
>  	}
>  
>  	return NULL;
> @@ -510,7 +509,7 @@ static void *enabled_monitors_next(struct
> seq_file *m, void *p, loff_t *pos)
>  
>  static void *enabled_monitors_start(struct seq_file *m, loff_t *pos)
>  {
> -	struct rv_monitor_def *m_def;
> +	struct rv_monitor *mon;
>  	loff_t l;
>  
>  	mutex_lock(&rv_interface_lock);
> @@ -518,15 +517,15 @@ static void *enabled_monitors_start(struct
> seq_file *m, loff_t *pos)
>  	if (list_empty(&rv_monitors_list))
>  		return NULL;
>  
> -	m_def = list_entry(&rv_monitors_list, struct rv_monitor_def,
> list);
> +	mon = list_entry(&rv_monitors_list, struct rv_monitor,
> list);
>  
>  	for (l = 0; l <= *pos; ) {
> -		m_def = enabled_monitors_next(m, m_def, &l);
> -		if (!m_def)
> +		mon = enabled_monitors_next(m, mon, &l);
> +		if (!mon)
>  			break;
>  	}
>  
> -	return m_def;
> +	return mon;
>  }
>  
>  /*
> @@ -566,13 +565,13 @@ static const struct file_operations
> available_monitors_ops = {
>   */
>  static void disable_all_monitors(void)
>  {
> -	struct rv_monitor_def *mdef;
> +	struct rv_monitor *mon;
>  	int enabled = 0;
>  
>  	mutex_lock(&rv_interface_lock);
>  
> -	list_for_each_entry(mdef, &rv_monitors_list, list)
> -		enabled += __rv_disable_monitor(mdef, false);
> +	list_for_each_entry(mon, &rv_monitors_list, list)
> +		enabled += __rv_disable_monitor(mon, false);
>  
>  	if (enabled) {
>  		/*
> @@ -598,7 +597,7 @@ static ssize_t enabled_monitors_write(struct file
> *filp, const char __user *user
>  				      size_t count, loff_t *ppos)
>  {
>  	char buff[MAX_RV_MONITOR_NAME_SIZE + 2];
> -	struct rv_monitor_def *mdef;
> +	struct rv_monitor *mon;
>  	int retval = -EINVAL;
>  	bool enable = true;
>  	char *ptr, *tmp;
> @@ -633,17 +632,17 @@ static ssize_t enabled_monitors_write(struct
> file *filp, const char __user *user
>  	if (tmp)
>  		ptr = tmp+1;
>  
> -	list_for_each_entry(mdef, &rv_monitors_list, list) {
> -		if (strcmp(ptr, mdef->monitor->name) != 0)
> +	list_for_each_entry(mon, &rv_monitors_list, list) {
> +		if (strcmp(ptr, mon->name) != 0)
>  			continue;
>  
>  		/*
>  		 * Monitor found!
>  		 */
>  		if (enable)
> -			retval = rv_enable_monitor(mdef);
> +			retval = rv_enable_monitor(mon);
>  		else
> -			retval = rv_disable_monitor(mdef);
> +			retval = rv_disable_monitor(mon);
>  
>  		if (!retval)
>  			retval = count;
> @@ -702,11 +701,11 @@ static void turn_monitoring_off(void)
>  
>  static void reset_all_monitors(void)
>  {
> -	struct rv_monitor_def *mdef;
> +	struct rv_monitor *mon;
>  
> -	list_for_each_entry(mdef, &rv_monitors_list, list) {
> -		if (mdef->monitor->enabled && mdef->monitor->reset)
> -			mdef->monitor->reset();
> +	list_for_each_entry(mon, &rv_monitors_list, list) {
> +		if (mon->enabled && mon->reset)
> +			mon->reset();
>  	}
>  }
>  
> @@ -768,10 +767,10 @@ static const struct file_operations
> monitoring_on_fops = {
>  	.read   = monitoring_on_read_data,
>  };
>  
> -static void destroy_monitor_dir(struct rv_monitor_def *mdef)
> +static void destroy_monitor_dir(struct rv_monitor *mon)
>  {
> -	reactor_cleanup_monitor(mdef);
> -	rv_remove(mdef->root_d);
> +	reactor_cleanup_monitor(mon);
> +	rv_remove(mon->root_d);
>  }
>  
>  /**
> @@ -783,7 +782,7 @@ static void destroy_monitor_dir(struct
> rv_monitor_def *mdef)
>   */
>  int rv_register_monitor(struct rv_monitor *monitor, struct
> rv_monitor *parent)
>  {
> -	struct rv_monitor_def *r, *p = NULL;
> +	struct rv_monitor *r;
>  	int retval = 0;
>  
>  	if (strlen(monitor->name) >= MAX_RV_MONITOR_NAME_SIZE) {
> @@ -795,49 +794,31 @@ int rv_register_monitor(struct rv_monitor
> *monitor, struct rv_monitor *parent)
>  	mutex_lock(&rv_interface_lock);
>  
>  	list_for_each_entry(r, &rv_monitors_list, list) {
> -		if (strcmp(monitor->name, r->monitor->name) == 0) {
> +		if (strcmp(monitor->name, r->name) == 0) {
>  			pr_info("Monitor %s is already
> registered\n", monitor->name);
>  			retval = -EEXIST;
>  			goto out_unlock;
>  		}
>  	}
>  
> -	if (parent) {
> -		list_for_each_entry(r, &rv_monitors_list, list) {
> -			if (strcmp(parent->name, r->monitor->name)
> == 0) {
> -				p = r;
> -				break;
> -			}
> -		}
> -	}
> -
> -	if (p && rv_is_nested_monitor(p)) {
> +	if (parent && rv_is_nested_monitor(parent)) {
>  		pr_info("Parent monitor %s is already nested, cannot
> nest further\n",
>  			parent->name);
>  		retval = -EINVAL;
>  		goto out_unlock;
>  	}
>  
> -	r = kzalloc(sizeof(struct rv_monitor_def), GFP_KERNEL);
> -	if (!r) {
> -		retval = -ENOMEM;
> -		goto out_unlock;
> -	}
> -
> -	r->monitor = monitor;
> -	r->parent = parent;
> +	monitor->parent = parent;
>  
> -	retval = create_monitor_dir(r, p);
> -	if (retval) {
> -		kfree(r);
> -		goto out_unlock;
> -	}
> +	retval = create_monitor_dir(monitor, parent);
> +	if (retval)
> +		return retval;
>  
>  	/* keep children close to the parent for easier
> visualisation */
> -	if (p)
> -		list_add(&r->list, &p->list);
> +	if (parent)
> +		list_add(&monitor->list, &parent->list);
>  	else
> -		list_add_tail(&r->list, &rv_monitors_list);
> +		list_add_tail(&monitor->list, &rv_monitors_list);
>  
>  out_unlock:
>  	mutex_unlock(&rv_interface_lock);
> @@ -852,17 +833,11 @@ int rv_register_monitor(struct rv_monitor
> *monitor, struct rv_monitor *parent)
>   */
>  int rv_unregister_monitor(struct rv_monitor *monitor)
>  {
> -	struct rv_monitor_def *ptr, *next;
> -
>  	mutex_lock(&rv_interface_lock);
>  
> -	list_for_each_entry_safe(ptr, next, &rv_monitors_list, list)
> {
> -		if (strcmp(monitor->name, ptr->monitor->name) == 0)
> {
> -			rv_disable_monitor(ptr);
> -			list_del(&ptr->list);
> -			destroy_monitor_dir(ptr);
> -		}
> -	}
> +	rv_disable_monitor(monitor);
> +	list_del(&monitor->list);
> +	destroy_monitor_dir(monitor);
>  
>  	mutex_unlock(&rv_interface_lock);
>  	return 0;
> diff --git a/kernel/trace/rv/rv.h b/kernel/trace/rv/rv.h
> index 873364094402..f039ec1c9156 100644
> --- a/kernel/trace/rv/rv.h
> +++ b/kernel/trace/rv/rv.h
> @@ -32,34 +32,23 @@ struct rv_reactor_def {
>  };
>  #endif
>  
> -struct rv_monitor_def {
> -	struct list_head	list;
> -	struct rv_monitor	*monitor;
> -	struct rv_monitor	*parent;
> -	struct dentry		*root_d;
> -#ifdef CONFIG_RV_REACTORS
> -	struct rv_reactor_def	*rdef;
> -	bool			reacting;
> -#endif
> -};
> -
>  struct dentry *get_monitors_root(void);
> -int rv_disable_monitor(struct rv_monitor_def *mdef);
> -int rv_enable_monitor(struct rv_monitor_def *mdef);
> -bool rv_is_container_monitor(struct rv_monitor_def *mdef);
> -bool rv_is_nested_monitor(struct rv_monitor_def *mdef);
> +int rv_disable_monitor(struct rv_monitor *mon);
> +int rv_enable_monitor(struct rv_monitor *mon);
> +bool rv_is_container_monitor(struct rv_monitor *mon);
> +bool rv_is_nested_monitor(struct rv_monitor *mon);
>  
>  #ifdef CONFIG_RV_REACTORS
> -int reactor_populate_monitor(struct rv_monitor_def *mdef);
> -void reactor_cleanup_monitor(struct rv_monitor_def *mdef);
> +int reactor_populate_monitor(struct rv_monitor *mon);
> +void reactor_cleanup_monitor(struct rv_monitor *mon);
>  int init_rv_reactors(struct dentry *root_dir);
>  #else
> -static inline int reactor_populate_monitor(struct rv_monitor_def
> *mdef)
> +static inline int reactor_populate_monitor(struct rv_monitor *mon)
>  {
>  	return 0;
>  }
>  
> -static inline void reactor_cleanup_monitor(struct rv_monitor_def
> *mdef)
> +static inline void reactor_cleanup_monitor(struct rv_monitor *mon)
>  {
>  	return;
>  }
> diff --git a/kernel/trace/rv/rv_reactors.c
> b/kernel/trace/rv/rv_reactors.c
> index 740603670dd1..7cc620a1be1a 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -138,10 +138,10 @@ static const struct file_operations
> available_reactors_ops = {
>   */
>  static int monitor_reactor_show(struct seq_file *m, void *p)
>  {
> -	struct rv_monitor_def *mdef = m->private;
> +	struct rv_monitor *mon = m->private;
>  	struct rv_reactor_def *rdef = p;
>  
> -	if (mdef->rdef == rdef)
> +	if (mon->rdef == rdef)
>  		seq_printf(m, "[%s]\n", rdef->reactor->name);
>  	else
>  		seq_printf(m, "%s\n", rdef->reactor->name);
> @@ -158,41 +158,41 @@ static const struct seq_operations
> monitor_reactors_seq_ops = {
>  	.show	= monitor_reactor_show
>  };
>  
> -static void monitor_swap_reactors_single(struct rv_monitor_def
> *mdef,
> +static void monitor_swap_reactors_single(struct rv_monitor *mon,
>  					 struct rv_reactor_def
> *rdef,
>  					 bool reacting, bool nested)
>  {
>  	bool monitor_enabled;
>  
>  	/* nothing to do */
> -	if (mdef->rdef == rdef)
> +	if (mon->rdef == rdef)
>  		return;
>  
> -	monitor_enabled = mdef->monitor->enabled;
> +	monitor_enabled = mon->enabled;
>  	if (monitor_enabled)
> -		rv_disable_monitor(mdef);
> +		rv_disable_monitor(mon);
>  
>  	/* swap reactor's usage */
> -	mdef->rdef->counter--;
> +	mon->rdef->counter--;
>  	rdef->counter++;
>  
> -	mdef->rdef = rdef;
> -	mdef->reacting = reacting;
> -	mdef->monitor->react = rdef->reactor->react;
> +	mon->rdef = rdef;
> +	mon->reacting = reacting;
> +	mon->react = rdef->reactor->react;
>  
>  	/* enable only once if iterating through a container */
>  	if (monitor_enabled && !nested)
> -		rv_enable_monitor(mdef);
> +		rv_enable_monitor(mon);
>  }
>  
> -static void monitor_swap_reactors(struct rv_monitor_def *mdef,
> +static void monitor_swap_reactors(struct rv_monitor *mon,
>  				  struct rv_reactor_def *rdef, bool
> reacting)
>  {
> -	struct rv_monitor_def *p = mdef;
> +	struct rv_monitor *p = mon;
>  
> -	if (rv_is_container_monitor(mdef))
> +	if (rv_is_container_monitor(mon))
>  		list_for_each_entry_continue(p, &rv_monitors_list,
> list) {
> -			if (p->parent != mdef->monitor)
> +			if (p->parent != mon)
>  				break;
>  			monitor_swap_reactors_single(p, rdef,
> reacting, true);
>  		}
> @@ -202,7 +202,7 @@ static void monitor_swap_reactors(struct
> rv_monitor_def *mdef,
>  	 * All nested monitors are enabled also if they were off, we
> may refine
>  	 * this logic in the future.
>  	 */
> -	monitor_swap_reactors_single(mdef, rdef, reacting, false);
> +	monitor_swap_reactors_single(mon, rdef, reacting, false);
>  }
>  
>  static ssize_t
> @@ -210,7 +210,7 @@ monitor_reactors_write(struct file *file, const
> char __user *user_buf,
>  		      size_t count, loff_t *ppos)
>  {
>  	char buff[MAX_RV_REACTOR_NAME_SIZE + 2];
> -	struct rv_monitor_def *mdef;
> +	struct rv_monitor *mon;
>  	struct rv_reactor_def *rdef;
>  	struct seq_file *seq_f;
>  	int retval = -EINVAL;
> @@ -237,7 +237,7 @@ monitor_reactors_write(struct file *file, const
> char __user *user_buf,
>  	 * See monitor_reactors_open()
>  	 */
>  	seq_f = file->private_data;
> -	mdef = seq_f->private;
> +	mon = seq_f->private;
>  
>  	mutex_lock(&rv_interface_lock);
>  
> @@ -252,7 +252,7 @@ monitor_reactors_write(struct file *file, const
> char __user *user_buf,
>  		else
>  			enable = true;
>  
> -		monitor_swap_reactors(mdef, rdef, enable);
> +		monitor_swap_reactors(mon, rdef, enable);
>  
>  		retval = count;
>  		break;
> @@ -268,7 +268,7 @@ monitor_reactors_write(struct file *file, const
> char __user *user_buf,
>   */
>  static int monitor_reactors_open(struct inode *inode, struct file
> *file)
>  {
> -	struct rv_monitor_def *mdef = inode->i_private;
> +	struct rv_monitor *mon = inode->i_private;
>  	struct seq_file *seq_f;
>  	int ret;
>  
> @@ -284,7 +284,7 @@ static int monitor_reactors_open(struct inode
> *inode, struct file *file)
>  	/*
>  	 * Copy the create file "private" data to the seq_file
> private data.
>  	 */
> -	seq_f->private = mdef;
> +	seq_f->private = mon;
>  
>  	return 0;
>  };
> @@ -454,37 +454,37 @@ static const struct file_operations
> reacting_on_fops = {
>  
>  /**
>   * reactor_populate_monitor - creates per monitor reactors file
> - * @mdef:	monitor's definition.
> + * @mon:	The monitor.
>   *
>   * Returns 0 if successful, error otherwise.
>   */
> -int reactor_populate_monitor(struct rv_monitor_def *mdef)
> +int reactor_populate_monitor(struct rv_monitor *mon)
>  {
>  	struct dentry *tmp;
>  
> -	tmp = rv_create_file("reactors", RV_MODE_WRITE, mdef-
> >root_d, mdef, &monitor_reactors_ops);
> +	tmp = rv_create_file("reactors", RV_MODE_WRITE, mon->root_d,
> mon, &monitor_reactors_ops);
>  	if (!tmp)
>  		return -ENOMEM;
>  
>  	/*
>  	 * Configure as the rv_nop reactor.
>  	 */
> -	mdef->rdef = get_reactor_rdef_by_name("nop");
> -	mdef->rdef->counter++;
> -	mdef->reacting = false;
> +	mon->rdef = get_reactor_rdef_by_name("nop");
> +	mon->rdef->counter++;
> +	mon->reacting = false;
>  
>  	return 0;
>  }
>  
>  /**
>   * reactor_cleanup_monitor - cleanup a monitor reference
> - * @mdef:       monitor's definition.
> + * @mon:       the monitor.
>   */
> -void reactor_cleanup_monitor(struct rv_monitor_def *mdef)
> +void reactor_cleanup_monitor(struct rv_monitor *mon)
>  {
>  	lockdep_assert_held(&rv_interface_lock);
> -	mdef->rdef->counter--;
> -	WARN_ON_ONCE(mdef->rdef->counter < 0);
> +	mon->rdef->counter--;
> +	WARN_ON_ONCE(mon->rdef->counter < 0);
>  }
>  
>  /*

[PATCH 3/6] rv: Merge struct rv_reactor_def into struct rv_reactor

[Nam Cao]

Each struct rv_reactor has a unique struct rv_reactor_def associated with
it. struct rv_reactor is statically allocated, while struct rv_reactor_def
is dynamically allocated.

This makes the code more complicated than it should be:

  - Lookup is required to get the associated rv_reactor_def from rv_reactor

  - Dynamic memory allocation is required for rv_reactor_def. This is
    harder to get right compared to static memory. For instance, there is
    an existing mistake: rv_unregister_reactor() does not free the memory
    allocated by rv_register_reactor(). This is fortunately not a real
    memory leak problem as rv_unregister_reactor() is never called.

Simplify and merge rv_reactor_def into rv_reactor.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 include/linux/rv.h            |  5 +-
 kernel/trace/rv/rv.h          |  9 ----
 kernel/trace/rv/rv_reactors.c | 92 +++++++++++++++--------------------
 3 files changed, 43 insertions(+), 63 deletions(-)

diff --git a/include/linux/rv.h b/include/linux/rv.h
index dba53aecdfab..c22c9b8c1567 100644
--- a/include/linux/rv.h
+++ b/include/linux/rv.h
@@ -90,6 +90,9 @@ struct rv_reactor {
 	const char		*name;
 	const char		*description;
 	__printf(1, 2) void	(*react)(const char *msg, ...);
+	struct list_head	list;
+	/* protected by the monitor interface lock */
+	int			counter;
 };
 #endif
 
@@ -101,7 +104,7 @@ struct rv_monitor {
 	void			(*disable)(void);
 	void			(*reset)(void);
 #ifdef CONFIG_RV_REACTORS
-	struct rv_reactor_def	*rdef;
+	struct rv_reactor	*reactor;
 	__printf(1, 2) void	(*react)(const char *msg, ...);
 	bool			reacting;
 #endif
diff --git a/kernel/trace/rv/rv.h b/kernel/trace/rv/rv.h
index f039ec1c9156..8c38f9dd41bc 100644
--- a/kernel/trace/rv/rv.h
+++ b/kernel/trace/rv/rv.h
@@ -23,15 +23,6 @@ struct rv_interface {
 extern struct mutex rv_interface_lock;
 extern struct list_head rv_monitors_list;
 
-#ifdef CONFIG_RV_REACTORS
-struct rv_reactor_def {
-	struct list_head	list;
-	struct rv_reactor	*reactor;
-	/* protected by the monitor interface lock */
-	int			counter;
-};
-#endif
-
 struct dentry *get_monitors_root(void);
 int rv_disable_monitor(struct rv_monitor *mon);
 int rv_enable_monitor(struct rv_monitor *mon);
diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index 7cc620a1be1a..2c7909e6d0e7 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -70,12 +70,12 @@
  */
 static LIST_HEAD(rv_reactors_list);
 
-static struct rv_reactor_def *get_reactor_rdef_by_name(char *name)
+static struct rv_reactor *get_reactor_rdef_by_name(char *name)
 {
-	struct rv_reactor_def *r;
+	struct rv_reactor *r;
 
 	list_for_each_entry(r, &rv_reactors_list, list) {
-		if (strcmp(name, r->reactor->name) == 0)
+		if (strcmp(name, r->name) == 0)
 			return r;
 	}
 	return NULL;
@@ -86,9 +86,9 @@ static struct rv_reactor_def *get_reactor_rdef_by_name(char *name)
  */
 static int reactors_show(struct seq_file *m, void *p)
 {
-	struct rv_reactor_def *rea_def = p;
+	struct rv_reactor *reactor = p;
 
-	seq_printf(m, "%s\n", rea_def->reactor->name);
+	seq_printf(m, "%s\n", reactor->name);
 	return 0;
 }
 
@@ -139,12 +139,12 @@ static const struct file_operations available_reactors_ops = {
 static int monitor_reactor_show(struct seq_file *m, void *p)
 {
 	struct rv_monitor *mon = m->private;
-	struct rv_reactor_def *rdef = p;
+	struct rv_reactor *reactor = p;
 
-	if (mon->rdef == rdef)
-		seq_printf(m, "[%s]\n", rdef->reactor->name);
+	if (mon->reactor == reactor)
+		seq_printf(m, "[%s]\n", reactor->name);
 	else
-		seq_printf(m, "%s\n", rdef->reactor->name);
+		seq_printf(m, "%s\n", reactor->name);
 	return 0;
 }
 
@@ -159,13 +159,13 @@ static const struct seq_operations monitor_reactors_seq_ops = {
 };
 
 static void monitor_swap_reactors_single(struct rv_monitor *mon,
-					 struct rv_reactor_def *rdef,
+					 struct rv_reactor *reactor,
 					 bool reacting, bool nested)
 {
 	bool monitor_enabled;
 
 	/* nothing to do */
-	if (mon->rdef == rdef)
+	if (mon->reactor == reactor)
 		return;
 
 	monitor_enabled = mon->enabled;
@@ -173,12 +173,12 @@ static void monitor_swap_reactors_single(struct rv_monitor *mon,
 		rv_disable_monitor(mon);
 
 	/* swap reactor's usage */
-	mon->rdef->counter--;
-	rdef->counter++;
+	mon->reactor->counter--;
+	reactor->counter++;
 
-	mon->rdef = rdef;
+	mon->reactor = reactor;
 	mon->reacting = reacting;
-	mon->react = rdef->reactor->react;
+	mon->react = reactor->react;
 
 	/* enable only once if iterating through a container */
 	if (monitor_enabled && !nested)
@@ -186,7 +186,7 @@ static void monitor_swap_reactors_single(struct rv_monitor *mon,
 }
 
 static void monitor_swap_reactors(struct rv_monitor *mon,
-				  struct rv_reactor_def *rdef, bool reacting)
+				  struct rv_reactor *reactor, bool reacting)
 {
 	struct rv_monitor *p = mon;
 
@@ -194,7 +194,7 @@ static void monitor_swap_reactors(struct rv_monitor *mon,
 		list_for_each_entry_continue(p, &rv_monitors_list, list) {
 			if (p->parent != mon)
 				break;
-			monitor_swap_reactors_single(p, rdef, reacting, true);
+			monitor_swap_reactors_single(p, reactor, reacting, true);
 		}
 	/*
 	 * This call enables and disables the monitor if they were active.
@@ -202,7 +202,7 @@ static void monitor_swap_reactors(struct rv_monitor *mon,
 	 * All nested monitors are enabled also if they were off, we may refine
 	 * this logic in the future.
 	 */
-	monitor_swap_reactors_single(mon, rdef, reacting, false);
+	monitor_swap_reactors_single(mon, reactor, reacting, false);
 }
 
 static ssize_t
@@ -211,7 +211,7 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
 {
 	char buff[MAX_RV_REACTOR_NAME_SIZE + 2];
 	struct rv_monitor *mon;
-	struct rv_reactor_def *rdef;
+	struct rv_reactor *reactor;
 	struct seq_file *seq_f;
 	int retval = -EINVAL;
 	bool enable;
@@ -243,16 +243,16 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
 
 	retval = -EINVAL;
 
-	list_for_each_entry(rdef, &rv_reactors_list, list) {
-		if (strcmp(ptr, rdef->reactor->name) != 0)
+	list_for_each_entry(reactor, &rv_reactors_list, list) {
+		if (strcmp(ptr, reactor->name) != 0)
 			continue;
 
-		if (rdef == get_reactor_rdef_by_name("nop"))
+		if (strcmp(reactor->name, "nop"))
 			enable = false;
 		else
 			enable = true;
 
-		monitor_swap_reactors(mon, rdef, enable);
+		monitor_swap_reactors(mon, reactor, enable);
 
 		retval = count;
 		break;
@@ -299,23 +299,16 @@ static const struct file_operations monitor_reactors_ops = {
 
 static int __rv_register_reactor(struct rv_reactor *reactor)
 {
-	struct rv_reactor_def *r;
+	struct rv_reactor *r;
 
 	list_for_each_entry(r, &rv_reactors_list, list) {
-		if (strcmp(reactor->name, r->reactor->name) == 0) {
+		if (strcmp(reactor->name, r->name) == 0) {
 			pr_info("Reactor %s is already registered\n", reactor->name);
 			return -EINVAL;
 		}
 	}
 
-	r = kzalloc(sizeof(struct rv_reactor_def), GFP_KERNEL);
-	if (!r)
-		return -ENOMEM;
-
-	r->reactor = reactor;
-	r->counter = 0;
-
-	list_add_tail(&r->list, &rv_reactors_list);
+	list_add_tail(&reactor->list, &rv_reactors_list);
 
 	return 0;
 }
@@ -350,26 +343,19 @@ int rv_register_reactor(struct rv_reactor *reactor)
  */
 int rv_unregister_reactor(struct rv_reactor *reactor)
 {
-	struct rv_reactor_def *ptr, *next;
 	int ret = 0;
 
 	mutex_lock(&rv_interface_lock);
 
-	list_for_each_entry_safe(ptr, next, &rv_reactors_list, list) {
-		if (strcmp(reactor->name, ptr->reactor->name) == 0) {
-
-			if (!ptr->counter) {
-				list_del(&ptr->list);
-			} else {
-				printk(KERN_WARNING
-				       "rv: the rv_reactor %s is in use by %d monitor(s)\n",
-				       ptr->reactor->name, ptr->counter);
-				printk(KERN_WARNING "rv: the rv_reactor %s cannot be removed\n",
-				       ptr->reactor->name);
-				ret = -EBUSY;
-				break;
-			}
-		}
+	if (!reactor->counter) {
+		list_del(&reactor->list);
+	} else {
+		printk(KERN_WARNING
+		       "rv: the rv_reactor %s is in use by %d monitor(s)\n",
+		       reactor->name, reactor->counter);
+		printk(KERN_WARNING "rv: the rv_reactor %s cannot be removed\n",
+		       reactor->name);
+		ret = -EBUSY;
 	}
 
 	mutex_unlock(&rv_interface_lock);
@@ -469,8 +455,8 @@ int reactor_populate_monitor(struct rv_monitor *mon)
 	/*
 	 * Configure as the rv_nop reactor.
 	 */
-	mon->rdef = get_reactor_rdef_by_name("nop");
-	mon->rdef->counter++;
+	mon->reactor = get_reactor_rdef_by_name("nop");
+	mon->reactor->counter++;
 	mon->reacting = false;
 
 	return 0;
@@ -483,8 +469,8 @@ int reactor_populate_monitor(struct rv_monitor *mon)
 void reactor_cleanup_monitor(struct rv_monitor *mon)
 {
 	lockdep_assert_held(&rv_interface_lock);
-	mon->rdef->counter--;
-	WARN_ON_ONCE(mon->rdef->counter < 0);
+	mon->reactor->counter--;
+	WARN_ON_ONCE(mon->reactor->counter < 0);
 }
 
 /*
-- 
2.39.5

Re: [PATCH 3/6] rv: Merge struct rv_reactor_def into struct rv_reactor

[Gabriele Monaco]

On Mon, 2025-07-21 at 11:47 +0200, Nam Cao wrote:
> Each struct rv_reactor has a unique struct rv_reactor_def associated
> with it. struct rv_reactor is statically allocated, while struct
> rv_reactor_def is dynamically allocated.
> 
> This makes the code more complicated than it should be:
> 
>   - Lookup is required to get the associated rv_reactor_def from
> rv_reactor
> 
>   - Dynamic memory allocation is required for rv_reactor_def. This is
>     harder to get right compared to static memory. For instance,
> there is an existing mistake: rv_unregister_reactor() does not free
> the memory allocated by rv_register_reactor(). This is fortunately
> not a real memory leak problem as rv_unregister_reactor() is never
> called.
> 
> Simplify and merge rv_reactor_def into rv_reactor.
> 
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> ---

Also here, looks good to me, thanks!

Reviewed-by: Gabriele Monaco <gmonaco@redhat.com>

>  include/linux/rv.h            |  5 +-
>  kernel/trace/rv/rv.h          |  9 ----
>  kernel/trace/rv/rv_reactors.c | 92 +++++++++++++++------------------
> --
>  3 files changed, 43 insertions(+), 63 deletions(-)
> 
> diff --git a/include/linux/rv.h b/include/linux/rv.h
> index dba53aecdfab..c22c9b8c1567 100644
> --- a/include/linux/rv.h
> +++ b/include/linux/rv.h
> @@ -90,6 +90,9 @@ struct rv_reactor {
>  	const char		*name;
>  	const char		*description;
>  	__printf(1, 2) void	(*react)(const char *msg, ...);
> +	struct list_head	list;
> +	/* protected by the monitor interface lock */
> +	int			counter;
>  };
>  #endif
>  
> @@ -101,7 +104,7 @@ struct rv_monitor {
>  	void			(*disable)(void);
>  	void			(*reset)(void);
>  #ifdef CONFIG_RV_REACTORS
> -	struct rv_reactor_def	*rdef;
> +	struct rv_reactor	*reactor;
>  	__printf(1, 2) void	(*react)(const char *msg, ...);
>  	bool			reacting;
>  #endif
> diff --git a/kernel/trace/rv/rv.h b/kernel/trace/rv/rv.h
> index f039ec1c9156..8c38f9dd41bc 100644
> --- a/kernel/trace/rv/rv.h
> +++ b/kernel/trace/rv/rv.h
> @@ -23,15 +23,6 @@ struct rv_interface {
>  extern struct mutex rv_interface_lock;
>  extern struct list_head rv_monitors_list;
>  
> -#ifdef CONFIG_RV_REACTORS
> -struct rv_reactor_def {
> -	struct list_head	list;
> -	struct rv_reactor	*reactor;
> -	/* protected by the monitor interface lock */
> -	int			counter;
> -};
> -#endif
> -
>  struct dentry *get_monitors_root(void);
>  int rv_disable_monitor(struct rv_monitor *mon);
>  int rv_enable_monitor(struct rv_monitor *mon);
> diff --git a/kernel/trace/rv/rv_reactors.c
> b/kernel/trace/rv/rv_reactors.c
> index 7cc620a1be1a..2c7909e6d0e7 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -70,12 +70,12 @@
>   */
>  static LIST_HEAD(rv_reactors_list);
>  
> -static struct rv_reactor_def *get_reactor_rdef_by_name(char *name)
> +static struct rv_reactor *get_reactor_rdef_by_name(char *name)
>  {
> -	struct rv_reactor_def *r;
> +	struct rv_reactor *r;
>  
>  	list_for_each_entry(r, &rv_reactors_list, list) {
> -		if (strcmp(name, r->reactor->name) == 0)
> +		if (strcmp(name, r->name) == 0)
>  			return r;
>  	}
>  	return NULL;
> @@ -86,9 +86,9 @@ static struct rv_reactor_def
> *get_reactor_rdef_by_name(char *name)
>   */
>  static int reactors_show(struct seq_file *m, void *p)
>  {
> -	struct rv_reactor_def *rea_def = p;
> +	struct rv_reactor *reactor = p;
>  
> -	seq_printf(m, "%s\n", rea_def->reactor->name);
> +	seq_printf(m, "%s\n", reactor->name);
>  	return 0;
>  }
>  
> @@ -139,12 +139,12 @@ static const struct file_operations
> available_reactors_ops = {
>  static int monitor_reactor_show(struct seq_file *m, void *p)
>  {
>  	struct rv_monitor *mon = m->private;
> -	struct rv_reactor_def *rdef = p;
> +	struct rv_reactor *reactor = p;
>  
> -	if (mon->rdef == rdef)
> -		seq_printf(m, "[%s]\n", rdef->reactor->name);
> +	if (mon->reactor == reactor)
> +		seq_printf(m, "[%s]\n", reactor->name);
>  	else
> -		seq_printf(m, "%s\n", rdef->reactor->name);
> +		seq_printf(m, "%s\n", reactor->name);
>  	return 0;
>  }
>  
> @@ -159,13 +159,13 @@ static const struct seq_operations
> monitor_reactors_seq_ops = {
>  };
>  
>  static void monitor_swap_reactors_single(struct rv_monitor *mon,
> -					 struct rv_reactor_def
> *rdef,
> +					 struct rv_reactor *reactor,
>  					 bool reacting, bool nested)
>  {
>  	bool monitor_enabled;
>  
>  	/* nothing to do */
> -	if (mon->rdef == rdef)
> +	if (mon->reactor == reactor)
>  		return;
>  
>  	monitor_enabled = mon->enabled;
> @@ -173,12 +173,12 @@ static void monitor_swap_reactors_single(struct
> rv_monitor *mon,
>  		rv_disable_monitor(mon);
>  
>  	/* swap reactor's usage */
> -	mon->rdef->counter--;
> -	rdef->counter++;
> +	mon->reactor->counter--;
> +	reactor->counter++;
>  
> -	mon->rdef = rdef;
> +	mon->reactor = reactor;
>  	mon->reacting = reacting;
> -	mon->react = rdef->reactor->react;
> +	mon->react = reactor->react;
>  
>  	/* enable only once if iterating through a container */
>  	if (monitor_enabled && !nested)
> @@ -186,7 +186,7 @@ static void monitor_swap_reactors_single(struct
> rv_monitor *mon,
>  }
>  
>  static void monitor_swap_reactors(struct rv_monitor *mon,
> -				  struct rv_reactor_def *rdef, bool
> reacting)
> +				  struct rv_reactor *reactor, bool
> reacting)
>  {
>  	struct rv_monitor *p = mon;
>  
> @@ -194,7 +194,7 @@ static void monitor_swap_reactors(struct
> rv_monitor *mon,
>  		list_for_each_entry_continue(p, &rv_monitors_list,
> list) {
>  			if (p->parent != mon)
>  				break;
> -			monitor_swap_reactors_single(p, rdef,
> reacting, true);
> +			monitor_swap_reactors_single(p, reactor,
> reacting, true);
>  		}
>  	/*
>  	 * This call enables and disables the monitor if they were
> active.
> @@ -202,7 +202,7 @@ static void monitor_swap_reactors(struct
> rv_monitor *mon,
>  	 * All nested monitors are enabled also if they were off, we
> may refine
>  	 * this logic in the future.
>  	 */
> -	monitor_swap_reactors_single(mon, rdef, reacting, false);
> +	monitor_swap_reactors_single(mon, reactor, reacting, false);
>  }
>  
>  static ssize_t
> @@ -211,7 +211,7 @@ monitor_reactors_write(struct file *file, const
> char __user *user_buf,
>  {
>  	char buff[MAX_RV_REACTOR_NAME_SIZE + 2];
>  	struct rv_monitor *mon;
> -	struct rv_reactor_def *rdef;
> +	struct rv_reactor *reactor;
>  	struct seq_file *seq_f;
>  	int retval = -EINVAL;
>  	bool enable;
> @@ -243,16 +243,16 @@ monitor_reactors_write(struct file *file, const
> char __user *user_buf,
>  
>  	retval = -EINVAL;
>  
> -	list_for_each_entry(rdef, &rv_reactors_list, list) {
> -		if (strcmp(ptr, rdef->reactor->name) != 0)
> +	list_for_each_entry(reactor, &rv_reactors_list, list) {
> +		if (strcmp(ptr, reactor->name) != 0)
>  			continue;
>  
> -		if (rdef == get_reactor_rdef_by_name("nop"))
> +		if (strcmp(reactor->name, "nop"))
>  			enable = false;
>  		else
>  			enable = true;
>  
> -		monitor_swap_reactors(mon, rdef, enable);
> +		monitor_swap_reactors(mon, reactor, enable);
>  
>  		retval = count;
>  		break;
> @@ -299,23 +299,16 @@ static const struct file_operations
> monitor_reactors_ops = {
>  
>  static int __rv_register_reactor(struct rv_reactor *reactor)
>  {
> -	struct rv_reactor_def *r;
> +	struct rv_reactor *r;
>  
>  	list_for_each_entry(r, &rv_reactors_list, list) {
> -		if (strcmp(reactor->name, r->reactor->name) == 0) {
> +		if (strcmp(reactor->name, r->name) == 0) {
>  			pr_info("Reactor %s is already
> registered\n", reactor->name);
>  			return -EINVAL;
>  		}
>  	}
>  
> -	r = kzalloc(sizeof(struct rv_reactor_def), GFP_KERNEL);
> -	if (!r)
> -		return -ENOMEM;
> -
> -	r->reactor = reactor;
> -	r->counter = 0;
> -
> -	list_add_tail(&r->list, &rv_reactors_list);
> +	list_add_tail(&reactor->list, &rv_reactors_list);
>  
>  	return 0;
>  }
> @@ -350,26 +343,19 @@ int rv_register_reactor(struct rv_reactor
> *reactor)
>   */
>  int rv_unregister_reactor(struct rv_reactor *reactor)
>  {
> -	struct rv_reactor_def *ptr, *next;
>  	int ret = 0;
>  
>  	mutex_lock(&rv_interface_lock);
>  
> -	list_for_each_entry_safe(ptr, next, &rv_reactors_list, list)
> {
> -		if (strcmp(reactor->name, ptr->reactor->name) == 0)
> {
> -
> -			if (!ptr->counter) {
> -				list_del(&ptr->list);
> -			} else {
> -				printk(KERN_WARNING
> -				       "rv: the rv_reactor %s is in
> use by %d monitor(s)\n",
> -				       ptr->reactor->name, ptr-
> >counter);
> -				printk(KERN_WARNING "rv: the
> rv_reactor %s cannot be removed\n",
> -				       ptr->reactor->name);
> -				ret = -EBUSY;
> -				break;
> -			}
> -		}
> +	if (!reactor->counter) {
> +		list_del(&reactor->list);
> +	} else {
> +		printk(KERN_WARNING
> +		       "rv: the rv_reactor %s is in use by %d
> monitor(s)\n",
> +		       reactor->name, reactor->counter);
> +		printk(KERN_WARNING "rv: the rv_reactor %s cannot be
> removed\n",
> +		       reactor->name);
> +		ret = -EBUSY;
>  	}
>  
>  	mutex_unlock(&rv_interface_lock);
> @@ -469,8 +455,8 @@ int reactor_populate_monitor(struct rv_monitor
> *mon)
>  	/*
>  	 * Configure as the rv_nop reactor.
>  	 */
> -	mon->rdef = get_reactor_rdef_by_name("nop");
> -	mon->rdef->counter++;
> +	mon->reactor = get_reactor_rdef_by_name("nop");
> +	mon->reactor->counter++;
>  	mon->reacting = false;
>  
>  	return 0;
> @@ -483,8 +469,8 @@ int reactor_populate_monitor(struct rv_monitor
> *mon)
>  void reactor_cleanup_monitor(struct rv_monitor *mon)
>  {
>  	lockdep_assert_held(&rv_interface_lock);
> -	mon->rdef->counter--;
> -	WARN_ON_ONCE(mon->rdef->counter < 0);
> +	mon->reactor->counter--;
> +	WARN_ON_ONCE(mon->reactor->counter < 0);
>  }
>  
>  /*

[PATCH 4/6] rv: Remove rv_reactor's reference counter

[Nam Cao]

rv_reactor has a reference counter to ensure it is not removed while
monitors are still using it.

However, this is futile, as __exit functions are not expected to fail and
will proceed normally despite rv_unregister_reactor() returning an error.

At the moment, reactors do not support being built as modules, therefore
they are never removed and the reference counters are not necessary.

If we support building RV reactors as modules in the future, MODULE_SOFTDEP
should be used instead of a custom implementation.

Remove this reference counter.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 include/linux/rv.h            |  2 --
 kernel/trace/rv/rv.c          |  1 -
 kernel/trace/rv/rv.h          |  6 ------
 kernel/trace/rv/rv_reactors.c | 33 ++-------------------------------
 4 files changed, 2 insertions(+), 40 deletions(-)

diff --git a/include/linux/rv.h b/include/linux/rv.h
index c22c9b8c1567..2f867d6f72ba 100644
--- a/include/linux/rv.h
+++ b/include/linux/rv.h
@@ -91,8 +91,6 @@ struct rv_reactor {
 	const char		*description;
 	__printf(1, 2) void	(*react)(const char *msg, ...);
 	struct list_head	list;
-	/* protected by the monitor interface lock */
-	int			counter;
 };
 #endif
 
diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
index 6c0be2fdc52d..6c8498743b98 100644
--- a/kernel/trace/rv/rv.c
+++ b/kernel/trace/rv/rv.c
@@ -769,7 +769,6 @@ static const struct file_operations monitoring_on_fops = {
 
 static void destroy_monitor_dir(struct rv_monitor *mon)
 {
-	reactor_cleanup_monitor(mon);
 	rv_remove(mon->root_d);
 }
 
diff --git a/kernel/trace/rv/rv.h b/kernel/trace/rv/rv.h
index 8c38f9dd41bc..1485a70c1bf4 100644
--- a/kernel/trace/rv/rv.h
+++ b/kernel/trace/rv/rv.h
@@ -31,7 +31,6 @@ bool rv_is_nested_monitor(struct rv_monitor *mon);
 
 #ifdef CONFIG_RV_REACTORS
 int reactor_populate_monitor(struct rv_monitor *mon);
-void reactor_cleanup_monitor(struct rv_monitor *mon);
 int init_rv_reactors(struct dentry *root_dir);
 #else
 static inline int reactor_populate_monitor(struct rv_monitor *mon)
@@ -39,11 +38,6 @@ static inline int reactor_populate_monitor(struct rv_monitor *mon)
 	return 0;
 }
 
-static inline void reactor_cleanup_monitor(struct rv_monitor *mon)
-{
-	return;
-}
-
 static inline int init_rv_reactors(struct dentry *root_dir)
 {
 	return 0;
diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index 2c7909e6d0e7..a8e849e6cd85 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -172,10 +172,6 @@ static void monitor_swap_reactors_single(struct rv_monitor *mon,
 	if (monitor_enabled)
 		rv_disable_monitor(mon);
 
-	/* swap reactor's usage */
-	mon->reactor->counter--;
-	reactor->counter++;
-
 	mon->reactor = reactor;
 	mon->reacting = reacting;
 	mon->react = reactor->react;
@@ -343,23 +339,10 @@ int rv_register_reactor(struct rv_reactor *reactor)
  */
 int rv_unregister_reactor(struct rv_reactor *reactor)
 {
-	int ret = 0;
-
 	mutex_lock(&rv_interface_lock);
-
-	if (!reactor->counter) {
-		list_del(&reactor->list);
-	} else {
-		printk(KERN_WARNING
-		       "rv: the rv_reactor %s is in use by %d monitor(s)\n",
-		       reactor->name, reactor->counter);
-		printk(KERN_WARNING "rv: the rv_reactor %s cannot be removed\n",
-		       reactor->name);
-		ret = -EBUSY;
-	}
-
+	list_del(&reactor->list);
 	mutex_unlock(&rv_interface_lock);
-	return ret;
+	return 0;
 }
 
 /*
@@ -456,23 +439,11 @@ int reactor_populate_monitor(struct rv_monitor *mon)
 	 * Configure as the rv_nop reactor.
 	 */
 	mon->reactor = get_reactor_rdef_by_name("nop");
-	mon->reactor->counter++;
 	mon->reacting = false;
 
 	return 0;
 }
 
-/**
- * reactor_cleanup_monitor - cleanup a monitor reference
- * @mon:       the monitor.
- */
-void reactor_cleanup_monitor(struct rv_monitor *mon)
-{
-	lockdep_assert_held(&rv_interface_lock);
-	mon->reactor->counter--;
-	WARN_ON_ONCE(mon->reactor->counter < 0);
-}
-
 /*
  * Nop reactor register
  */
-- 
2.39.5

Re: [PATCH 4/6] rv: Remove rv_reactor's reference counter

[Gabriele Monaco]

On Mon, 2025-07-21 at 11:47 +0200, Nam Cao wrote:
> rv_reactor has a reference counter to ensure it is not removed while
> monitors are still using it.
> 
> However, this is futile, as __exit functions are not expected to fail
> and
> will proceed normally despite rv_unregister_reactor() returning an
> error.
> 
> At the moment, reactors do not support being built as modules,
> therefore
> they are never removed and the reference counters are not necessary.
> 
> If we support building RV reactors as modules in the future,
> MODULE_SOFTDEP should be used instead of a custom implementation.
> 

Mmh, I'm not understanding how, let's assume I create a custom reactor
as a kernel module and I want to use it on existing models (built in or
modules themselves), I'd do.

 insmod myreactor
 echo myreactor > mymodel/reactors
 rmmod myreactor
 ## I want this one to fail because the reactor is in use

 echo nop > mymodel/reactors
 rmmod myreactor
 # now it can succeed

How is MODULE_SOFTDEP helping in this scenario?
Am I missing something here?

Thanks,
Gabriele

> Remove this reference counter.
> 
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> ---
>  include/linux/rv.h            |  2 --
>  kernel/trace/rv/rv.c          |  1 -
>  kernel/trace/rv/rv.h          |  6 ------
>  kernel/trace/rv/rv_reactors.c | 33 ++-------------------------------
>  4 files changed, 2 insertions(+), 40 deletions(-)
> 
> diff --git a/include/linux/rv.h b/include/linux/rv.h
> index c22c9b8c1567..2f867d6f72ba 100644
> --- a/include/linux/rv.h
> +++ b/include/linux/rv.h
> @@ -91,8 +91,6 @@ struct rv_reactor {
>  	const char		*description;
>  	__printf(1, 2) void	(*react)(const char *msg, ...);
>  	struct list_head	list;
> -	/* protected by the monitor interface lock */
> -	int			counter;
>  };
>  #endif
>  
> diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
> index 6c0be2fdc52d..6c8498743b98 100644
> --- a/kernel/trace/rv/rv.c
> +++ b/kernel/trace/rv/rv.c
> @@ -769,7 +769,6 @@ static const struct file_operations
> monitoring_on_fops = {
>  
>  static void destroy_monitor_dir(struct rv_monitor *mon)
>  {
> -	reactor_cleanup_monitor(mon);
>  	rv_remove(mon->root_d);
>  }
>  
> diff --git a/kernel/trace/rv/rv.h b/kernel/trace/rv/rv.h
> index 8c38f9dd41bc..1485a70c1bf4 100644
> --- a/kernel/trace/rv/rv.h
> +++ b/kernel/trace/rv/rv.h
> @@ -31,7 +31,6 @@ bool rv_is_nested_monitor(struct rv_monitor *mon);
>  
>  #ifdef CONFIG_RV_REACTORS
>  int reactor_populate_monitor(struct rv_monitor *mon);
> -void reactor_cleanup_monitor(struct rv_monitor *mon);
>  int init_rv_reactors(struct dentry *root_dir);
>  #else
>  static inline int reactor_populate_monitor(struct rv_monitor *mon)
> @@ -39,11 +38,6 @@ static inline int reactor_populate_monitor(struct
> rv_monitor *mon)
>  	return 0;
>  }
>  
> -static inline void reactor_cleanup_monitor(struct rv_monitor *mon)
> -{
> -	return;
> -}
> -
>  static inline int init_rv_reactors(struct dentry *root_dir)
>  {
>  	return 0;
> diff --git a/kernel/trace/rv/rv_reactors.c
> b/kernel/trace/rv/rv_reactors.c
> index 2c7909e6d0e7..a8e849e6cd85 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -172,10 +172,6 @@ static void monitor_swap_reactors_single(struct
> rv_monitor *mon,
>  	if (monitor_enabled)
>  		rv_disable_monitor(mon);
>  
> -	/* swap reactor's usage */
> -	mon->reactor->counter--;
> -	reactor->counter++;
> -
>  	mon->reactor = reactor;
>  	mon->reacting = reacting;
>  	mon->react = reactor->react;
> @@ -343,23 +339,10 @@ int rv_register_reactor(struct rv_reactor
> *reactor)
>   */
>  int rv_unregister_reactor(struct rv_reactor *reactor)
>  {
> -	int ret = 0;
> -
>  	mutex_lock(&rv_interface_lock);
> -
> -	if (!reactor->counter) {
> -		list_del(&reactor->list);
> -	} else {
> -		printk(KERN_WARNING
> -		       "rv: the rv_reactor %s is in use by %d
> monitor(s)\n",
> -		       reactor->name, reactor->counter);
> -		printk(KERN_WARNING "rv: the rv_reactor %s cannot be
> removed\n",
> -		       reactor->name);
> -		ret = -EBUSY;
> -	}
> -
> +	list_del(&reactor->list);
>  	mutex_unlock(&rv_interface_lock);
> -	return ret;
> +	return 0;
>  }
>  
>  /*
> @@ -456,23 +439,11 @@ int reactor_populate_monitor(struct rv_monitor
> *mon)
>  	 * Configure as the rv_nop reactor.
>  	 */
>  	mon->reactor = get_reactor_rdef_by_name("nop");
> -	mon->reactor->counter++;
>  	mon->reacting = false;
>  
>  	return 0;
>  }
>  
> -/**
> - * reactor_cleanup_monitor - cleanup a monitor reference
> - * @mon:       the monitor.
> - */
> -void reactor_cleanup_monitor(struct rv_monitor *mon)
> -{
> -	lockdep_assert_held(&rv_interface_lock);
> -	mon->reactor->counter--;
> -	WARN_ON_ONCE(mon->reactor->counter < 0);
> -}
> -
>  /*
>   * Nop reactor register
>   */

Re: [PATCH 4/6] rv: Remove rv_reactor's reference counter

[Nam Cao]

On Mon, Jul 21, 2025 at 03:20:44PM +0200, Gabriele Monaco wrote:
> Mmh, I'm not understanding how, let's assume I create a custom reactor
> as a kernel module and I want to use it on existing models (built in or
> modules themselves), I'd do.
> 
>  insmod myreactor
>  echo myreactor > mymodel/reactors
>  rmmod myreactor
>  ## I want this one to fail because the reactor is in use
> 
>  echo nop > mymodel/reactors
>  rmmod myreactor
>  # now it can succeed
> 
> How is MODULE_SOFTDEP helping in this scenario?
> Am I missing something here?

You are right, MODULE_SOFTDEP does not help this use case.

I did a quick search, it seems try_module_get() and module_put() are what
we need for this. Let me amend the commit message.

But my essential point in this patch is that, the current ref count
implementation does not work. Furthermore, we should use the centralized
kernel module's facilities, not implementing our own custom logic.

Best regards,
Nam

Re: [PATCH 4/6] rv: Remove rv_reactor's reference counter

[Gabriele Monaco]

On Mon, 2025-07-21 at 16:04 +0200, Nam Cao wrote:
> On Mon, Jul 21, 2025 at 03:20:44PM +0200, Gabriele Monaco wrote:
> > Mmh, I'm not understanding how, let's assume I create a custom
> > reactor
> > as a kernel module and I want to use it on existing models (built
> > in or
> > modules themselves), I'd do.
> > 
> >  insmod myreactor
> >  echo myreactor > mymodel/reactors
> >  rmmod myreactor
> >  ## I want this one to fail because the reactor is in use
> > 
> >  echo nop > mymodel/reactors
> >  rmmod myreactor
> >  # now it can succeed
> > 
> > How is MODULE_SOFTDEP helping in this scenario?
> > Am I missing something here?
> 
> You are right, MODULE_SOFTDEP does not help this use case.
> 
> I did a quick search, it seems try_module_get() and module_put() are
> what we need for this. Let me amend the commit message.
> 

I wasn't aware of it, then sure we should be using that.

> But my essential point in this patch is that, the current ref count
> implementation does not work. Furthermore, we should use the
> centralized kernel module's facilities, not implementing our own
> custom logic.
> 

Yeah I get your point. If I understand you correctly, what's broken is
that we just return EBUSY and ignore that on __exit instead of doing
something about it (set nop to all monitors using this reactor).

I wonder if we shouldn't also fix this (using the module refcount).
But that can be done in the future, I'm not even sure reactors as
modules currently work.

Also, I'd need to verify this but depending on the order of exit
functions, we might be seeing the same problems with built-in reactors
when active on shutdown. I'm going to play a bit with this and see if
this workaround of not deleting the reactor was introduced for that (I
doubt though).

Thanks,
Gabriele

Re: [PATCH 4/6] rv: Remove rv_reactor's reference counter

[Nam Cao]

On Mon, Jul 21, 2025 at 05:49:02PM +0200, Gabriele Monaco wrote:
> Yeah I get your point. If I understand you correctly, what's broken is
> that we just return EBUSY and ignore that on __exit instead of doing
> something about it (set nop to all monitors using this reactor).

Yeah, EBUSY is ignored.

> I wonder if we shouldn't also fix this (using the module refcount).
> But that can be done in the future, I'm not even sure reactors as
> modules currently work.

Yes, let's worry about it when we allow building reactors as modules.

> Also, I'd need to verify this but depending on the order of exit
> functions, we might be seeing the same problems with built-in reactors
> when active on shutdown. I'm going to play a bit with this and see if
> this workaround of not deleting the reactor was introduced for that (I
> doubt though).

I'm not sure I follow. exit functions are never called for built-in
reactors. They are even discarded out of the built image.

Nam

Re: [PATCH 4/6] rv: Remove rv_reactor's reference counter

[Gabriele Monaco]

On Tue, 2025-07-22 at 10:27 +0200, Nam Cao wrote:
> On Mon, Jul 21, 2025 at 05:49:02PM +0200, Gabriele Monaco wrote:
> 
> > Also, I'd need to verify this but depending on the order of exit
> > functions, we might be seeing the same problems with built-in
> > reactors when active on shutdown. I'm going to play a bit with this
> > and see if this workaround of not deleting the reactor was
> > introduced for that (I doubt though).
> 
> I'm not sure I follow. exit functions are never called for built-in
> reactors. They are even discarded out of the built image.
> 

You're right, was tripping, ignore what I just said..

Reviewed-by: Gabriele Monaco <gmonaco@redhat.com>

Thanks,
Gabriele

[PATCH 5/6] rv: Remove the nop reactor

[Nam Cao]

As suggested by the name, the nop reactor does not do anything. It is the
default reactor when nothing else is selected.

However, the monitors already null-check the reactor function pointers.
Thus, instead of a nop reactor, just set the react function pointer to
NULL. The nop reactor can then be removed.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 kernel/trace/rv/rv_reactors.c | 63 ++++++-----------------------------
 1 file changed, 11 insertions(+), 52 deletions(-)

diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index a8e849e6cd85..aee622e4b833 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -70,17 +70,6 @@
  */
 static LIST_HEAD(rv_reactors_list);
 
-static struct rv_reactor *get_reactor_rdef_by_name(char *name)
-{
-	struct rv_reactor *r;
-
-	list_for_each_entry(r, &rv_reactors_list, list) {
-		if (strcmp(name, r->name) == 0)
-			return r;
-	}
-	return NULL;
-}
-
 /*
  * Available reactors seq functions.
  */
@@ -174,7 +163,7 @@ static void monitor_swap_reactors_single(struct rv_monitor *mon,
 
 	mon->reactor = reactor;
 	mon->reacting = reacting;
-	mon->react = reactor->react;
+	mon->react = reactor ? reactor->react : NULL;
 
 	/* enable only once if iterating through a container */
 	if (monitor_enabled && !nested)
@@ -210,10 +199,15 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
 	struct rv_reactor *reactor;
 	struct seq_file *seq_f;
 	int retval = -EINVAL;
-	bool enable;
 	char *ptr;
 	int len;
 
+	/*
+	 * See monitor_reactors_open()
+	 */
+	seq_f = file->private_data;
+	mon = seq_f->private;
+
 	if (count < 1 || count > MAX_RV_REACTOR_NAME_SIZE + 1)
 		return -EINVAL;
 
@@ -226,14 +220,10 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
 	ptr = strim(buff);
 
 	len = strlen(ptr);
-	if (!len)
+	if (!len) {
+		monitor_swap_reactors(mon, NULL, false);
 		return count;
-
-	/*
-	 * See monitor_reactors_open()
-	 */
-	seq_f = file->private_data;
-	mon = seq_f->private;
+	}
 
 	mutex_lock(&rv_interface_lock);
 
@@ -243,12 +233,7 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
 		if (strcmp(ptr, reactor->name) != 0)
 			continue;
 
-		if (strcmp(reactor->name, "nop"))
-			enable = false;
-		else
-			enable = true;
-
-		monitor_swap_reactors(mon, reactor, enable);
+		monitor_swap_reactors(mon, reactor, true);
 
 		retval = count;
 		break;
@@ -435,32 +420,12 @@ int reactor_populate_monitor(struct rv_monitor *mon)
 	if (!tmp)
 		return -ENOMEM;
 
-	/*
-	 * Configure as the rv_nop reactor.
-	 */
-	mon->reactor = get_reactor_rdef_by_name("nop");
-	mon->reacting = false;
-
 	return 0;
 }
 
-/*
- * Nop reactor register
- */
-__printf(1, 2) static void rv_nop_reaction(const char *msg, ...)
-{
-}
-
-static struct rv_reactor rv_nop = {
-	.name = "nop",
-	.description = "no-operation reactor: do nothing.",
-	.react = rv_nop_reaction
-};
-
 int init_rv_reactors(struct dentry *root_dir)
 {
 	struct dentry *available, *reacting;
-	int retval;
 
 	available = rv_create_file("available_reactors", RV_MODE_READ, root_dir, NULL,
 				   &available_reactors_ops);
@@ -471,16 +436,10 @@ int init_rv_reactors(struct dentry *root_dir)
 	if (!reacting)
 		goto rm_available;
 
-	retval = __rv_register_reactor(&rv_nop);
-	if (retval)
-		goto rm_reacting;
-
 	turn_reacting_on();
 
 	return 0;
 
-rm_reacting:
-	rv_remove(reacting);
 rm_available:
 	rv_remove(available);
 out_err:
-- 
2.39.5

Re: [PATCH 5/6] rv: Remove the nop reactor

[Gabriele Monaco]

On Mon, 2025-07-21 at 11:47 +0200, Nam Cao wrote:
> As suggested by the name, the nop reactor does not do anything. It is
> the
> default reactor when nothing else is selected.
> 
> However, the monitors already null-check the reactor function
> pointers.
> Thus, instead of a nop reactor, just set the react function pointer
> to
> NULL. The nop reactor can then be removed.
> 
> Signed-off-by: Nam Cao <namcao@linutronix.de>

Thanks for the patch, I'd need to go through this a bit more in detail.

As far as I remember, the only way to disable reaction is to set it to
the nop reactor.
With your patch the behaviour changes and, to disable the reactor, you
now need to write an empty string, this should be documented somewhere,
at the very least. Perhaps userspace tools (tools/verification/rv)
might break and would need adaptation.

We could still remove the kernel side implementation, but from
userspace (tracefs) we might want to keep the nop reactor available,
setting it would set the reactor to NULL under the hood.

If you really want to change also the user space interface, we might
want to imitate other tracefs features and use something like printk /
!printk to enable/disable a reactor.

What do you think? Did I miss anything here?

Thanks,
Gabriele

> ---
>  kernel/trace/rv/rv_reactors.c | 63 ++++++---------------------------
> --
>  1 file changed, 11 insertions(+), 52 deletions(-)
> 
> diff --git a/kernel/trace/rv/rv_reactors.c
> b/kernel/trace/rv/rv_reactors.c
> index a8e849e6cd85..aee622e4b833 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -70,17 +70,6 @@
>   */
>  static LIST_HEAD(rv_reactors_list);
>  
> -static struct rv_reactor *get_reactor_rdef_by_name(char *name)
> -{
> -	struct rv_reactor *r;
> -
> -	list_for_each_entry(r, &rv_reactors_list, list) {
> -		if (strcmp(name, r->name) == 0)
> -			return r;
> -	}
> -	return NULL;
> -}
> -
>  /*
>   * Available reactors seq functions.
>   */
> @@ -174,7 +163,7 @@ static void monitor_swap_reactors_single(struct
> rv_monitor *mon,
>  
>  	mon->reactor = reactor;
>  	mon->reacting = reacting;
> -	mon->react = reactor->react;
> +	mon->react = reactor ? reactor->react : NULL;
>  
>  	/* enable only once if iterating through a container */
>  	if (monitor_enabled && !nested)
> @@ -210,10 +199,15 @@ monitor_reactors_write(struct file *file, const
> char __user *user_buf,
>  	struct rv_reactor *reactor;
>  	struct seq_file *seq_f;
>  	int retval = -EINVAL;
> -	bool enable;
>  	char *ptr;
>  	int len;
>  
> +	/*
> +	 * See monitor_reactors_open()
> +	 */
> +	seq_f = file->private_data;
> +	mon = seq_f->private;
> +
>  	if (count < 1 || count > MAX_RV_REACTOR_NAME_SIZE + 1)
>  		return -EINVAL;
>  
> @@ -226,14 +220,10 @@ monitor_reactors_write(struct file *file, const
> char __user *user_buf,
>  	ptr = strim(buff);
>  
>  	len = strlen(ptr);
> -	if (!len)
> +	if (!len) {
> +		monitor_swap_reactors(mon, NULL, false);
>  		return count;
> -
> -	/*
> -	 * See monitor_reactors_open()
> -	 */
> -	seq_f = file->private_data;
> -	mon = seq_f->private;
> +	}
>  
>  	mutex_lock(&rv_interface_lock);
>  
> @@ -243,12 +233,7 @@ monitor_reactors_write(struct file *file, const
> char __user *user_buf,
>  		if (strcmp(ptr, reactor->name) != 0)
>  			continue;
>  
> -		if (strcmp(reactor->name, "nop"))
> -			enable = false;
> -		else
> -			enable = true;
> -
> -		monitor_swap_reactors(mon, reactor, enable);
> +		monitor_swap_reactors(mon, reactor, true);
>  
>  		retval = count;
>  		break;
> @@ -435,32 +420,12 @@ int reactor_populate_monitor(struct rv_monitor
> *mon)
>  	if (!tmp)
>  		return -ENOMEM;
>  
> -	/*
> -	 * Configure as the rv_nop reactor.
> -	 */
> -	mon->reactor = get_reactor_rdef_by_name("nop");
> -	mon->reacting = false;
> -
>  	return 0;
>  }
>  
> -/*
> - * Nop reactor register
> - */
> -__printf(1, 2) static void rv_nop_reaction(const char *msg, ...)
> -{
> -}
> -
> -static struct rv_reactor rv_nop = {
> -	.name = "nop",
> -	.description = "no-operation reactor: do nothing.",
> -	.react = rv_nop_reaction
> -};
> -
>  int init_rv_reactors(struct dentry *root_dir)
>  {
>  	struct dentry *available, *reacting;
> -	int retval;
>  
>  	available = rv_create_file("available_reactors",
> RV_MODE_READ, root_dir, NULL,
>  				   &available_reactors_ops);
> @@ -471,16 +436,10 @@ int init_rv_reactors(struct dentry *root_dir)
>  	if (!reacting)
>  		goto rm_available;
>  
> -	retval = __rv_register_reactor(&rv_nop);
> -	if (retval)
> -		goto rm_reacting;
> -
>  	turn_reacting_on();
>  
>  	return 0;
>  
> -rm_reacting:
> -	rv_remove(reacting);
>  rm_available:
>  	rv_remove(available);
>  out_err:

Re: [PATCH 5/6] rv: Remove the nop reactor

[Nam Cao]

On Mon, Jul 21, 2025 at 12:29:24PM +0200, Gabriele Monaco wrote:
> On Mon, 2025-07-21 at 11:47 +0200, Nam Cao wrote:
> > As suggested by the name, the nop reactor does not do anything. It is
> > the
> > default reactor when nothing else is selected.
> > 
> > However, the monitors already null-check the reactor function
> > pointers.
> > Thus, instead of a nop reactor, just set the react function pointer
> > to
> > NULL. The nop reactor can then be removed.
> > 
> > Signed-off-by: Nam Cao <namcao@linutronix.de>
> 
> Thanks for the patch, I'd need to go through this a bit more in detail.
> 
> As far as I remember, the only way to disable reaction is to set it to
> the nop reactor.
> With your patch the behaviour changes and, to disable the reactor, you
> now need to write an empty string, this should be documented somewhere,
> at the very least. Perhaps userspace tools (tools/verification/rv)
> might break and would need adaptation.
> 
> We could still remove the kernel side implementation, but from
> userspace (tracefs) we might want to keep the nop reactor available,
> setting it would set the reactor to NULL under the hood.
> 
> If you really want to change also the user space interface, we might
> want to imitate other tracefs features and use something like printk /
> !printk to enable/disable a reactor.
> 
> What do you think? Did I miss anything here?

You didn't miss anything, I miss that this "nop" interface is inspired by
tracefs.

Although I prefer writing empty string, compared to writing "nop"; it is
better to be consistent with tracefs.

Let me drop this patch.

Best regards,
Nam

[PATCH 6/6] rv: Remove struct rv_monitor::reacting

[Nam Cao]

The field 'reacting' in struct rv_monitor is set but never used. Delete it.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 include/linux/rv.h            |  1 -
 kernel/trace/rv/rv_reactors.c | 14 ++++++--------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/include/linux/rv.h b/include/linux/rv.h
index 2f867d6f72ba..80731242fe60 100644
--- a/include/linux/rv.h
+++ b/include/linux/rv.h
@@ -104,7 +104,6 @@ struct rv_monitor {
 #ifdef CONFIG_RV_REACTORS
 	struct rv_reactor	*reactor;
 	__printf(1, 2) void	(*react)(const char *msg, ...);
-	bool			reacting;
 #endif
 	struct list_head	list;
 	struct rv_monitor	*parent;
diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index aee622e4b833..6b03f3f6ecc1 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -149,7 +149,7 @@ static const struct seq_operations monitor_reactors_seq_ops = {
 
 static void monitor_swap_reactors_single(struct rv_monitor *mon,
 					 struct rv_reactor *reactor,
-					 bool reacting, bool nested)
+					 bool nested)
 {
 	bool monitor_enabled;
 
@@ -162,7 +162,6 @@ static void monitor_swap_reactors_single(struct rv_monitor *mon,
 		rv_disable_monitor(mon);
 
 	mon->reactor = reactor;
-	mon->reacting = reacting;
 	mon->react = reactor ? reactor->react : NULL;
 
 	/* enable only once if iterating through a container */
@@ -170,8 +169,7 @@ static void monitor_swap_reactors_single(struct rv_monitor *mon,
 		rv_enable_monitor(mon);
 }
 
-static void monitor_swap_reactors(struct rv_monitor *mon,
-				  struct rv_reactor *reactor, bool reacting)
+static void monitor_swap_reactors(struct rv_monitor *mon, struct rv_reactor *reactor)
 {
 	struct rv_monitor *p = mon;
 
@@ -179,7 +177,7 @@ static void monitor_swap_reactors(struct rv_monitor *mon,
 		list_for_each_entry_continue(p, &rv_monitors_list, list) {
 			if (p->parent != mon)
 				break;
-			monitor_swap_reactors_single(p, reactor, reacting, true);
+			monitor_swap_reactors_single(p, reactor, true);
 		}
 	/*
 	 * This call enables and disables the monitor if they were active.
@@ -187,7 +185,7 @@ static void monitor_swap_reactors(struct rv_monitor *mon,
 	 * All nested monitors are enabled also if they were off, we may refine
 	 * this logic in the future.
 	 */
-	monitor_swap_reactors_single(mon, reactor, reacting, false);
+	monitor_swap_reactors_single(mon, reactor, false);
 }
 
 static ssize_t
@@ -221,7 +219,7 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
 
 	len = strlen(ptr);
 	if (!len) {
-		monitor_swap_reactors(mon, NULL, false);
+		monitor_swap_reactors(mon, NULL);
 		return count;
 	}
 
@@ -233,7 +231,7 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
 		if (strcmp(ptr, reactor->name) != 0)
 			continue;
 
-		monitor_swap_reactors(mon, reactor, true);
+		monitor_swap_reactors(mon, reactor);
 
 		retval = count;
 		break;
-- 
2.39.5

Re: [PATCH 6/6] rv: Remove struct rv_monitor::reacting

[Gabriele Monaco]

On Mon, 2025-07-21 at 11:47 +0200, Nam Cao wrote:
> The field 'reacting' in struct rv_monitor is set but never used.
> Delete it.
> 
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> ---

Yeah seems to be the case, thanks for spotting it.

Reviewed-by: Gabriele Monaco <gmonaco@redhat.com>

>  include/linux/rv.h            |  1 -
>  kernel/trace/rv/rv_reactors.c | 14 ++++++--------
>  2 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/rv.h b/include/linux/rv.h
> index 2f867d6f72ba..80731242fe60 100644
> --- a/include/linux/rv.h
> +++ b/include/linux/rv.h
> @@ -104,7 +104,6 @@ struct rv_monitor {
>  #ifdef CONFIG_RV_REACTORS
>  	struct rv_reactor	*reactor;
>  	__printf(1, 2) void	(*react)(const char *msg, ...);
> -	bool			reacting;
>  #endif
>  	struct list_head	list;
>  	struct rv_monitor	*parent;
> diff --git a/kernel/trace/rv/rv_reactors.c
> b/kernel/trace/rv/rv_reactors.c
> index aee622e4b833..6b03f3f6ecc1 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -149,7 +149,7 @@ static const struct seq_operations
> monitor_reactors_seq_ops = {
>  
>  static void monitor_swap_reactors_single(struct rv_monitor *mon,
>  					 struct rv_reactor *reactor,
> -					 bool reacting, bool nested)
> +					 bool nested)
>  {
>  	bool monitor_enabled;
>  
> @@ -162,7 +162,6 @@ static void monitor_swap_reactors_single(struct
> rv_monitor *mon,
>  		rv_disable_monitor(mon);
>  
>  	mon->reactor = reactor;
> -	mon->reacting = reacting;
>  	mon->react = reactor ? reactor->react : NULL;
>  
>  	/* enable only once if iterating through a container */
> @@ -170,8 +169,7 @@ static void monitor_swap_reactors_single(struct
> rv_monitor *mon,
>  		rv_enable_monitor(mon);
>  }
>  
> -static void monitor_swap_reactors(struct rv_monitor *mon,
> -				  struct rv_reactor *reactor, bool
> reacting)
> +static void monitor_swap_reactors(struct rv_monitor *mon, struct
> rv_reactor *reactor)
>  {
>  	struct rv_monitor *p = mon;
>  
> @@ -179,7 +177,7 @@ static void monitor_swap_reactors(struct
> rv_monitor *mon,
>  		list_for_each_entry_continue(p, &rv_monitors_list,
> list) {
>  			if (p->parent != mon)
>  				break;
> -			monitor_swap_reactors_single(p, reactor,
> reacting, true);
> +			monitor_swap_reactors_single(p, reactor,
> true);
>  		}
>  	/*
>  	 * This call enables and disables the monitor if they were
> active.
> @@ -187,7 +185,7 @@ static void monitor_swap_reactors(struct
> rv_monitor *mon,
>  	 * All nested monitors are enabled also if they were off, we
> may refine
>  	 * this logic in the future.
>  	 */
> -	monitor_swap_reactors_single(mon, reactor, reacting, false);
> +	monitor_swap_reactors_single(mon, reactor, false);
>  }
>  
>  static ssize_t
> @@ -221,7 +219,7 @@ monitor_reactors_write(struct file *file, const
> char __user *user_buf,
>  
>  	len = strlen(ptr);
>  	if (!len) {
> -		monitor_swap_reactors(mon, NULL, false);
> +		monitor_swap_reactors(mon, NULL);
>  		return count;
>  	}
>  
> @@ -233,7 +231,7 @@ monitor_reactors_write(struct file *file, const
> char __user *user_buf,
>  		if (strcmp(ptr, reactor->name) != 0)
>  			continue;
>  
> -		monitor_swap_reactors(mon, reactor, true);
> +		monitor_swap_reactors(mon, reactor);
>  
>  		retval = count;
>  		break;