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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <71CC9E29-ECBD-47DB-AE85-1477BF54C45D@fb.com>
Date:   Mon, 19 Apr 2021 20:26:02 +0000
From:   Song Liu <songliubraving@...com>
To:     Namhyung Kim <namhyung@...nel.org>
CC:     Song Liu <song@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Jiri Olsa <jolsa@...nel.org>
Subject: Re: [PATCH v3 3/4] perf-stat: introduce config
 stat.bpf-counter-events



> On Apr 17, 2021, at 9:45 AM, Namhyung Kim <namhyung@...nel.org> wrote:
> 
> Hi Song,
> 
> On Sat, Apr 17, 2021 at 7:13 AM Song Liu <song@...nel.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@...nel.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

[...]