Re: [PATCH] perf trace: Keep exited threads for summary

[Namhyung Kim <namhyung@kernel.org>]

On Mon, Sep 30, 2024 at 11:07:45AM +0200, Veronika Molnarova wrote:
> 
> 
> On 9/27/24 17:19, Michael Petlan wrote:
> > Since 9ffa6c7512ca ("perf machine thread: Remove exited threads by
> > default") perf cleans exited threads up, but as said, sometimes they
> > are necessary to be kept. The mentioned commit does not cover all the
> > cases, we also need the information to construct the summary table in
> > perf-trace.
> > 
> > Before:
> >     # perf trace -s true
> > 
> >      Summary of events:
> > 
> > After:
> >     # perf trace -s -- true
> > 
> >      Summary of events:
> > 
> >      true (383382), 64 events, 91.4%
> > 
> >        syscall            calls  errors  total       min       avg       max       stddev
> >                                          (msec)    (msec)    (msec)    (msec)        (%)
> >        --------------- --------  ------ -------- --------- --------- ---------     ------
> >        mmap                   8      0     0.150     0.013     0.019     0.031     11.90%
> >        mprotect               3      0     0.045     0.014     0.015     0.017      6.47%
> >        openat                 2      0     0.014     0.006     0.007     0.007      9.73%
> >        munmap                 1      0     0.009     0.009     0.009     0.009      0.00%
> >        access                 1      1     0.009     0.009     0.009     0.009      0.00%
> >        pread64                4      0     0.006     0.001     0.001     0.002      4.53%
> >        fstat                  2      0     0.005     0.001     0.002     0.003     37.59%
> >        arch_prctl             2      1     0.003     0.001     0.002     0.002     25.91%
> >        read                   1      0     0.003     0.003     0.003     0.003      0.00%
> >        close                  2      0     0.003     0.001     0.001     0.001      3.86%
> >        brk                    1      0     0.002     0.002     0.002     0.002      0.00%
> >        rseq                   1      0     0.001     0.001     0.001     0.001      0.00%
> >        prlimit64              1      0     0.001     0.001     0.001     0.001      0.00%
> >        set_robust_list        1      0     0.001     0.001     0.001     0.001      0.00%
> >        set_tid_address        1      0     0.001     0.001     0.001     0.001      0.00%
> >        execve                 1      0     0.000     0.000     0.000     0.000      0.00%
> > 
> > Fixes: 9ffa6c7512ca ("perf machine thread: Remove exited threads by default")
> > 
> > Reported-by: Veronika Molnarova <vmolnaro@redhat.com>
> > Signed-off-by: Michael Petlan <mpetlan@redhat.com>
> 
> The -S and -s option works after this patch, but --errno-summary ends with
> segfault for some reason.
> 
> Before:
> 	# perf trace --errno-summary -- true
> 
> 
>  	Summary of events:
> 
> After:
> 	# perf trace --errno-summary -- true
> 
> 	 Summary of events:
> 	
> 	 true (11386), 60 events, 90.9%
> 	
>    	   syscall            calls  errors  total       min       avg       max       stddev
>         	                             (msec)    (msec)    (msec)    (msec)        (%)
>  	   --------------- --------  ------ -------- --------- --------- ---------     ------
>    	   mmap                   8      0     0.044     0.003     0.006     0.010     15.17%
>    	   mprotect               3      0     0.014     0.004     0.005     0.005      4.56%
>    	   openat                 2      0     0.009     0.004     0.004     0.005      5.14%
>    	   munmap                 1      0     0.007     0.007     0.007     0.007      0.00%
>   	   access                 1      1     0.004     0.004     0.004     0.004      0.00%
> 	perf: Segmentation fault
> 	Obtained 12 stack frames.
> 	./perf() [0x5ae123]
> 	./perf() [0x5ae1cb]
> 	/lib64/libc.so.6(+0x3dbb0) [0x7f633845fbb0]
> 	./perf() [0x56e2fb]
> 	./perf() [0x4850b3]
> 	./perf() [0x48ee89]
> 	./perf() [0x493042]
> 	./perf() [0x49334b]
> 	./perf() [0x40ee8c]
> 	/lib64/libc.so.6(+0x27b8a) [0x7f6338449b8a]
> 	/lib64/libc.so.6(__libc_start_main+0x8b) [0x7f6338449c4b]
> 	./perf() [0x40f4b5]
> 	Segmentation fault
> 
> Will try to check what is causing this issue.

It seems like a different bug and shoul not be affected by this change.

  Program received signal SIGSEGV, Segmentation fault.
  0x000055555574cf67 in perf_env__arch_strerrno (env=0x0, err=110) at util/env.c:475
  475		if (env->arch_strerrno == NULL)
  (gdb) bt
  #0  0x000055555574cf67 in perf_env__arch_strerrno (env=0x0, err=110) at util/env.c:475
  #1  0x000055555563fc0b in thread__dump_stats (ttrace=0x55555661a220, trace=0x7fffffffaaf0, fp=0x7fffee3f24e0 <_IO_2_1_stderr_>) at builtin-trace.c:4674
  #2  0x000055555563fef8 in trace__fprintf_thread (fp=0x7fffee3f24e0 <_IO_2_1_stderr_>, thread=0x5555560f75d0, trace=0x7fffffffaaf0) at builtin-trace.c:4709
  #3  0x0000555555640112 in trace__fprintf_thread_summary (trace=0x7fffffffaaf0, fp=0x7fffee3f24e0 <_IO_2_1_stderr_>) at builtin-trace.c:4748
  #4  0x000055555563eba7 in trace__run (trace=0x7fffffffaaf0, argc=2, argv=0x7fffffffe390) at builtin-trace.c:4457
  #5  0x0000555555642a61 in cmd_trace (argc=2, argv=0x7fffffffe390) at builtin-trace.c:5484
  #6  0x0000555555648a37 in run_builtin (p=0x5555560185e8 <commands+648>, argc=5, argv=0x7fffffffe390) at perf.c:351
  #7  0x0000555555648cde in handle_internal_command (argc=5, argv=0x7fffffffe390) at perf.c:404
  #8  0x0000555555648e37 in run_argv (argcp=0x7fffffffe18c, argv=0x7fffffffe180) at perf.c:448
  #9  0x0000555555649185 in main (argc=5, argv=0x7fffffffe390) at perf.c:562

It needs to pass a non-NULL 'env' when --errno-summary is given.

Anyway, I'll pick this up.

Thanks,
Namhyung


> 
> > ---
> >  tools/perf/builtin-trace.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index f6e847529073..1a12ed71c809 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -5449,6 +5449,10 @@ int cmd_trace(int argc, const char **argv)
> >  	if (trace.summary_only)
> >  		trace.summary = trace.summary_only;
> >  
> > +	/* Keep exited threads, otherwise information might be lost for summary */
> > +	if (trace.summary || trace.summary_only)
> > +		symbol_conf.keep_exited_threads = true;
> > +
> >  	if (output_name != NULL) {
> >  		err = trace__open_output(&trace, output_name);
> >  		if (err < 0) {
>