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@xxxxxxxxxxxxx>
> ---

Also here, looks good to me, thanks!

Reviewed-by: Gabriele Monaco <gmonaco@xxxxxxxxxx>

>  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);
>  }
>  
>  /*