| Message ID | 20241028071308.406073025@goodmis.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8b0cb3a4c5e85dda8957ba6a4c8c081a9aec6e80 |
| Headers | show |
| Series | ftrace: Use guard to take ftrace_lock | expand |
On Mon, Oct 28, 2024 at 03:12:33AM -0400, Steven Rostedt wrote: > From: Steven Rostedt <rostedt@goodmis.org> > > The ftrace_lock is taken for most of the ftrace_graph_set_hash() function > throughout the end. Use guard to take the ftrace_lock to simplify the exit > paths. > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > kernel/trace/ftrace.c | 15 ++++----------- > 1 file changed, 4 insertions(+), 11 deletions(-) > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index c0fabd7da5b2..b4ef469f4fd2 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -6816,12 +6816,10 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer) > > func_g.len = strlen(func_g.search); > > - mutex_lock(&ftrace_lock); > + guard(mutex)(&ftrace_lock); > > - if (unlikely(ftrace_disabled)) { > - mutex_unlock(&ftrace_lock); > + if (unlikely(ftrace_disabled)) > return -ENODEV; > - } > > do_for_each_ftrace_rec(pg, rec) { > > @@ -6837,7 +6835,7 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer) > if (entry) > continue; > if (add_hash_entry(hash, rec->ip) == NULL) > - goto out; > + return 0; > } else { > if (entry) { > free_hash_entry(hash, entry); > @@ -6846,13 +6844,8 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer) > } > } > } while_for_each_ftrace_rec(); > -out: > - mutex_unlock(&ftrace_lock); > > - if (fail) > - return -EINVAL; > - > - return 0; > + return fail ? -EINVAL : 0; > } Isn't the fail case more a case of -ESRCH / -ENOENT rather than -EINVAL? Anyway, that's orthogonal, the patch preserves existing semantics and looks okay (as do the others fwiw).
On Mon, 28 Oct 2024 10:16:56 +0100 Peter Zijlstra <peterz@infradead.org> wrote: > > @@ -6846,13 +6844,8 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer) > > } > > } > > } while_for_each_ftrace_rec(); > > -out: > > - mutex_unlock(&ftrace_lock); > > > > - if (fail) > > - return -EINVAL; > > - > > - return 0; > > + return fail ? -EINVAL : 0; > > } > > Isn't the fail case more a case of -ESRCH / -ENOENT rather than -EINVAL? Could be. Although this is mostly for internal use. I should check to see if this gets back to user space. And yeah, it probably should be changed. > > Anyway, that's orthogonal, the patch preserves existing semantics and > looks okay (as do the others fwiw). Thanks for the review! -- Steve
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index c0fabd7da5b2..b4ef469f4fd2 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -6816,12 +6816,10 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer) func_g.len = strlen(func_g.search); - mutex_lock(&ftrace_lock); + guard(mutex)(&ftrace_lock); - if (unlikely(ftrace_disabled)) { - mutex_unlock(&ftrace_lock); + if (unlikely(ftrace_disabled)) return -ENODEV; - } do_for_each_ftrace_rec(pg, rec) { @@ -6837,7 +6835,7 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer) if (entry) continue; if (add_hash_entry(hash, rec->ip) == NULL) - goto out; + return 0; } else { if (entry) { free_hash_entry(hash, entry); @@ -6846,13 +6844,8 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer) } } } while_for_each_ftrace_rec(); -out: - mutex_unlock(&ftrace_lock); - if (fail) - return -EINVAL; - - return 0; + return fail ? -EINVAL : 0; } static ssize_t