[PATCH] rtla: Stop the record trace on interrupt

[PATCH] rtla: Stop the record trace on interrupt

[Crystal Wood]

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.

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
 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);
 		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);
+	}
 }
 
 /*
@@ -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;
 
 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);
 		if (max_lat) {
 			printf("  Max latency was %s\n", max_lat);
 			free(max_lat);
-- 
2.54.0

Re: [PATCH] rtla: Stop the record trace on interrupt

[Tomas Glozar]

út 12. 5. 2026 v 0:35 odesílatel Crystal Wood <crwood@redhat.com> 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 <crwood@redhat.com>
> ---
>  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

Re: [PATCH] rtla: Stop the record trace on interrupt

[Tomas Glozar]

út 12. 5. 2026 v 15:50 odesílatel Tomas Glozar <tglozar@redhat.com> napsal:
>
> This affects only the "new" (since 6.19) --on-end trace option, right?

s/6.19/6.17/