On Mon, Apr 19, 2021 at 08:26:02PM +0000, Song Liu wrote:
>
>
> > On Apr 17, 2021, at 9:45 AM, Namhyung Kim <namhy...@kernel.org> wrote:
> >
> > Hi Song,
> >
> > On Sat, Apr 17, 2021 at 7:13 AM Song Liu <s...@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 <s...@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?
good catch, should it at least do:
evsel->follower_skel->bss->enabled = 0;
because then the follower goes down only with perf process, right?
still doing the counts till the end..?
also while checking on that I realized we open the counters
in separate path for this in bperf_reload_leader_program by
calling evsel__open_per_cpu.. and we're missing all the
fallback code and setup from create_perf_stat_counter
I think we should at least call create_perf_stat_counter,
and also propagate error value if it fails
jirka
>
> [...]
>
> >> + 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
>
> [...]
>
