* [PATCH 0/2] tracing: Clean up of set_tracer_flag()
@ 2025-11-06 0:33 Steven Rostedt
2025-11-06 0:33 ` [PATCH 1/2] tracing: Exit out immediately after update_marker_trace() Steven Rostedt
2025-11-06 0:33 ` [PATCH 2/2] tracing: Use switch statement instead of ifs in set_tracer_flag() Steven Rostedt
0 siblings, 2 replies; 6+ messages in thread
From: Steven Rostedt @ 2025-11-06 0:33 UTC (permalink / raw)
To: linux-kernel, linux-trace-kernel
Cc: Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton
The set_tracer_flag() function takes a mask that has a single bit set.
It then goes through a bunch of "if ()" statements looking at what the
mask is to perform the necessary operations. It is simpler to switch that
over to a "switch ()" statement.
Steven Rostedt (2):
tracing: Exit out immediately after update_marker_trace()
tracing: Use switch statement instead of ifs in set_tracer_flag()
----
kernel/trace/trace.c | 41 ++++++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 15 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] tracing: Exit out immediately after update_marker_trace() 2025-11-06 0:33 [PATCH 0/2] tracing: Clean up of set_tracer_flag() Steven Rostedt @ 2025-11-06 0:33 ` Steven Rostedt 2025-11-10 5:24 ` Masami Hiramatsu 2025-11-06 0:33 ` [PATCH 2/2] tracing: Use switch statement instead of ifs in set_tracer_flag() Steven Rostedt 1 sibling, 1 reply; 6+ messages in thread From: Steven Rostedt @ 2025-11-06 0:33 UTC (permalink / raw) To: linux-kernel, linux-trace-kernel Cc: Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton From: Steven Rostedt <rostedt@goodmis.org> The call to update_marker_trace() in set_tracer_flag() performs the update to the tr->trace_flags. There's no reason to perform it again after it is called. Return immediately instead. Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> --- kernel/trace/trace.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index dea1566b3301..07bd10808277 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -5285,8 +5285,11 @@ int set_tracer_flag(struct trace_array *tr, u64 mask, int enabled) } } - if (mask == TRACE_ITER(COPY_MARKER)) + if (mask == TRACE_ITER(COPY_MARKER)) { update_marker_trace(tr, enabled); + /* update_marker_trace updates the tr->trace_flags */ + return 0; + } if (enabled) tr->trace_flags |= mask; -- 2.51.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] tracing: Exit out immediately after update_marker_trace() 2025-11-06 0:33 ` [PATCH 1/2] tracing: Exit out immediately after update_marker_trace() Steven Rostedt @ 2025-11-10 5:24 ` Masami Hiramatsu 0 siblings, 0 replies; 6+ messages in thread From: Masami Hiramatsu @ 2025-11-10 5:24 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, linux-trace-kernel, Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton On Wed, 05 Nov 2025 19:33:25 -0500 Steven Rostedt <rostedt@kernel.org> wrote: > From: Steven Rostedt <rostedt@goodmis.org> > > The call to update_marker_trace() in set_tracer_flag() performs the update > to the tr->trace_flags. There's no reason to perform it again after it is > called. Return immediately instead. > Looks good to me. Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thanks, > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > kernel/trace/trace.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index dea1566b3301..07bd10808277 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -5285,8 +5285,11 @@ int set_tracer_flag(struct trace_array *tr, u64 mask, int enabled) > } > } > > - if (mask == TRACE_ITER(COPY_MARKER)) > + if (mask == TRACE_ITER(COPY_MARKER)) { > update_marker_trace(tr, enabled); > + /* update_marker_trace updates the tr->trace_flags */ > + return 0; > + } > > if (enabled) > tr->trace_flags |= mask; > -- > 2.51.0 > > -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] tracing: Use switch statement instead of ifs in set_tracer_flag() 2025-11-06 0:33 [PATCH 0/2] tracing: Clean up of set_tracer_flag() Steven Rostedt 2025-11-06 0:33 ` [PATCH 1/2] tracing: Exit out immediately after update_marker_trace() Steven Rostedt @ 2025-11-06 0:33 ` Steven Rostedt 2025-11-10 5:48 ` Masami Hiramatsu 1 sibling, 1 reply; 6+ messages in thread From: Steven Rostedt @ 2025-11-06 0:33 UTC (permalink / raw) To: linux-kernel, linux-trace-kernel Cc: Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton From: Steven Rostedt <rostedt@goodmis.org> The "mask" passed in to set_trace_flag() has a single bit set. The function then checks if the mask is equal to one of the option masks and performs the appropriate function associated to that option. Instead of having a bunch of "if ()" statement, use a "switch ()" statement instead to make it cleaner and a bit more optimal. No function changes. Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> --- kernel/trace/trace.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 07bd10808277..8460bec9f263 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -5251,11 +5251,13 @@ int trace_keep_overwrite(struct tracer *tracer, u64 mask, int set) int set_tracer_flag(struct trace_array *tr, u64 mask, int enabled) { - if ((mask == TRACE_ITER(RECORD_TGID)) || - (mask == TRACE_ITER(RECORD_CMD)) || - (mask == TRACE_ITER(TRACE_PRINTK)) || - (mask == TRACE_ITER(COPY_MARKER))) + switch (mask) { + case TRACE_ITER(RECORD_TGID): + case TRACE_ITER(RECORD_CMD): + case TRACE_ITER(TRACE_PRINTK): + case TRACE_ITER(COPY_MARKER): lockdep_assert_held(&event_mutex); + } /* do nothing if flag is already set */ if (!!(tr->trace_flags & mask) == !!enabled) @@ -5266,7 +5268,8 @@ int set_tracer_flag(struct trace_array *tr, u64 mask, int enabled) if (tr->current_trace->flag_changed(tr, mask, !!enabled)) return -EINVAL; - if (mask == TRACE_ITER(TRACE_PRINTK)) { + switch (mask) { + case TRACE_ITER(TRACE_PRINTK): if (enabled) { update_printk_trace(tr); } else { @@ -5283,9 +5286,9 @@ int set_tracer_flag(struct trace_array *tr, u64 mask, int enabled) if (printk_trace == tr) update_printk_trace(&global_trace); } - } + break; - if (mask == TRACE_ITER(COPY_MARKER)) { + case TRACE_ITER(COPY_MARKER): update_marker_trace(tr, enabled); /* update_marker_trace updates the tr->trace_flags */ return 0; @@ -5296,10 +5299,12 @@ int set_tracer_flag(struct trace_array *tr, u64 mask, int enabled) else tr->trace_flags &= ~mask; - if (mask == TRACE_ITER(RECORD_CMD)) + switch (mask) { + case TRACE_ITER(RECORD_CMD): trace_event_enable_cmd_record(enabled); + break; - if (mask == TRACE_ITER(RECORD_TGID)) { + case TRACE_ITER(RECORD_TGID): if (trace_alloc_tgid_map() < 0) { tr->trace_flags &= ~TRACE_ITER(RECORD_TGID); @@ -5307,24 +5312,27 @@ int set_tracer_flag(struct trace_array *tr, u64 mask, int enabled) } trace_event_enable_tgid_record(enabled); - } + break; - if (mask == TRACE_ITER(EVENT_FORK)) + case TRACE_ITER(EVENT_FORK): trace_event_follow_fork(tr, enabled); + break; - if (mask == TRACE_ITER(FUNC_FORK)) + case TRACE_ITER(FUNC_FORK): ftrace_pid_follow_fork(tr, enabled); + break; - if (mask == TRACE_ITER(OVERWRITE)) { + case TRACE_ITER(OVERWRITE): ring_buffer_change_overwrite(tr->array_buffer.buffer, enabled); #ifdef CONFIG_TRACER_MAX_TRACE ring_buffer_change_overwrite(tr->max_buffer.buffer, enabled); #endif - } + break; - if (mask == TRACE_ITER(PRINTK)) { + case TRACE_ITER(PRINTK): trace_printk_start_stop_comm(enabled); trace_printk_control(enabled); + break; } return 0; -- 2.51.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] tracing: Use switch statement instead of ifs in set_tracer_flag() 2025-11-06 0:33 ` [PATCH 2/2] tracing: Use switch statement instead of ifs in set_tracer_flag() Steven Rostedt @ 2025-11-10 5:48 ` Masami Hiramatsu 2025-11-10 16:42 ` Steven Rostedt 0 siblings, 1 reply; 6+ messages in thread From: Masami Hiramatsu @ 2025-11-10 5:48 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, linux-trace-kernel, Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton On Wed, 05 Nov 2025 19:33:26 -0500 Steven Rostedt <rostedt@kernel.org> wrote: > From: Steven Rostedt <rostedt@goodmis.org> > > The "mask" passed in to set_trace_flag() has a single bit set. The > function then checks if the mask is equal to one of the option masks and > performs the appropriate function associated to that option. > > Instead of having a bunch of "if ()" statement, use a "switch ()" > statement instead to make it cleaner and a bit more optimal. > > No function changes. > Looks good to me. Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> BTW, set_tracer_flag() seems to expect to modify only one bit. If we can count the number of its in @mask and reject if it is not 1, we can use bit-mask instead of the first switch()? if (!mask || /* mask has no bit */ (mask & ~(1 << (ffs64(mask) - 1)))) /* mask has more than 2 bits */ return -EINVAL; Thanks, > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > kernel/trace/trace.c | 38 +++++++++++++++++++++++--------------- > 1 file changed, 23 insertions(+), 15 deletions(-) > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 07bd10808277..8460bec9f263 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -5251,11 +5251,13 @@ int trace_keep_overwrite(struct tracer *tracer, u64 mask, int set) > > int set_tracer_flag(struct trace_array *tr, u64 mask, int enabled) > { > - if ((mask == TRACE_ITER(RECORD_TGID)) || > - (mask == TRACE_ITER(RECORD_CMD)) || > - (mask == TRACE_ITER(TRACE_PRINTK)) || > - (mask == TRACE_ITER(COPY_MARKER))) > + switch (mask) { > + case TRACE_ITER(RECORD_TGID): > + case TRACE_ITER(RECORD_CMD): > + case TRACE_ITER(TRACE_PRINTK): > + case TRACE_ITER(COPY_MARKER): > lockdep_assert_held(&event_mutex); > + } > > /* do nothing if flag is already set */ > if (!!(tr->trace_flags & mask) == !!enabled) > @@ -5266,7 +5268,8 @@ int set_tracer_flag(struct trace_array *tr, u64 mask, int enabled) > if (tr->current_trace->flag_changed(tr, mask, !!enabled)) > return -EINVAL; > > - if (mask == TRACE_ITER(TRACE_PRINTK)) { > + switch (mask) { > + case TRACE_ITER(TRACE_PRINTK): > if (enabled) { > update_printk_trace(tr); > } else { > @@ -5283,9 +5286,9 @@ int set_tracer_flag(struct trace_array *tr, u64 mask, int enabled) > if (printk_trace == tr) > update_printk_trace(&global_trace); > } > - } > + break; > > - if (mask == TRACE_ITER(COPY_MARKER)) { > + case TRACE_ITER(COPY_MARKER): > update_marker_trace(tr, enabled); > /* update_marker_trace updates the tr->trace_flags */ > return 0; > @@ -5296,10 +5299,12 @@ int set_tracer_flag(struct trace_array *tr, u64 mask, int enabled) > else > tr->trace_flags &= ~mask; > > - if (mask == TRACE_ITER(RECORD_CMD)) > + switch (mask) { > + case TRACE_ITER(RECORD_CMD): > trace_event_enable_cmd_record(enabled); > + break; > > - if (mask == TRACE_ITER(RECORD_TGID)) { > + case TRACE_ITER(RECORD_TGID): > > if (trace_alloc_tgid_map() < 0) { > tr->trace_flags &= ~TRACE_ITER(RECORD_TGID); > @@ -5307,24 +5312,27 @@ int set_tracer_flag(struct trace_array *tr, u64 mask, int enabled) > } > > trace_event_enable_tgid_record(enabled); > - } > + break; > > - if (mask == TRACE_ITER(EVENT_FORK)) > + case TRACE_ITER(EVENT_FORK): > trace_event_follow_fork(tr, enabled); > + break; > > - if (mask == TRACE_ITER(FUNC_FORK)) > + case TRACE_ITER(FUNC_FORK): > ftrace_pid_follow_fork(tr, enabled); > + break; > > - if (mask == TRACE_ITER(OVERWRITE)) { > + case TRACE_ITER(OVERWRITE): > ring_buffer_change_overwrite(tr->array_buffer.buffer, enabled); > #ifdef CONFIG_TRACER_MAX_TRACE > ring_buffer_change_overwrite(tr->max_buffer.buffer, enabled); > #endif > - } > + break; > > - if (mask == TRACE_ITER(PRINTK)) { > + case TRACE_ITER(PRINTK): > trace_printk_start_stop_comm(enabled); > trace_printk_control(enabled); > + break; > } > > return 0; > -- > 2.51.0 > > -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] tracing: Use switch statement instead of ifs in set_tracer_flag() 2025-11-10 5:48 ` Masami Hiramatsu @ 2025-11-10 16:42 ` Steven Rostedt 0 siblings, 0 replies; 6+ messages in thread From: Steven Rostedt @ 2025-11-10 16:42 UTC (permalink / raw) To: Masami Hiramatsu (Google) Cc: Steven Rostedt, linux-kernel, linux-trace-kernel, Mark Rutland, Mathieu Desnoyers, Andrew Morton On Mon, 10 Nov 2025 14:48:26 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > BTW, set_tracer_flag() seems to expect to modify only one bit. > If we can count the number of its in @mask and reject if it is > not 1, we can use bit-mask instead of the first switch()? > > if (!mask || /* mask has no bit */ > (mask & ~(1 << (ffs64(mask) - 1)))) /* mask has more than 2 bits */ > return -EINVAL; Well, this has been around for over a decade without any issues. I don't think a check would be of much use. Not to mention, invalid masks are OK to pass in. If anything, I would have liked to pass in the bit number and not a mask. But that's something we could do another time. -- Steve ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-11-10 16:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-11-06 0:33 [PATCH 0/2] tracing: Clean up of set_tracer_flag() Steven Rostedt 2025-11-06 0:33 ` [PATCH 1/2] tracing: Exit out immediately after update_marker_trace() Steven Rostedt 2025-11-10 5:24 ` Masami Hiramatsu 2025-11-06 0:33 ` [PATCH 2/2] tracing: Use switch statement instead of ifs in set_tracer_flag() Steven Rostedt 2025-11-10 5:48 ` Masami Hiramatsu 2025-11-10 16:42 ` Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox