Re: [PATCH] perf stat: Create '--add-default' option to append default list

["Jin, Yao" <yao.jin@linux.intel.com>]

On 12/23/2020 8:56 AM, Jin, Yao wrote:
> Hi Arnaldo,
> 
> On 12/23/2020 12:15 AM, Arnaldo Carvalho de Melo wrote:
>> Em Tue, Dec 22, 2020 at 09:11:31AM +0800, Jin Yao escreveu:
>>> The event default list includes the most common events which are widely
>>> used by users. But with -e option, the current perf only counts the events
>>> assigned by -e option. Users may want to collect some extra events with
>>> the default list. For this case, users have to manually add all the events
>>> from the default list. It's inconvenient. Also, users may don't know how to
>>> get the default list.
>>>
>>> It's better to add a new option to append default list to the -e events.
>>> The new option is '--add-default'.
>>>
>>> Before:
>>>
>>> root@kbl-ppc:~# ./perf stat -e power/energy-pkg/ -a -- sleep 1
>>>
>>>   Performance counter stats for 'system wide':
>>>
>>>                2.05 Joules power/energy-pkg/
>>>
>>>         1.000857974 seconds time elapsed
>>>
>>> After:
>>>
>>> root@kbl-ppc:~# ./perf stat -e power/energy-pkg/ -a --add-default -- sleep 1
>>
>> I thought about:
>>
>>      perf stat -e +power/energy-pkg/ -a -- sleep 1
>>
> 
> I was surprised to see that '+<event>' syntax had been supported.
> 
> root@kbl-ppc:~# ./perf stat -e +power/energy-pkg/ -a -- sleep 1
> 
>   Performance counter stats for 'system wide':
> 
>                1.99 Joules +power/energy-pkg/
> 
>         1.000877852 seconds time elapsed
> 
> root@kbl-ppc:~# ./perf stat -e +power/energy-pkg/,+cycles -a -- sleep 1
> 
>   Performance counter stats for 'system wide':
> 
>                2.00 Joules +power/energy-pkg/
>          13,780,620        +cycles
> 
>         1.001639147 seconds time elapsed
> 
> Are there any scripts or usages need the prefix '+' before event? I don't know. But if we append the 
> '+<event>' to the default event list, will break something potentially?
> 
>> Which would have its counterpart:
>>
>>      perf stat -e -cycles -0a --sleep 1
>>
>> To remove an event from the defaults, perhaps to deal with some specific
>> hardware where the default or what is in -d, -dd, -ddd, etc can't all be
>> counted. I.e. - and + would remove or add from whaver list was there at
>> that point.
>>
>> - Arnaldo
> 
> Yes, + and - are more flexible solution. Just for above question, will '+<event>' break existing 
> usage? And for '-', I don't know if user can remember clearly for what the events are in default list.
> 

For '-', another difficulty is it may conflict with the hardware cache event.

Say we remove the "-                      { return '-'; }" from parse-events.l, such as:

diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 9db5097317f4..145653d1ce16 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -387,7 +387,6 @@ r{num_raw_hex}              { return raw(yyscanner); }
  {name}                 { return pmu_str_check(yyscanner, _parse_state); }
  {name_tag}             { return str(yyscanner, PE_NAME); }
  "/"                    { BEGIN(config); return '/'; }
--                      { return '-'; }
  ,                      { BEGIN(event); return ','; }
  :                      { return ':'; }
  "{"                    { BEGIN(event); return '{'; }

The syntax of '-<event>' is supported.

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

  Performance counter stats for 'system wide':

         14,008,859      -cycles

        1.001471494 seconds time elapsed

But the parsing of hardware cache event would be failed. :(

root@kbl-ppc:~# ./perf stat -e LLC-stores -a -- sleep 1
event syntax error: 'LLC-stores'
                         \___ parser error

That complicates things. :(

Thanks
Jin Yao

> Thanks
> Jin Yao
> 
>>>   Performance counter stats for 'system wide':
>>>
>>>                2.10 Joules power/energy-pkg/         #    0.000 K/sec
>>>            8,009.89 msec   cpu-clock                 #    7.995 CPUs utilized
>>>                 140        context-switches          #    0.017 K/sec
>>>                   9        cpu-migrations            #    0.001 K/sec
>>>                  66        page-faults               #    0.008 K/sec
>>>          10,671,929        cycles                    #    0.001 GHz
>>>           4,736,880        instructions              #    0.44  insn per cycle
>>>             942,951        branches                  #    0.118 M/sec
>>>              76,096        branch-misses             #    8.07% of all branches
>>>
>>>         1.001809960 seconds time elapsed
>>>
>>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>>> ---
>>>   tools/perf/Documentation/perf-stat.txt | 5 +++++
>>>   tools/perf/builtin-stat.c              | 4 +++-
>>>   tools/perf/util/stat.h                 | 1 +
>>>   3 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
>>> index 5d4a673d7621..75a83c2e4dc5 100644
>>> --- a/tools/perf/Documentation/perf-stat.txt
>>> +++ b/tools/perf/Documentation/perf-stat.txt
>>> @@ -438,6 +438,11 @@ convenient for post processing.
>>>   --summary::
>>>   Print summary for interval mode (-I).
>>> +--add-default::
>>> +The default event list includes the most common events which are widely
>>> +used by users. But with -e option, the perf only counts the events assigned
>>> +by -e option. This options appends the default event list to the -e events.
>>> +
>>>   EXAMPLES
>>>   --------
>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>>> index 89c32692f40c..6ac7b946f9a7 100644
>>> --- a/tools/perf/builtin-stat.c
>>> +++ b/tools/perf/builtin-stat.c
>>> @@ -1173,6 +1173,8 @@ static struct option stat_options[] = {
>>>                  "print summary for interval mode"),
>>>       OPT_BOOLEAN(0, "quiet", &stat_config.quiet,
>>>               "don't print output (useful with record)"),
>>> +    OPT_BOOLEAN(0, "add-default", &stat_config.add_default,
>>> +               "add default events"),
>>>   #ifdef HAVE_LIBPFM
>>>       OPT_CALLBACK(0, "pfm-events", &evsel_list, "event",
>>>           "libpfm4 event selector. use 'perf list' to list available events",
>>> @@ -1755,7 +1757,7 @@ static int add_default_attributes(void)
>>>           free(str);
>>>       }
>>> -    if (!evsel_list->core.nr_entries) {
>>> +    if (!evsel_list->core.nr_entries || stat_config.add_default) {
>>>           if (target__has_cpu(&target))
>>>               default_attrs0[0].config = PERF_COUNT_SW_CPU_CLOCK;
>>> diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
>>> index 9979b4b100f2..6ccc6936348c 100644
>>> --- a/tools/perf/util/stat.h
>>> +++ b/tools/perf/util/stat.h
>>> @@ -123,6 +123,7 @@ struct perf_stat_config {
>>>       bool             metric_no_merge;
>>>       bool             stop_read_counter;
>>>       bool             quiet;
>>> +    bool             add_default;
>>>       FILE            *output;
>>>       unsigned int         interval;
>>>       unsigned int         timeout;
>>> -- 
>>> 2.17.1
>>>
>>