Re: [PATCH v3 3/4] perf-stat: introduce config stat.bpf-counter-events

[Song Liu <songliubraving@fb.com>]

> On Apr 17, 2021, at 9:45 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> 
> Hi Song,
> 
> On Sat, Apr 17, 2021 at 7:13 AM Song Liu <song@kernel.org> wrote:
>> 
>> Currently, to use BPF to aggregate perf event counters, the user uses
>> --bpf-counters option. Enable "use bpf by default" events with a config
>> option, stat.bpf-counter-events. Events with name in the option will use
>> BPF.
>> 
>> This also enables mixed BPF event and regular event in the same sesssion.
>> For example:
>> 
>>   perf config stat.bpf-counter-events=instructions
>>   perf stat -e instructions,cs
>> 
>> The second command will use BPF for "instructions" but not "cs".
>> 
>> Signed-off-by: Song Liu <song@kernel.org>
>> ---
>> @@ -535,12 +549,13 @@ static int enable_counters(void)
>>        struct evsel *evsel;
>>        int err;
>> 
>> -       if (target__has_bpf(&target)) {
>> -               evlist__for_each_entry(evsel_list, evsel) {
>> -                       err = bpf_counter__enable(evsel);
>> -                       if (err)
>> -                               return err;
>> -               }
>> +       evlist__for_each_entry(evsel_list, evsel) {
>> +               if (!evsel__is_bpf(evsel))
>> +                       continue;
>> +
>> +               err = bpf_counter__enable(evsel);
>> +               if (err)
>> +                       return err;
> 
> I just realized it doesn't have a disable counterpart.

I guess it is not really necessary for perf-stat? It is probably good
to have it though. How about we do it in a follow up patch?

[...]

>> +       if (!evsel__bpf_counter_events)
>> +               return false;
>> +
>> +       ptr = strstr(evsel__bpf_counter_events, name);
>> +       name_len = strlen(name);
>> +
>> +       /* check name matches a full token in evsel__bpf_counter_events */
>> +       match = (ptr != NULL) &&
>> +               ((ptr == evsel__bpf_counter_events) || (*(ptr - 1) == ',')) &&
>> +               ((*(ptr + name_len) == ',') || (*(ptr + name_len) == '\0'));
> 
> I'm not sure we have an event name which is a substring of another.
> Maybe it can retry if it fails to match.

We have ref-cycles and cycles. And some raw events may be substring of others?

Thanks,
Song

[...]