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