* [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
* [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 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
* 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;
as well as URLs for NNTP newsgroup(s).