On Wed, 05 Nov 2025 19:33:26 -0500 Steven Rostedt <[email protected]> wrote:
> From: Steven Rostedt <[email protected]> > > 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) <[email protected]> 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) <[email protected]> > --- > 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) <[email protected]>
