Re: [PATCH v4 3/9] perf record --off-cpu: Parse offcpu-time event

[Howard Chu]

On Thu, Aug 8, 2024 at 7:25 AM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Wed, Aug 07, 2024 at 11:38:37PM +0800, Howard Chu wrote:
> > Parse offcpu-time event using parse_event, in off_cpu_start(), write
> > evlist fds got from evlist__open() to perf_event_array BPF map.
> >
> > Signed-off-by: Howard Chu <howardchu95@xxxxxxxxx>
> > ---
> >  tools/perf/util/bpf_off_cpu.c | 55 ++++++++++++++++++++---------------
> >  tools/perf/util/evsel.c       |  2 +-
> >  2 files changed, 32 insertions(+), 25 deletions(-)
> >
> > diff --git a/tools/perf/util/bpf_off_cpu.c b/tools/perf/util/bpf_off_cpu.c
> > index 1e0e454bfb5e..fae0bb8aaa13 100644
> > --- a/tools/perf/util/bpf_off_cpu.c
> > +++ b/tools/perf/util/bpf_off_cpu.c
> > @@ -13,6 +13,7 @@
> >  #include "util/cgroup.h"
> >  #include "util/strlist.h"
> >  #include <bpf/bpf.h>
> > +#include <internal/xyarray.h>
> >
> >  #include "bpf_skel/off_cpu.skel.h"
> >
> > @@ -38,39 +39,24 @@ union off_cpu_data {
> >
> >  static int off_cpu_config(struct evlist *evlist)
> >  {
> > -     struct evsel *evsel;
> > -     struct perf_event_attr attr = {
> > -             .type   = PERF_TYPE_SOFTWARE,
> > -             .config = PERF_COUNT_SW_BPF_OUTPUT,
> > -             .size   = sizeof(attr), /* to capture ABI version */
> > -     };
> > -     char *evname = strdup(OFFCPU_EVENT);
> > +     char off_cpu_event[64];
> >
> > -     if (evname == NULL)
> > -             return -ENOMEM;
> > -
> > -     evsel = evsel__new(&attr);
> > -     if (!evsel) {
> > -             free(evname);
> > -             return -ENOMEM;
> > +     /* after parsing off-cpu event, we'll specify its sample_type in evsel__config() */
> > +     scnprintf(off_cpu_event, sizeof(off_cpu_event), "bpf-output/no-inherit=1,name=%s/", OFFCPU_EVENT);
> > +     if (parse_event(evlist, off_cpu_event)) {
> > +             pr_err("Failed to open off-cpu event\n");
> > +             return -1;
> >       }
> >
> > -     evsel->core.attr.freq = 1;
> > -     evsel->core.attr.sample_period = 1;
> > -     /* off-cpu analysis depends on stack trace */
> > -     evsel->core.attr.sample_type = PERF_SAMPLE_CALLCHAIN;
> > -
> > -     evlist__add(evlist, evsel);
> > -
> > -     free(evsel->name);
> > -     evsel->name = evname;
> > -
> >       return 0;
> >  }
> >
> >  static void off_cpu_start(void *arg)
> >  {
> >       struct evlist *evlist = arg;
> > +     struct evsel *evsel;
> > +     struct perf_cpu pcpu;
> > +     int i, err;
> >
> >       /* update task filter for the given workload */
> >       if (!skel->bss->has_cpu && !skel->bss->has_task &&
> > @@ -86,6 +72,27 @@ static void off_cpu_start(void *arg)
> >               bpf_map_update_elem(fd, &pid, &val, BPF_ANY);
> >       }
> >
> > +     /* sample id and fds in BPF's perf_event_array can only be set after record__open() */
> > +     evsel = evlist__find_evsel_by_str(evlist, OFFCPU_EVENT);
> > +     if (evsel == NULL) {
> > +             pr_err("%s evsel not found\n", OFFCPU_EVENT);
> > +             return;
> > +     }
> > +
> > +     if (evsel->core.id)
> > +             skel->bss->sample_id = evsel->core.id[0];
> > +
> > +     perf_cpu_map__for_each_cpu(pcpu, i, evsel->core.cpus) {
> > +             err = bpf_map__update_elem(skel->maps.offcpu_output,
> > +                                        &pcpu.cpu, sizeof(__u32),
> > +                                        xyarray__entry(evsel->core.fd, pcpu.cpu, 0),
> > +                                        sizeof(__u32), BPF_ANY);
> > +             if (err) {
> > +                     pr_err("Failed to update perf event map for direct off-cpu dumping\n");
> > +                     return;
> > +             }
> > +     }
> > +
> >       skel->bss->enabled = 1;
> >  }
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index b961467133cf..ccd3bda02b5d 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -1379,7 +1379,7 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
> >               evsel__reset_sample_bit(evsel, BRANCH_STACK);
> >
> >       if (evsel__is_offcpu_event(evsel))
> > -             evsel->core.attr.sample_type &= OFFCPU_SAMPLE_TYPES;
> > +             evsel->core.attr.sample_type = OFFCPU_SAMPLE_TYPES;
>
> I don't think we need this.  It should check what you requested.
> IOW you don't need to put cgroup info when user didn't ask.

I misunderstood the purpose of this check, so I'll leave it as it was.

Thanks,
Howard
>
> Thanks,
> Namhyung
>
> >
> >       arch__post_evsel_config(evsel, attr);
> >  }
> > --
> > 2.45.2
> >