Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 20 Apr 2021 19:30:43 +0200
From:   Jiri Olsa <jolsa@redhat.com>
To:     Song Liu <songliubraving@fb.com>
Cc:     Namhyung Kim <namhyung@kernel.org>, Song Liu <song@kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Kernel Team <Kernel-team@fb.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        Jiri Olsa <jolsa@kernel.org>
Subject: Re: [PATCH v3 3/4] perf-stat: introduce config
 stat.bpf-counter-events
Message-ID: <YH8Pw4m0w6DuuEXo@krava>
References: <20210416221325.2373497-1-song@kernel.org>
 <20210416221325.2373497-4-song@kernel.org>
 <CAM9d7ciVj+d=Bgqmwu+v9mA1CGTbtr8pfXzgNKJs0Nh3BJtt8A@mail.gmail.com>
 <71CC9E29-ECBD-47DB-AE85-1477BF54C45D@fb.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <71CC9E29-ECBD-47DB-AE85-1477BF54C45D@fb.com>
Precedence: bulk

On Mon, Apr 19, 2021 at 08:26:02PM +0000, Song Liu wrote:
> 
> 
> > 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?

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
> 
> [...]
>