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

Song Liu
2021-04-19T20:26:13Z

> 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 …