| Message ID | 173289891992.73724.9491350426847416169.stgit@devnote2 (mailing list archive) |
|---|---|
| State | Queued |
| Commit | 7dbc10961c8af4d7cc7d47f259f14c95d870ac98 |
| Headers | show |
| Series | kprobes: tracing/probes: Fix and cleanup to use guard | expand |
On Sat, 30 Nov 2024 01:48:40 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > Use guard() or scoped_guard() in dynamic events for critical sections > rather than discrete lock/unlock pairs. > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > --- > Changes in v2: > - Use scoped_guard() instead of guard() to avoid goto warnings. I forgot you touched this file, and added a free guard to it which conflicts. That said, I have some issues with this patch. > --- > kernel/trace/trace_dynevent.c | 35 ++++++++++++++++------------------- > 1 file changed, 16 insertions(+), 19 deletions(-) > > diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c > index 4376887e0d8a..eb8f669c15e1 100644 > --- a/kernel/trace/trace_dynevent.c > +++ b/kernel/trace/trace_dynevent.c > @@ -63,9 +63,8 @@ int dyn_event_register(struct dyn_event_operations *ops) > return -EINVAL; > > INIT_LIST_HEAD(&ops->list); > - mutex_lock(&dyn_event_ops_mutex); > + guard(mutex)(&dyn_event_ops_mutex); > list_add_tail(&ops->list, &dyn_event_ops_list); > - mutex_unlock(&dyn_event_ops_mutex); I don't care for a scoped guards around simple paths. The great thing about guard()s is that they help prevent bugs when you have complex code between the lock and unlock that may need to exit. But replacing: mutex_lock(&dyn_event_ops_mutex); list_add_tail(&ops->list, &dyn_event_ops_list); mutex_unlock(&dyn_event_ops_mutex); } With: guard(mutex)(&dyn_event_ops_mutex); list_add_tail(&ops->list, &dyn_event_ops_list); } is overkill to me. The first one is much easier to read. The second one begs the question, "why did they use a guard here?" > return 0; > } > > @@ -106,20 +105,20 @@ int dyn_event_release(const char *raw_command, > struct dyn_event_operations *type goto out; > } > > - mutex_lock(&event_mutex); > - for_each_dyn_event_safe(pos, n) { > - if (type && type != pos->ops) > - continue; > - if (!pos->ops->match(system, event, > - argc - 1, (const char **)argv + 1, pos)) > - continue; > - > - ret = pos->ops->free(pos); > - if (ret) > - break; > + scoped_guard(mutex, &event_mutex) { > + for_each_dyn_event_safe(pos, n) { > + if (type && type != pos->ops) > + continue; > + if (!pos->ops->match(system, event, > + argc - 1, (const char **)argv + > 1, pos)) > + continue; > + > + ret = pos->ops->free(pos); > + if (ret) > + break; > + } > + tracing_reset_all_online_cpus(); > } This scoped_guard() doesn't give us anything. We still have the out label below (which my patch removes). > - tracing_reset_all_online_cpus(); > - mutex_unlock(&event_mutex); > out: > argv_free(argv); > return ret; > @@ -133,13 +132,12 @@ static int create_dyn_event(const char *raw_command) > if (raw_command[0] == '-' || raw_command[0] == '!') > return dyn_event_release(raw_command, NULL); > > - mutex_lock(&dyn_event_ops_mutex); > + guard(mutex)(&dyn_event_ops_mutex); > list_for_each_entry(ops, &dyn_event_ops_list, list) { > ret = ops->create(raw_command); > if (!ret || ret != -ECANCELED) > break; > } I also don't think this helps much here. > - mutex_unlock(&dyn_event_ops_mutex); > if (ret == -ECANCELED) > ret = -EINVAL; > > @@ -198,7 +196,7 @@ int dyn_events_release_all(struct dyn_event_operations *type) > struct dyn_event *ev, *tmp; > int ret = 0; > > - mutex_lock(&event_mutex); > + guard(mutex)(&event_mutex); > for_each_dyn_event(ev) { > if (type && ev->ops != type) > continue; > @@ -216,7 +214,6 @@ int dyn_events_release_all(struct dyn_event_operations *type) > } > out: And the same issue here too. Why the guard when you still need to do the goto? > tracing_reset_all_online_cpus(); > - mutex_unlock(&event_mutex); > > return ret; > } There's a reason I looked at this file an didn't add any guards to it (when I forgot that you touched it). They are all special cases, and I rather avoid adding special case guards to handle it. I don't believe it makes it any less error prone. Are you OK with dropping this patch? -- Steve
On Fri, 27 Dec 2024 10:12:18 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> Are you OK with dropping this patch?
Masami, I can't make a new for-next branch with this conflict.
-- Steve
On Fri, 27 Dec 2024 10:12:18 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Sat, 30 Nov 2024 01:48:40 +0900 > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > Use guard() or scoped_guard() in dynamic events for critical sections > > rather than discrete lock/unlock pairs. > > > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > --- > > Changes in v2: > > - Use scoped_guard() instead of guard() to avoid goto warnings. > > I forgot you touched this file, and added a free guard to it which > conflicts. That said, I have some issues with this patch. > > > --- > > kernel/trace/trace_dynevent.c | 35 ++++++++++++++++------------------- > > 1 file changed, 16 insertions(+), 19 deletions(-) > > > > diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c > > index 4376887e0d8a..eb8f669c15e1 100644 > > --- a/kernel/trace/trace_dynevent.c > > +++ b/kernel/trace/trace_dynevent.c > > @@ -63,9 +63,8 @@ int dyn_event_register(struct dyn_event_operations *ops) > > return -EINVAL; > > > > INIT_LIST_HEAD(&ops->list); > > - mutex_lock(&dyn_event_ops_mutex); > > + guard(mutex)(&dyn_event_ops_mutex); > > list_add_tail(&ops->list, &dyn_event_ops_list); > > - mutex_unlock(&dyn_event_ops_mutex); > > I don't care for a scoped guards around simple paths. The great thing about > guard()s is that they help prevent bugs when you have complex code between > the lock and unlock that may need to exit. > > But replacing: > > > mutex_lock(&dyn_event_ops_mutex); > list_add_tail(&ops->list, &dyn_event_ops_list); > mutex_unlock(&dyn_event_ops_mutex); > } > > > With: > > guard(mutex)(&dyn_event_ops_mutex); > list_add_tail(&ops->list, &dyn_event_ops_list); > } > > is overkill to me. The first one is much easier to read. The second one > begs the question, "why did they use a guard here?" OK. fair enough. I think I was getting a little too excited. :( > > > return 0; > > } > > > > @@ -106,20 +105,20 @@ int dyn_event_release(const char *raw_command, > > struct dyn_event_operations *type goto out; > > } > > > > - mutex_lock(&event_mutex); > > - for_each_dyn_event_safe(pos, n) { > > - if (type && type != pos->ops) > > - continue; > > - if (!pos->ops->match(system, event, > > - argc - 1, (const char **)argv + 1, pos)) > > - continue; > > - > > - ret = pos->ops->free(pos); > > - if (ret) > > - break; > > + scoped_guard(mutex, &event_mutex) { > > + for_each_dyn_event_safe(pos, n) { > > + if (type && type != pos->ops) > > + continue; > > + if (!pos->ops->match(system, event, > > + argc - 1, (const char **)argv + > > 1, pos)) > > + continue; > > + > > + ret = pos->ops->free(pos); > > + if (ret) > > + break; > > + } > > + tracing_reset_all_online_cpus(); > > } > > This scoped_guard() doesn't give us anything. We still have the out label > below (which my patch removes). OK. > > - tracing_reset_all_online_cpus(); > > - mutex_unlock(&event_mutex); > > out: > > argv_free(argv); > > return ret; > > @@ -133,13 +132,12 @@ static int create_dyn_event(const char *raw_command) > > if (raw_command[0] == '-' || raw_command[0] == '!') > > return dyn_event_release(raw_command, NULL); > > > > - mutex_lock(&dyn_event_ops_mutex); > > + guard(mutex)(&dyn_event_ops_mutex); > > list_for_each_entry(ops, &dyn_event_ops_list, list) { > > ret = ops->create(raw_command); > > if (!ret || ret != -ECANCELED) > > break; > > } > > I also don't think this helps much here. OK. > > - mutex_unlock(&dyn_event_ops_mutex); > > if (ret == -ECANCELED) > > ret = -EINVAL; > > > > @@ -198,7 +196,7 @@ int dyn_events_release_all(struct dyn_event_operations *type) > > struct dyn_event *ev, *tmp; > > int ret = 0; > > > > - mutex_lock(&event_mutex); > > + guard(mutex)(&event_mutex); > > for_each_dyn_event(ev) { > > if (type && ev->ops != type) > > continue; > > @@ -216,7 +214,6 @@ int dyn_events_release_all(struct dyn_event_operations *type) > > } > > out: > > And the same issue here too. Why the guard when you still need to do the > goto? Yeah, we still need to call tracing_reset_all_online_cpus() here. > > > > tracing_reset_all_online_cpus(); > > - mutex_unlock(&event_mutex); > > > > return ret; > > } > > > There's a reason I looked at this file an didn't add any guards to it (when > I forgot that you touched it). They are all special cases, and I rather > avoid adding special case guards to handle it. I don't believe it makes it > any less error prone. > > Are you OK with dropping this patch? Yeah, let's drop it. Thanks for your review! > > -- Steve
diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c index 4376887e0d8a..eb8f669c15e1 100644 --- a/kernel/trace/trace_dynevent.c +++ b/kernel/trace/trace_dynevent.c @@ -63,9 +63,8 @@ int dyn_event_register(struct dyn_event_operations *ops) return -EINVAL; INIT_LIST_HEAD(&ops->list); - mutex_lock(&dyn_event_ops_mutex); + guard(mutex)(&dyn_event_ops_mutex); list_add_tail(&ops->list, &dyn_event_ops_list); - mutex_unlock(&dyn_event_ops_mutex); return 0; } @@ -106,20 +105,20 @@ int dyn_event_release(const char *raw_command, struct dyn_event_operations *type goto out; } - mutex_lock(&event_mutex); - for_each_dyn_event_safe(pos, n) { - if (type && type != pos->ops) - continue; - if (!pos->ops->match(system, event, - argc - 1, (const char **)argv + 1, pos)) - continue; - - ret = pos->ops->free(pos); - if (ret) - break; + scoped_guard(mutex, &event_mutex) { + for_each_dyn_event_safe(pos, n) { + if (type && type != pos->ops) + continue; + if (!pos->ops->match(system, event, + argc - 1, (const char **)argv + 1, pos)) + continue; + + ret = pos->ops->free(pos); + if (ret) + break; + } + tracing_reset_all_online_cpus(); } - tracing_reset_all_online_cpus(); - mutex_unlock(&event_mutex); out: argv_free(argv); return ret; @@ -133,13 +132,12 @@ static int create_dyn_event(const char *raw_command) if (raw_command[0] == '-' || raw_command[0] == '!') return dyn_event_release(raw_command, NULL); - mutex_lock(&dyn_event_ops_mutex); + guard(mutex)(&dyn_event_ops_mutex); list_for_each_entry(ops, &dyn_event_ops_list, list) { ret = ops->create(raw_command); if (!ret || ret != -ECANCELED) break; } - mutex_unlock(&dyn_event_ops_mutex); if (ret == -ECANCELED) ret = -EINVAL; @@ -198,7 +196,7 @@ int dyn_events_release_all(struct dyn_event_operations *type) struct dyn_event *ev, *tmp; int ret = 0; - mutex_lock(&event_mutex); + guard(mutex)(&event_mutex); for_each_dyn_event(ev) { if (type && ev->ops != type) continue; @@ -216,7 +214,6 @@ int dyn_events_release_all(struct dyn_event_operations *type) } out: tracing_reset_all_online_cpus(); - mutex_unlock(&event_mutex); return ret; }