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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZraG8ryV_qubOt8R@x1>
Date: Fri, 9 Aug 2024 18:15:30 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Ian Rogers <irogers@...gle.com>, Kan Liang <kan.liang@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
	linux-perf-users@...r.kernel.org,
	Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH] perf annotate: Fix --group behavior when leader has no
 samples

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@...hat.com>
> Tested-by: Arnaldo Carvalho de Melo <acme@...hat.com>
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
>  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
>