[PATCH v1] rtla: Save trace when option `--trace` is specified

[PATCH v1] rtla: Save trace when option `--trace` is specified

[Costa Shulyupin]

Rtla saves trace output only when option `--auto` or `--stop` is
specified. Single option `--trace` enables tracing, but doesn't save it.

Save trace output in any case.

Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
---
 tools/tracing/rtla/src/osnoise_hist.c | 9 ++++-----
 tools/tracing/rtla/src/osnoise_top.c  | 9 ++++-----
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/tools/tracing/rtla/src/osnoise_hist.c b/tools/tracing/rtla/src/osnoise_hist.c
index b4930b835b0a1..38fd10b2a3488 100644
--- a/tools/tracing/rtla/src/osnoise_hist.c
+++ b/tools/tracing/rtla/src/osnoise_hist.c
@@ -981,12 +981,11 @@ int osnoise_hist_main(int argc, char *argv[])
 
 	return_value = 0;
 
-	if (osnoise_trace_is_off(tool, record)) {
+	if (osnoise_trace_is_off(tool, record))
 		printf("rtla osnoise hit stop tracing\n");
-		if (params->trace_output) {
-			printf("  Saving trace to %s\n", params->trace_output);
-			save_trace_to_file(record->trace.inst, params->trace_output);
-		}
+	if (params->trace_output) {
+		printf("  Saving trace to %s\n", params->trace_output);
+		save_trace_to_file(record->trace.inst, params->trace_output);
 	}
 
 out_hist:
diff --git a/tools/tracing/rtla/src/osnoise_top.c b/tools/tracing/rtla/src/osnoise_top.c
index 4772677ac762c..0af639982b7c1 100644
--- a/tools/tracing/rtla/src/osnoise_top.c
+++ b/tools/tracing/rtla/src/osnoise_top.c
@@ -811,12 +811,11 @@ int osnoise_top_main(int argc, char **argv)
 
 	return_value = 0;
 
-	if (osnoise_trace_is_off(tool, record)) {
+	if (osnoise_trace_is_off(tool, record))
 		printf("osnoise hit stop tracing\n");
-		if (params->trace_output) {
-			printf("  Saving trace to %s\n", params->trace_output);
-			save_trace_to_file(record->trace.inst, params->trace_output);
-		}
+	if (params->trace_output) {
+		printf("  Saving trace to %s\n", params->trace_output);
+		save_trace_to_file(record->trace.inst, params->trace_output);
 	}
 
 out_top:
-- 
2.47.0

Re: [PATCH v1] rtla: Save trace when option `--trace` is specified

[Steven Rostedt]

On Mon, 27 Jan 2025 19:02:40 +0200
Costa Shulyupin <costa.shul@redhat.com> wrote:

> +++ b/tools/tracing/rtla/src/osnoise_hist.c
> @@ -981,12 +981,11 @@ int osnoise_hist_main(int argc, char *argv[])
>  
>  	return_value = 0;
>  
> -	if (osnoise_trace_is_off(tool, record)) {
> +	if (osnoise_trace_is_off(tool, record))
>  		printf("rtla osnoise hit stop tracing\n");
> -		if (params->trace_output) {
> -			printf("  Saving trace to %s\n", params->trace_output);
> -			save_trace_to_file(record->trace.inst, params->trace_output);
> -		}
> +	if (params->trace_output) {
> +		printf("  Saving trace to %s\n", params->trace_output);
> +		save_trace_to_file(record->trace.inst, params->trace_output);
>  	}

Without tracing being off, this can run forever if the events come in
faster than it can be recorded. And save trace uses the "trace" file, which
is slow to read.

Are you sure you want that?

-- Steve

Re: [PATCH v1] rtla: Save trace when option `--trace` is specified

[Tomas Glozar]

pá 28. 2. 2025 v 2:05 odesílatel Steven Rostedt <rostedt@goodmis.org> napsal:
>
> Without tracing being off, this can run forever if the events come in
> faster than it can be recorded. And save trace uses the "trace" file, which
> is slow to read.
>
> Are you sure you want that?
>

Isn't that a different problem though? As far as I'm aware, it
shouldn't matter at all whether tracing is stopped on the main
instance for collecting the trace from the record instance.

If the record instance generates a lot of samples, this can already
run forever, regardless of whether tracing is stopped on the main
instance or not. This should be a separate fix.

Tomas

Re: [PATCH v1] rtla: Save trace when option `--trace` is specified

[Costa Shulyupin]

Actually trace collection can be performed using just trace-cmd start,
trace-cmd stop, and trace-cmd extract.

Re: [PATCH v1] rtla: Save trace when option `--trace` is specified

[Steven Rostedt]

On Mon, 3 Mar 2025 16:34:21 +0100
Tomas Glozar <tglozar@redhat.com> wrote:

> pá 28. 2. 2025 v 2:05 odesílatel Steven Rostedt <rostedt@goodmis.org> napsal:
> >
> > Without tracing being off, this can run forever if the events come in
> > faster than it can be recorded. And save trace uses the "trace" file, which
> > is slow to read.
> >
> > Are you sure you want that?
> >  
> 
> Isn't that a different problem though? As far as I'm aware, it
> shouldn't matter at all whether tracing is stopped on the main
> instance for collecting the trace from the record instance.
> 
> If the record instance generates a lot of samples, this can already
> run forever, regardless of whether tracing is stopped on the main
> instance or not. This should be a separate fix.

Not sure what you mean by "main instance"?

The code being changed is:

-	if (osnoise_trace_is_off(tool, record)) {
+	if (osnoise_trace_is_off(tool, record))
 		printf("rtla osnoise hit stop tracing\n");
-		if (params->trace_output) {
-			printf("  Saving trace to %s\n", params->trace_output);
-			save_trace_to_file(record->trace.inst, params->trace_output);
-		}
+	if (params->trace_output) {
+		printf("  Saving trace to %s\n", params->trace_output);
+		save_trace_to_file(record->trace.inst, params->trace_output);
 	}

Where osnoise_trace_is_off() is:

bool osnoise_trace_is_off(struct osnoise_tool *tool, struct osnoise_tool *record)
{
        /*
         * The tool instance is always present, it is the one used to collect
         * data.
         */
        if (!tracefs_trace_is_on(tool->trace.inst))
                return true;

        /*
         * The trace record instance is only enabled when -t is set. IOW, when the system
         * is tracing.
         */
        return record && !tracefs_trace_is_on(record->trace.inst);
}


Which returns true if the tool->trace.inst is on and false otherwise.

-- Steve

Re: [PATCH v1] rtla: Save trace when option `--trace` is specified

[Tomas Glozar]

po 3. 3. 2025 v 21:04 odesílatel Steven Rostedt <rostedt@goodmis.org> napsal:
>
> Not sure what you mean by "main instance"?
>

By "main instance", I meant tool->trace.inst. My point was that
record->trace.inst, which is a different instance, would be still on.
But that's not the case, osnoise_stop_tracing() stops *all* osnoise
instances, not just the first one - sorry, that was my mistake.

>
> The code being changed is:
>
> -       if (osnoise_trace_is_off(tool, record)) {
> +       if (osnoise_trace_is_off(tool, record))
>                 printf("rtla osnoise hit stop tracing\n");
> -               if (params->trace_output) {
> -                       printf("  Saving trace to %s\n", params->trace_output);
> -                       save_trace_to_file(record->trace.inst, params->trace_output);
> -               }
> +       if (params->trace_output) {
> +               printf("  Saving trace to %s\n", params->trace_output);
> +               save_trace_to_file(record->trace.inst, params->trace_output);
>         }
>

So we need to stop tracing here, before we save the trace, if we want
to do that. Perhaps combining this with the cleanup patch [1] and
doing the stopping in save_trace_to_file would make sense?

[1] - https://lore.kernel.org/linux-trace-kernel/20250219115138.406075-1-costa.shul@redhat.com/

Tomas

Re: [PATCH v1] rtla: Save trace when option `--trace` is specified

[Tomas Glozar]

po 3. 3. 2025 v 16:55 odesílatel Costa Shulyupin <costa.shul@redhat.com> napsal:
>
> Actually trace collection can be performed using just trace-cmd start,
> trace-cmd stop, and trace-cmd extract.
>

Yes, but that does not necessarily mean we cannot add it to rtla, too;
we already do that e.g. for event histograms, which you could also
replace with trace-cmd. It has the benefit of synchronizing the
start/stop of the trace collection with the osnoise/timerlat tracer.

Tomas

Re: [PATCH v1] rtla: Save trace when option `--trace` is specified

[Tomas Glozar]

út 4. 3. 2025 v 9:00 odesílatel Tomas Glozar <tglozar@redhat.com> napsal:
>
> So we need to stop tracing here, before we save the trace, if we want
> to do that. Perhaps combining this with the cleanup patch [1] and
> doing the stopping in save_trace_to_file would make sense?
>

Also, the patch will also save the trace if running with -a and the
threshold was not violated, which is not what one usually wants, e.g.:

$ rtla osnoise top -c 0 -q -a 10000000 -d 5s
                                         Operating System Noise

duration:   0 00:00:05 | time is in us
CPU Period       Runtime        Noise  % CPU Aval   Max Noise   Max
Single          HW          NMI          IRQ      Softirq       Threa
d
 0 #4           4000000        37712    99.05720       10998
555        7624            0         4011           34           2
4
 Saving trace to osnoise_trace.txt

I believe it would be better to add a new option, something like
--force-trace, that would be used to save the trace even if there is
no threshold violation. -t/--trace and -a could then be used with the
same semantics as before.

Tomas

Re: [PATCH v1] rtla: Save trace when option `--trace` is specified

[Steven Rostedt]

On Tue, 4 Mar 2025 09:16:11 +0100
Tomas Glozar <tglozar@redhat.com> wrote:

> út 4. 3. 2025 v 9:00 odesílatel Tomas Glozar <tglozar@redhat.com> napsal:
> >
> > So we need to stop tracing here, before we save the trace, if we want
> > to do that. Perhaps combining this with the cleanup patch [1] and
> > doing the stopping in save_trace_to_file would make sense?
> >  
> 
> Also, the patch will also save the trace if running with -a and the
> threshold was not violated, which is not what one usually wants, e.g.:
> 
> $ rtla osnoise top -c 0 -q -a 10000000 -d 5s
>                                          Operating System Noise
> 
> duration:   0 00:00:05 | time is in us
> CPU Period       Runtime        Noise  % CPU Aval   Max Noise   Max
> Single          HW          NMI          IRQ      Softirq       Threa
> d
>  0 #4           4000000        37712    99.05720       10998
> 555        7624            0         4011           34           2
> 4
>  Saving trace to osnoise_trace.txt
> 
> I believe it would be better to add a new option, something like
> --force-trace, that would be used to save the trace even if there is
> no threshold violation. -t/--trace and -a could then be used with the
> same semantics as before.

As long is this is what you expect to happen. I just wanted to point out
that recording the trace while it is active means it may never stop
recording. If that is OK, then I'm fine with the change.

-- Steve

Re: [PATCH v1] rtla: Save trace when option `--trace` is specified

[Steven Rostedt]

On Tue, 4 Mar 2025 12:10:53 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 4 Mar 2025 09:16:11 +0100
> Tomas Glozar <tglozar@redhat.com> wrote:
> 
> > út 4. 3. 2025 v 9:00 odesílatel Tomas Glozar <tglozar@redhat.com> napsal:  
> > >
> > > So we need to stop tracing here, before we save the trace, if we want
> > > to do that. Perhaps combining this with the cleanup patch [1] and
> > > doing the stopping in save_trace_to_file would make sense?
> > >    
> > 
> > Also, the patch will also save the trace if running with -a and the
> > threshold was not violated, which is not what one usually wants, e.g.:
> > 
> > $ rtla osnoise top -c 0 -q -a 10000000 -d 5s
> >                                          Operating System Noise
> > 
> > duration:   0 00:00:05 | time is in us
> > CPU Period       Runtime        Noise  % CPU Aval   Max Noise   Max
> > Single          HW          NMI          IRQ      Softirq       Threa
> > d
> >  0 #4           4000000        37712    99.05720       10998
> > 555        7624            0         4011           34           2
> > 4
> >  Saving trace to osnoise_trace.txt
> > 
> > I believe it would be better to add a new option, something like
> > --force-trace, that would be used to save the trace even if there is
> > no threshold violation. -t/--trace and -a could then be used with the
> > same semantics as before.  
> 
> As long is this is what you expect to happen. I just wanted to point out
> that recording the trace while it is active means it may never stop
> recording. If that is OK, then I'm fine with the change.
>

I just noticed that this patch was never applied. Is it still something to add?

-- Steve

Re: [PATCH v1] rtla: Save trace when option `--trace` is specified

[Costa Shulyupin]

On Thu, 1 May 2025 at 17:51, Steven Rostedt <rostedt@goodmis.org> wrote:
> I just noticed that this patch was never applied. Is it still something to add?
No, it’s no longer needed — we can drop it.
Costa