Re: [PATCH 5/5] perf annotate: Add --skip-empty option

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Mon, Aug 05, 2024 at 01:14:27PM -0700, Namhyung Kim wrote:
> On Mon, Aug 05, 2024 at 04:22:12PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Sat, Aug 03, 2024 at 02:13:32PM -0700, Namhyung Kim wrote:
> > > Like in perf report, we want to hide empty events in the perf annotate
> > > output.  This is consistent when the option is set in perf report.
> > > 
> > > For example, the following command would use 3 events including dummy.
> > > 
> > >   $ perf mem record -a -- perf test -w noploop
> > > 
> > >   $ perf evlist
> > >   cpu/mem-loads,ldlat=30/P
> > >   cpu/mem-stores/P
> > >   dummy:u
> > > 
> > > Just using perf annotate with --group will show the all 3 events.
> > 
> > Seems unrelated, just before compiling with this patch:
> > 
> > root@x1:~# perf mem record -a -- perf test -w noploop
> > Memory events are enabled on a subset of CPUs: 4-11
> > [ perf record: Woken up 1 times to write data ]
> > [ perf record: Captured and wrote 10.506 MB perf.data (2775 samples) ]
> > root@x1:~#
> > 
> > root@x1:~# perf annotate --group --stdio2 sched_clock
> > root@x1:~# perf annotate --stdio2 sched_clock
> > Samples: 178  of event 'cpu_atom/mem-stores/P', 4000 Hz, Event count (approx.): 565268, [percent: local period]
> > sched_clock() /usr/lib/debug/lib/modules/6.8.11-200.fc39.x86_64/vmlinux
> > Percent      0xffffffff810511e0 <sched_clock>:
> >                endbr64        
> >    5.76        incl    pcpu_hot+0x8
> >    5.47      → callq   sched_clock_noinstr
> >   88.78        decl    pcpu_hot+0x8
> >              ↓ je      1e     
> >              → jmp     __x86_return_thunk
> >          1e: → callq   __SCT__preempt_schedule_notrace
> >              → jmp     __x86_return_thunk
> > root@x1:~# perf annotate --group --stdio2 sched_clock
> > root@x1:~# perf annotate --group --stdio sched_clock
> > root@x1:~# perf annotate --group sched_clock
> > root@x1:~#
> > 
> > root@x1:~# perf evlist
> > cpu_atom/mem-loads,ldlat=30/P
> > cpu_atom/mem-stores/P
> > dummy:u
> > root@x1:~#
> > 
> > root@x1:~# perf report --header-only | grep cmdline
> > # cmdline : /home/acme/bin/perf mem record -a -- perf test -w noploop 
> > root@x1:~#
> > 
> > I thought it would be some hybrid oddity but seems to be just --group
> > related, seems like it stops if the first event has no samples? Because
> > it works with another symbol:
> 
> Good catch.  Yeah I found it only checked the first event.  Something
> like below should fix the issue.

Nope, with the patch applied:

root@x1:~# perf annotate --group --stdio sched_clock
root@x1:~# perf annotate --stdio sched_clock
 Percent |      Source code & Disassembly of vmlinux for cpu_atom/mem-stores/P (147 samples, percent: local period)
-------------------------------------------------------------------------------------------------------------------
         : 0                0xffffffff810511e0 <sched_clock>:
    0.00 :   ffffffff810511e0:       endbr64
    5.11 :   ffffffff810511e4:       incl    %gs:0x7efe2d5d(%rip)       # 33f48 <pcpu_hot+0x8>
    0.13 :   ffffffff810511eb:       callq   0xffffffff821350d0
   94.76 :   ffffffff810511f0:       decl    %gs:0x7efe2d51(%rip)       # 33f48 <pcpu_hot+0x8>
    0.00 :   ffffffff810511f7:       je      0xffffffff810511fe
    0.00 :   ffffffff810511f9:       jmp     0xffffffff82153320
    0.00 :   ffffffff810511fe:       callq   0xffffffff82153990
    0.00 :   ffffffff81051203:       jmp     0xffffffff82153320
root@x1:~# perf annotate --group --stdio sched_clock
root@x1:~# perf annotate --group --stdio2 sched_clock
root@x1:~# perf annotate --group sched_clock
root@x1:~#
 
> Thanks,
> Namhyung
> 
> 
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index efcadb7620b8..8d3ec439b783 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -632,11 +632,15 @@ 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 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;
>  
>  		hists__find_annotations(hists, pos, ann);