Re: [PATCH v2] perf stat: Append to default list if use -e +event

Jin, Yao
2021-01-21T06:12:32Z

Hi Arnaldo,

On 1/21/2021 8:03 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jan 20, 2021 at 10:25:53PM +0100, Jiri Olsa escreveu:
>> On Mon, Jan 18, 2021 at 12:54:37PM +0800, Jin, Yao wrote:
>>> root@kbl-ppc:# ./perf stat -e +power/energy-pkg/ -a -- sleep 1
>
>>> Performance counter stats for 'system wide':
>
>>> 2.02 Joules +power/energy-pkg/
>
>>> 1.000859434 seconds time elapsed
>
>>> The '+' prefix is printed. So I finally decide not to remove the '+' prefix
>>> in order to keep original behavior.
>
>> hm, originaly there's no purpose for the '+', right?
>> it seems it's more like bug then anything else
>
>> you added function to the '+' to add default events to specified event,
>> which I think is good idea, but I don't think we should display the
>> extra '+' in output
>
> The value would be to stress that that is an event added to the ones
> without the + prefix, i.e. the default ones.
>
> But by having the command line copied over and the added events at the
> first lines we should have that abundantly clear.
>
> Also we won't print removed events (using -), is that available already?
>

Sorry, the '-' support is not available in this patch. Can I do the patch for '+' first and then do
a follow up patch for '-' at next step?

> Nope:
>
> [root@quaco ~]# perf stat -e -cycles sleep
> event syntax error: '-cycles'
> \___ parser error
> Run 'perf list' for a list of valid events
>
> Usage: perf stat [<options>] [<command>]
>
> -e, --event <event> event selector. use 'perf list' to list available events
> [root@quaco ~]#
>
> Like with:
>
> [root@quaco ~]# perf stat -d sleep 1
> > Performance counter stats for 'sleep 1':
>
> 0.80 msec task-clock # 0.001 CPUs utilized
> 1 context-switches # 0.001 M/sec
> 0 cpu-migrations # 0.000 K/sec
> 59 page-faults # 0.073 M/sec
> 2,215,522 cycles # 2.757 GHz (7.40%)
> 1,073,189 instructions # 0.48 insn per cycle (80.59%)
> 203,615 branches # 253.392 M/sec
> 8,309 branch-misses # 4.08% of all branches
> 245,866 L1-dcache-loads # 305.972 M/sec
> 14,024 L1-dcache-load-misses # 5.70% of all L1-dcache accesses (92.60%)
> <not counted> LLC-loads (0.00%)
> <not counted> LLC-load-misses (0.00%)
>
> 1.001887261 seconds time elapsed
>
> 0.000000000 seconds user
> 0.001413000 seconds sys
>
>
> Some events weren't counted. Try disabling the NMI watchdog:
> echo 0 > /proc/sys/kernel/nmi_watchdog
> perf stat ...
> echo 1 > /proc/sys/kernel/nmi_watchdog
> [root@quaco ~]# perf stat -e -LLC* -d sleep 1
> event syntax error: '-LLC*'
> \___ parser error
> Run 'perf list' for a list of valid events
>
> Usage: perf stat [<options>] [<command>]
>
> -e, --event <event> event selector. use 'perf list' to list available events
> [root@quaco ~]#
>
> - Arnaldo
>

So if we just want to append the default list, we only need to set detailed_run=1, then ideally
perf-stat will print the default list.

But for now, there are no task-clock, context-switches, cpu-migrations, page-faults, instructions,
branches and branch-misses displayed.

root@kbl-ppc:~# ./perf stat -e cycles -d -a -- sleep 1

Performance counter stats for 'system wide':

124,178,207 cycles (80.02%)
6,444,490 L1-dcache-loads (80.01%)
1,043,169 L1-dcache-load-misses # 16.19% of all L1-dcache accesses (80.02%)
564,474 LLC-loads (80.02%)
49,262 LLC-load-misses # 8.73% of all LL-cache accesses (79.92%)

1.001614947 seconds time elapsed

Do we still need the '+' prefix to add the specified event on top of default list? It looks current
syntax should already support that feature, but just need to fix some issues.

Thanks
Jin Yao