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

[Gabriele Monaco <gmonaco@redhat.com>]

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
>   */