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.5On 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
> */
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
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
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
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
© 2016 - 2025 Red Hat, Inc.