Re: [PATCH 5/8] perf tools: Detect missing kernel features properly

[Namhyung Kim]

On Tue, Oct 01, 2024 at 10:53:02AM -0700, Ian Rogers wrote:
> On Mon, Sep 30, 2024 at 5:20 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> >
> > The evsel__detect_missing_features() is to check if the attributes of
> > the evsel is supported or not.  But it checks the attribute based on the
> > given evsel, it might miss something if the attr doesn't have the bit or
> > give incorrect results if the event is special.
> >
> > Also it maintains the order of the feature that was added to the kernel
> > which means it can assume older features should be supported once it
> > detects the current feature is working.  To minimized the confusion and
> > to accurately check the kernel features, I think it's better to use a
> > software event and go through all the features at once.
> >
> > Also make the function static since it's only used in evsel.c.
> >
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > ---
> >  tools/perf/util/evsel.c | 345 +++++++++++++++++++++++++++++-----------
> >  tools/perf/util/evsel.h |   1 -
> >  2 files changed, 249 insertions(+), 97 deletions(-)
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index f202d28147d62a44..32e30c293d0c6198 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/zalloc.h>
> >  #include <sys/ioctl.h>
> >  #include <sys/resource.h>
> > +#include <sys/syscall.h>
> >  #include <sys/types.h>
> >  #include <dirent.h>
> >  #include <stdlib.h>
> > @@ -2150,120 +2151,272 @@ int evsel__prepare_open(struct evsel *evsel, struct perf_cpu_map *cpus,
> >         return err;
> >  }
> >
> > -bool evsel__detect_missing_features(struct evsel *evsel)
> > +static bool has_attr_feature(struct perf_event_attr *attr, unsigned long flags)
> >  {
> > +       int fd = syscall(SYS_perf_event_open, attr, /*pid=*/0, /*cpu=*/-1,
> > +                        /*group_fd=*/-1, flags);
> > +       close(fd);
> > +
> > +       if (fd < 0) {
> > +               attr->exclude_kernel = 1;
> > +
> > +               fd = syscall(SYS_perf_event_open, attr, /*pid=*/0, /*cpu=*/-1,
> > +                            /*group_fd=*/-1, flags);
> > +               close(fd);
> > +       }
> > +
> > +       if (fd < 0) {
> > +               attr->exclude_hv = 1;
> > +
> > +               fd = syscall(SYS_perf_event_open, attr, /*pid=*/0, /*cpu=*/-1,
> > +                            /*group_fd=*/-1, flags);
> > +               close(fd);
> > +       }
> > +
> > +       if (fd < 0) {
> > +               attr->exclude_guest = 1;
> > +
> > +               fd = syscall(SYS_perf_event_open, attr, /*pid=*/0, /*cpu=*/-1,
> > +                            /*group_fd=*/-1, flags);
> > +               close(fd);
> > +       }
> > +
> > +       attr->exclude_kernel = 0;
> > +       attr->exclude_guest = 0;
> > +       attr->exclude_hv = 0;
> > +
> > +       return fd >= 0;
> > +}
> > +
> > +static void evsel__detect_missing_brstack_features(struct evsel *evsel)
> 
> In the future could other PMU specific unsupported features be added
> not just brstack? Perhaps evsel__detect_missing_pmu_features would
> better capture that.

Yep, sounds reasonable.  I think we can add that if we have another
thing to check.

> 
> > +{
> > +       static bool detection_done = false;
> > +       struct perf_event_attr attr = {
> > +               .type = evsel->core.attr.type,
> > +               .config = evsel->core.attr.config,
> > +               .disabled = 1,
> > +               .sample_type = PERF_SAMPLE_BRANCH_STACK,
> > +               .sample_period = 1000,
> > +       };
> > +       int old_errno;
> > +
> > +       if (detection_done)
> > +               return;
> > +
> > +       old_errno = errno;
> > +
> >         /*
> >          * Must probe features in the order they were added to the
> > -        * perf_event_attr interface.
> > +        * perf_event_attr interface.  These are PMU specific limitation
> > +        * so we can detect with the given hardware event and stop on the
> > +        * first one succeeded.
> >          */
> > -       if (!perf_missing_features.branch_counters &&
> > -           (evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_COUNTERS)) {
> > -               perf_missing_features.branch_counters = true;
> > -               pr_debug2("switching off branch counters support\n");
> > -               return true;
> > -       } else if (!perf_missing_features.read_lost &&
> > -           (evsel->core.attr.read_format & PERF_FORMAT_LOST)) {
> > -               perf_missing_features.read_lost = true;
> > -               pr_debug2("switching off PERF_FORMAT_LOST support\n");
> > +
> > +       /* Please add new feature detection here. */
> > +
> > +       attr.branch_sample_type = PERF_SAMPLE_BRANCH_COUNTERS;
> > +       if (has_attr_feature(&attr, /*flags=*/0))
> > +               goto found;
> > +       perf_missing_features.branch_counters = true;
> 
> It feels like these global variables should be part of the PMU state.
> There is already perf_pmu.missing_features.

You're right.  But I think this is kinda global feature unless we have
different PMUs that provide different branch sampling capability.  It's
just the feature test requires a specific PMU and event.  But it'd be
better putting this into struct perf_pmu later.

Kan, can you confirm if Intel hybrid systems have the same branch stack
sampling capabilities on both cores?

Thanks,
Namhyung