Re: [PATCHv3] perf stat: Ensure group is defined on top of the same cpu mask

[Jiri Olsa]

On Tue, Jun 02, 2020 at 10:42:56AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jun 02, 2020 at 12:17:36PM +0200, Jiri Olsa escreveu:
> > Jin Yao reported the issue (and posted first versions of this change)
> > with groups being defined over events with different cpu mask.
>  
> > This causes assert aborts in get_group_fd, like:
>  
> >   # perf stat -M "C2_Pkg_Residency" -a -- sleep 1
> >   perf: util/evsel.c:1464: get_group_fd: Assertion `!(fd == -1)' failed.
> >   Aborted
>  
> > All the events in the group have to be defined over the same
> > cpus so the group_fd can be found for every leader/member pair.
>  
> > Adding check to ensure this condition is met and removing the
> > group (with warning) if we detect mixed cpus, like:
>  
> >   $ sudo perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}'
> >   WARNING: event cpu maps do not match, disabling group:
> >     anon group { power/energy-cores/, cycles }
> >     anon group { instructions, power/energy-cores/ }
> 
> So it doesn't disable the 'group', it disables the 'grouping' of those
> events, right? I.e. reading the WARNING, I thought that it would count
> nothing, since it lists both groups as being disabled, but when I tested
> I noticed that:
> 
>   [root@seventh ~]# perf stat -e '{power/energy-cores/,cycles},{instructions,power/energy-cores/}'
>   WARNING: grouped events cpus do not match, disabling group:
>     anon group { power/energy-cores/, cycles }
>     anon group { instructions, power/energy-cores/ }
>   ^C
>    Performance counter stats for 'system wide':
>   
>                12.62 Joules power/energy-cores/
>          106,920,637        cycles
>           80,228,899        instructions              #    0.75  insn per cycle
>                12.62 Joules power/energy-cores/
>   
>         14.514476987 seconds time elapsed
>   
>   
>   [root@seventh ~]#
> 
>   I.e. it counted the events, ungrouped, or am I missing something?

right, it disables 'grouping', events are scheduled/counted individualy

this way we will not hit the issue when looking for group_fd FD
and there's not any, because of different cpu maps

> 
> If I do:
> 
>   [root@seventh ~]# perf stat -e '{power/energy-cores/,power/energy-ram/},{instructions,cycles}' -a sleep 2
>   
>    Performance counter stats for 'system wide':
>   
>                 1.73 Joules power/energy-cores/                                         
>                 0.92 Joules power/energy-ram/                                           
>           12,191,658        instructions              #    0.67  insn per cycle         
>           18,275,233        cycles                                                      
>   
>          2.001272492 seconds time elapsed
>   
>   [root@seventh ~]# 
> 
> It works, grouped. One observation, shouldn't we somehow show in the
> output that the first two were indeed grouped, ditto for the second two?

yea, we don't display groups in output.. also there's no number
for the group, it's still separate events numbers in output
grouping is only used when creating events

> 
> Also, this needs improvement:
> 
>   [root@seventh ~]# perf stat -e '{power/energy-cores/,power/energy-ram/},{instructions,cycles}' sleep 2
>   Error:
>   The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (power/energy-cores/).
>   /bin/dmesg | grep -i perf may provide additional information.

yes, power events don't work with events without cpu being defined,
which is what we do for 'workload' session.. we should either check
for that and display some sensible error for power events

or perhaps check if we could monitor like perf record does with creating
events for task and every cpu in the system

thanks,
jirka