Re: [PATCH] perf annotate: Fix --group behavior when leader has no samples

From: Arnaldo Carvalho de Melo
Date: Fri Aug 09 2024 - 17:15:39 EST


On Tue, Aug 06, 2024 at 11:15:55PM -0700, Namhyung Kim wrote:
> When --group option is used, it should display all events together. But
> the current logic only checks if the first (leader) event has samples or
> not. Let's check the member events as well.
>
> Also it missed to put the linked samples from member evsels to the
> output RB-tree so that it can be displayed in the output.

Thanks, re-tested and applied.

- Arnaldo

> For example, take a look at this example.
>
> $ ./perf evlist
> cpu/mem-loads,ldlat=30/P
> cpu/mem-stores/P
> dummy:u
>
> It has three events but 'path_put' function has samples only for
> mem-stores (second) event.
>
> $ sudo ./perf annotate --stdio -f path_put
> Percent | Source code & Disassembly of kcore for cpu/mem-stores/P (2 samples, percent: local period)
> ----------------------------------------------------------------------------------------------------------
> : 0 0xffffffffae600020 <path_put>:
> 0.00 : ffffffffae600020: endbr64
> 0.00 : ffffffffae600024: nopl (%rax, %rax)
> 91.22 : ffffffffae600029: pushq %rbx
> 0.00 : ffffffffae60002a: movq %rdi, %rbx
> 0.00 : ffffffffae60002d: movq 8(%rdi), %rdi
> 8.78 : ffffffffae600031: callq 0xffffffffae614aa0
> 0.00 : ffffffffae600036: movq (%rbx), %rdi
> 0.00 : ffffffffae600039: popq %rbx
> 0.00 : ffffffffae60003a: jmp 0xffffffffae620670
> 0.00 : ffffffffae60003f: nop
>
> Therefore, it didn't show up when --group option is used since the
> leader ("mem-loads") event has no samples. But now it checks both
> events.
>
> Before:
> $ sudo ./perf annotate --stdio -f --group path_put
> (no output)
>
> After:
> $ sudo ./perf annotate --stdio -f --group path_put
> Percent | Source code & Disassembly of kcore for cpu/mem-loads,ldlat=30/P, cpu/mem-stores/P, dummy:u (0 samples, percent: local period)
> -------------------------------------------------------------------------------------------------------------------------------------------------------------
> : 0 0xffffffffae600020 <path_put>:
> 0.00 0.00 0.00 : ffffffffae600020: endbr64
> 0.00 0.00 0.00 : ffffffffae600024: nopl (%rax, %rax)
> 0.00 91.22 0.00 : ffffffffae600029: pushq %rbx
> 0.00 0.00 0.00 : ffffffffae60002a: movq %rdi, %rbx
> 0.00 0.00 0.00 : ffffffffae60002d: movq 8(%rdi), %rdi
> 0.00 8.78 0.00 : ffffffffae600031: callq 0xffffffffae614aa0
> 0.00 0.00 0.00 : ffffffffae600036: movq (%rbx), %rdi
> 0.00 0.00 0.00 : ffffffffae600039: popq %rbx
> 0.00 0.00 0.00 : ffffffffae60003a: jmp 0xffffffffae620670
> 0.00 0.00 0.00 : ffffffffae60003f: nop
>
> Reported-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Tested-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
> tools/perf/builtin-annotate.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index efcadb7620b8..1bfe41783a7c 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -632,13 +632,23 @@ static int __cmd_annotate(struct perf_annotate *ann)
> evlist__for_each_entry(session->evlist, pos) {
> struct hists *hists = evsel__hists(pos);
> u32 nr_samples = hists->stats.nr_samples;
> + struct ui_progress prog;
> + struct evsel *evsel;
>
> - if (nr_samples == 0)
> + if (!symbol_conf.event_group || !evsel__is_group_leader(pos))
> continue;
>
> - if (!symbol_conf.event_group || !evsel__is_group_leader(pos))
> + for_each_group_member(evsel, pos)
> + nr_samples += evsel__hists(evsel)->stats.nr_samples;
> +
> + if (nr_samples == 0)
> continue;
>
> + ui_progress__init(&prog, nr_samples,
> + "Sorting group events for output...");
> + evsel__output_resort(pos, &prog);
> + ui_progress__finish();
> +
> hists__find_annotations(hists, pos, ann);
> }
>
> --
> 2.46.0.rc2.264.g509ed76dc8-goog
>