Re: [PATCH v2] perf trace: Implement syscall summary in BPF

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Mar 19, 2025 at 04:39:17PM -0700, Howard Chu wrote:
> Hi Namhyung,
> 
> I haven't finished the code review yet, but here are something that I
> found interesting to share:

Thanks a lot for your review!

> 
> ## 1
> You used sudo ./perf trace -as --bpf-summary --summary-mode=total -- sleep 1 as
> an example
> 
> If I use perf trace --bpf-summary without the '-a', that is to record
> the process / task of 'sleep 1':
> 
> sudo ./perf trace -s --bpf-summary --summary-mode=total -- sleep 1
> 
> It won't be recording this one process. So there should be a sentence
> saying that bpf-summary only does system wide summaries.

Hmm.. you're right.  I added per-thread summary but it still works for
system-wide only.  I'll check the target and make it fail with an error
message if it's not in system-wide mode for now.  I think we can add
support for other targets later.

> 
> ## 2
> there is a bug in min section, which made min always 0
> 
> you can see it in the sample output you provided above:
>      syscall            calls  errors  total       min       avg
> max       stddev
>                                        (msec)    (msec)    (msec)
> (msec)        (%)
>      --------------- --------  ------ -------- --------- ---------
> ---------     ------
>      futex                372     18  4373.773     0.000    11.757
> 997.715    660.42%
>      poll                 241      0  2757.963     0.000    11.444
> 997.758    580.34%
>      epoll_wait           161      0  2460.854     0.000    15.285
> 325.189    260.73%
>      ppoll                 19      0  1298.652     0.000    68.350
> 667.172    281.46%
>      clock_nanosleep        1      0  1000.093     0.000  1000.093
> 1000.093      0.00%
>      epoll_pwait           16      0   192.787     0.000    12.049
> 173.994    348.73%
>      nanosleep              6      0    50.926     0.000     8.488
> 10.210     43.96%
> 
> clock_nanosleep has only 1 call so min can never be 0, it has to be
> equal to the max and the mean.

Oops, right.

> 
> This can be resolved by adding this line (same as what you did in the BPF code):
> 
> diff --git a/tools/perf/util/bpf-trace-summary.c
> b/tools/perf/util/bpf-trace-summary.c
> index 5ae9feca244d..eb98db7d6e33 100644
> --- a/tools/perf/util/bpf-trace-summary.c
> +++ b/tools/perf/util/bpf-trace-summary.c
> @@ -243,7 +243,7 @@ static int update_total_stats(struct hashmap
> *hash, struct syscall_key *map_key,
> 
>   if (stat->max_time < map_data->max_time)
>   stat->max_time = map_data->max_time;
> - if (stat->min_time > map_data->min_time)
> + if (stat->min_time > map_data->min_time || !stat->min_time)
>   stat->min_time = map_data->min_time;
> 
>   return 0;
> 
> (sorry for the poor formatting from the gmail browser app)

No problem, I'll add this change.

> 
> ## 3
> Apologies for misunderstanding how the calculation of the 'standard
> deviation of mean' works. You can decide what to do with it. :) Thanks
> for the explanation in the thread of the previous version.

No, it turns out that it can be calculated easily with the squared sum.
So I've added it.

Thanks,
Namhyung

> 
> Thanks,
> Howard
> 
> On Wed, Mar 19, 2025 at 3:19 PM Howard Chu <howardchu95@gmail.com> wrote:
> >
> > Hi Namhyung,
> >
> > On Wed, Mar 19, 2025 at 3:07 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Hello Howard,
> > >
> > > On Wed, Mar 19, 2025 at 12:00:10PM -0700, Howard Chu wrote:
> > > > Hello Namhyung,
> > > >
> > > > Can you please rebase it? I cannot apply it, getting:
> > > >
> > > > perf $ git apply --reject --whitespace=fix
> > > > ./v2_20250317_namhyung_perf_trace_implement_syscall_summary_in_bpf.mbx
> > > > Checking patch tools/perf/Documentation/perf-trace.txt...
> > > > Checking patch tools/perf/Makefile.perf...
> > > > Hunk #1 succeeded at 1198 (offset -8 lines).
> > > > Checking patch tools/perf/builtin-trace.c...
> > > > error: while searching for:
> > > >         bool       hexret;
> > > > };
> > > >
> > > > enum summary_mode {
> > > >         SUMMARY__NONE = 0,
> > > >         SUMMARY__BY_TOTAL,
> > > >         SUMMARY__BY_THREAD,
> > > > };
> > > >
> > > > struct trace {
> > > >         struct perf_tool        tool;
> > > >         struct {
> > > >
> > > > error: patch failed: tools/perf/builtin-trace.c:140
> > >
> > > Oops, I think I forgot to say it's on top of Ian's change.
> > > Please try this first.  Sorry for the confusion.
> > >
> > > https://lore.kernel.org/r/20250319050741.269828-1-irogers@google.com
> >
> > Yep, with Ian's patches it successfully applied. :)
> >
> > Thanks,
> > Howard
> > >
> > > Thanks,
> > > Namhyung
> > >