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