út 12. 5. 2026 v 0:35 odesílatel Crystal Wood <[email protected]> napsal: > > Before, when rtla gets a signal, it stopped the main trace but not the > record trace. save_trace_to_file() could also fail to keep up on a debug > kernel -- and in any case, it adds post-stoppage noise to the trace file. >
This affects only the "new" (since 6.19) --on-end trace option, right? The regular --trace/--on-threshold trace should already disable the instance in-kernel, as timerlat disables all instances set to the tracer on stop_tracing threshold. If so, that should be reflected in the commit message. > Signed-off-by: Crystal Wood <[email protected]> > --- > tools/tracing/rtla/src/common.c | 19 +++++++++++-------- > tools/tracing/rtla/src/common.h | 1 - > tools/tracing/rtla/src/timerlat.c | 2 +- > 3 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/tools/tracing/rtla/src/common.c b/tools/tracing/rtla/src/common.c > index 35e3d3aa922e..effad523e8cf 100644 > --- a/tools/tracing/rtla/src/common.c > +++ b/tools/tracing/rtla/src/common.c > @@ -10,7 +10,7 @@ > > #include "common.h" > > -struct trace_instance *trace_inst; > +struct osnoise_tool *trace_tool; > volatile int stop_tracing; > int nr_cpus; > > @@ -21,12 +21,16 @@ static void stop_trace(int sig) > * Stop requested twice in a row; abort event processing and > * exit immediately > */ > - tracefs_iterate_stop(trace_inst->inst); > + if (trace_tool) > + tracefs_iterate_stop(trace_tool->trace.inst); Can this condition be ever false? The signal should be attached after trace_inst is initialized. Of course, it doesn't hurt to have it there for safety, as long as we keep in mind that it does not protect from the trace instance being freed (which should be fixed by commit be8058f31b4e already). > return; > } > stop_tracing = 1; > - if (trace_inst) > - trace_instance_stop(trace_inst); > + if (trace_tool) { > + trace_instance_stop(&trace_tool->trace); > + if (trace_tool->record) > + trace_instance_stop(&trace_tool->record->trace); > + } > } > Otherwise this makes sense. The reason for doing trace_instance_stop() in stop_trace() and not waiting for cleanup is that in tracefs mode, the loop would get struck in tracefs_iterate_raw_events() if it cannot process the events faster than they are created (see commit c73cab9dbed and a4dfce7559d respectively). But it can cause a discrepancy in the trace output, as you point out. > /* > @@ -273,11 +277,10 @@ int run_tool(struct tool_ops *ops, int argc, char > *argv[]) > tool->params = params; > > /* > - * Save trace instance into global variable so that SIGINT can stop > - * the timerlat tracer. > + * Expose the tool to signal handlers so they can stop the trace. > * Otherwise, rtla could loop indefinitely when overloaded. > */ > - trace_inst = &tool->trace; > + trace_tool = tool; > > retval = ops->apply_config(tool); > if (retval) { > @@ -285,7 +288,7 @@ int run_tool(struct tool_ops *ops, int argc, char *argv[]) > goto out_free; > } > > - retval = enable_tracer_by_name(trace_inst->inst, ops->tracer); > + retval = enable_tracer_by_name(tool->trace.inst, ops->tracer); > if (retval) { > err_msg("Failed to enable %s tracer\n", ops->tracer); > goto out_free; > diff --git a/tools/tracing/rtla/src/common.h b/tools/tracing/rtla/src/common.h > index 51665db4ffce..eba40b6d9504 100644 > --- a/tools/tracing/rtla/src/common.h > +++ b/tools/tracing/rtla/src/common.h > @@ -54,7 +54,6 @@ struct osnoise_context { > int opt_workload; > }; > > -extern struct trace_instance *trace_inst; > extern volatile int stop_tracing; > This is removed as it is not needed now after the consolidation I assume, since all users are now in common.c? That could also be mentioned in the commit message. > struct hist_params { > diff --git a/tools/tracing/rtla/src/timerlat.c > b/tools/tracing/rtla/src/timerlat.c > index f8c057518d22..637f68d684f5 100644 > --- a/tools/tracing/rtla/src/timerlat.c > +++ b/tools/tracing/rtla/src/timerlat.c > @@ -202,7 +202,7 @@ void timerlat_analyze(struct osnoise_tool *tool, bool > stopped) > * If the trace did not stop with --aa-only, at least print > * the max known latency. > */ > - max_lat = tracefs_instance_file_read(trace_inst->inst, > "tracing_max_latency", NULL); > + max_lat = tracefs_instance_file_read(tool->trace.inst, > "tracing_max_latency", NULL); Not sure why this used trace_inst instead of tool which is right there, maybe again refactoring of the code? > if (max_lat) { > printf(" Max latency was %s\n", max_lat); > free(max_lat); > -- > 2.54.0 > Otherwise, LGTM. You can also add: Fixes: c73cab9dbed0 ("rtla/timerlat_hist: Stop timerlat tracer on signal") Fixes: a4dfce7559d7 ("rtla/timerlat_top: Stop timerlat tracer on signal") Fixes: 3aadb65db5d6 ("rtla/timerlat: Add action on end feature") as this fixes two bugs, one with the post-stoppage noise (that is present since the trace_instance_stop() was added), one with the save_trace_to_file() not keeping up (which should be since the --on-end option was added). Tomas
