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

[Ian Rogers]

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.

> +{
> +       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.

Thanks,
Ian

> +       pr_debug2("switching off branch counters support\n");
> +
> +       attr.branch_sample_type = PERF_SAMPLE_BRANCH_HW_INDEX;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.branch_hw_idx = true;
> +       pr_debug2("switching off branch HW index support\n");
> +
> +       attr.branch_sample_type = PERF_SAMPLE_BRANCH_NO_CYCLES | PERF_SAMPLE_BRANCH_NO_FLAGS;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.lbr_flags = true;
> +       pr_debug2_peo("switching off branch sample type no (cycles/flags)\n");
> +
> +found:
> +       detection_done = true;
> +       errno = old_errno;
> +}
> +
> +static bool evsel__detect_missing_features(struct evsel *evsel)
> +{
> +       static bool detection_done = false;
> +       struct perf_event_attr attr = {
> +               .type = PERF_TYPE_SOFTWARE,
> +               .config = PERF_COUNT_SW_TASK_CLOCK,
> +               .disabled = 1,
> +       };
> +       int old_errno;
> +
> +       if (evsel__has_br_stack(evsel))
> +               evsel__detect_missing_brstack_features(evsel);
> +
> +       if (detection_done)
> +               goto check;
> +
> +       old_errno = errno;
> +
> +       /*
> +        * Must probe features in the order they were added to the
> +        * perf_event_attr interface.  These are kernel core limitation
> +        * not PMU-specific so we can detect with a software event and
> +        * stop on the first one succeeded.
> +        */
> +
> +       /* Please add new feature detection here. */
> +
> +       attr.read_format = PERF_FORMAT_LOST;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.read_lost = true;
> +       pr_debug2("switching off PERF_FORMAT_LOST support\n");
> +       attr.read_format = 0;
> +
> +       attr.sample_type = PERF_SAMPLE_WEIGHT_STRUCT;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.weight_struct = true;
> +       pr_debug2("switching off weight struct support\n");
> +       attr.sample_type = 0;
> +
> +       attr.sample_type = PERF_SAMPLE_CODE_PAGE_SIZE;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.code_page_size = true;
> +       pr_debug2_peo("Kernel has no PERF_SAMPLE_CODE_PAGE_SIZE support\n");
> +       attr.sample_type = 0;
> +
> +       attr.sample_type = PERF_SAMPLE_DATA_PAGE_SIZE;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.data_page_size = true;
> +       pr_debug2_peo("Kernel has no PERF_SAMPLE_DATA_PAGE_SIZE support\n");
> +       attr.sample_type = 0;
> +
> +       attr.cgroup = 1;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.cgroup = true;
> +       pr_debug2_peo("Kernel has no cgroup sampling support\n");
> +       attr.cgroup = 0;
> +
> +       attr.aux_output = 1;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.aux_output = true;
> +       pr_debug2_peo("Kernel has no attr.aux_output support\n");
> +       attr.aux_output = 0;
> +
> +       attr.bpf_event = 1;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.bpf = true;
> +       pr_debug2_peo("switching off bpf_event\n");
> +       attr.bpf_event = 0;
> +
> +       attr.ksymbol = 1;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.ksymbol = true;
> +       pr_debug2_peo("switching off ksymbol\n");
> +       attr.ksymbol = 0;
> +
> +       attr.write_backward = 1;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.write_backward = true;
> +       pr_debug2_peo("switching off write_backward\n");
> +       attr.write_backward = 0;
> +
> +       attr.use_clockid = 1;
> +       attr.clockid = CLOCK_MONOTONIC;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.clockid = true;
> +       pr_debug2_peo("switching off clockid\n");
> +       attr.use_clockid = 0;
> +       attr.clockid = 0;
> +
> +       if (has_attr_feature(&attr, /*flags=*/PERF_FLAG_FD_CLOEXEC))
> +               goto found;
> +       perf_missing_features.cloexec = true;
> +       pr_debug2_peo("switching off cloexec flag\n");
> +
> +       attr.mmap2 = 1;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.mmap2 = true;
> +       pr_debug2_peo("switching off mmap2\n");
> +       attr.mmap2 = 0;
> +
> +       /* set this unconditionally? */
> +       perf_missing_features.sample_id_all = true;
> +       pr_debug2_peo("switching off sample_id_all\n");
> +
> +       attr.inherit = 1;
> +       attr.read_format = PERF_FORMAT_GROUP;
> +       if (has_attr_feature(&attr, /*flags=*/0))
> +               goto found;
> +       perf_missing_features.group_read = true;
> +       pr_debug2_peo("switching off group read\n");
> +       attr.inherit = 0;
> +       attr.read_format = 0;
> +
> +found:
> +       detection_done = true;
> +       errno = old_errno;
> +
> +check:
> +       if ((evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_COUNTERS) &&
> +           perf_missing_features.branch_counters)
>                 return true;
> -       } else if (!perf_missing_features.weight_struct &&
> -           (evsel->core.attr.sample_type & PERF_SAMPLE_WEIGHT_STRUCT)) {
> -               perf_missing_features.weight_struct = true;
> -               pr_debug2("switching off weight struct support\n");
> +
> +       if ((evsel->core.attr.read_format & PERF_FORMAT_LOST) &&
> +           perf_missing_features.read_lost)
>                 return true;
> -       } else if (!perf_missing_features.code_page_size &&
> -           (evsel->core.attr.sample_type & PERF_SAMPLE_CODE_PAGE_SIZE)) {
> -               perf_missing_features.code_page_size = true;
> -               pr_debug2_peo("Kernel has no PERF_SAMPLE_CODE_PAGE_SIZE support, bailing out\n");
> -               return false;
> -       } else if (!perf_missing_features.data_page_size &&
> -           (evsel->core.attr.sample_type & PERF_SAMPLE_DATA_PAGE_SIZE)) {
> -               perf_missing_features.data_page_size = true;
> -               pr_debug2_peo("Kernel has no PERF_SAMPLE_DATA_PAGE_SIZE support, bailing out\n");
> -               return false;
> -       } else if (!perf_missing_features.cgroup && evsel->core.attr.cgroup) {
> -               perf_missing_features.cgroup = true;
> -               pr_debug2_peo("Kernel has no cgroup sampling support, bailing out\n");
> -               return false;
> -       } else if (!perf_missing_features.branch_hw_idx &&
> -           (evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_HW_INDEX)) {
> -               perf_missing_features.branch_hw_idx = true;
> -               pr_debug2("switching off branch HW index support\n");
> +
> +       if ((evsel->core.attr.sample_type & PERF_SAMPLE_WEIGHT_STRUCT) &&
> +           perf_missing_features.weight_struct)
>                 return true;
> -       } else if (!perf_missing_features.aux_output && evsel->core.attr.aux_output) {
> -               perf_missing_features.aux_output = true;
> -               pr_debug2_peo("Kernel has no attr.aux_output support, bailing out\n");
> -               return false;
> -       } else if (!perf_missing_features.bpf && evsel->core.attr.bpf_event) {
> -               perf_missing_features.bpf = true;
> -               pr_debug2_peo("switching off bpf_event\n");
> +
> +       if (evsel->core.attr.use_clockid && evsel->core.attr.clockid != CLOCK_MONOTONIC &&
> +           !perf_missing_features.clockid) {
> +               perf_missing_features.clockid_wrong = true;
>                 return true;
> -       } else if (!perf_missing_features.ksymbol && evsel->core.attr.ksymbol) {
> -               perf_missing_features.ksymbol = true;
> -               pr_debug2_peo("switching off ksymbol\n");
> +       }
> +
> +       if (evsel->core.attr.use_clockid && perf_missing_features.clockid)
>                 return true;
> -       } else if (!perf_missing_features.write_backward && evsel->core.attr.write_backward) {
> -               perf_missing_features.write_backward = true;
> -               pr_debug2_peo("switching off write_backward\n");
> -               return false;
> -       } else if (!perf_missing_features.clockid_wrong && evsel->core.attr.use_clockid) {
> -               perf_missing_features.clockid_wrong = true;
> -               pr_debug2_peo("switching off clockid\n");
> +
> +       if ((evsel->open_flags & PERF_FLAG_FD_CLOEXEC) &&
> +           perf_missing_features.cloexec)
>                 return true;
> -       } else if (!perf_missing_features.clockid && evsel->core.attr.use_clockid) {
> -               perf_missing_features.clockid = true;
> -               pr_debug2_peo("switching off use_clockid\n");
> +
> +       if (evsel->core.attr.mmap2 && perf_missing_features.mmap2)
>                 return true;
> -       } else if (!perf_missing_features.cloexec && (evsel->open_flags & PERF_FLAG_FD_CLOEXEC)) {
> -               perf_missing_features.cloexec = true;
> -               pr_debug2_peo("switching off cloexec flag\n");
> +
> +       if ((evsel->core.attr.branch_sample_type & (PERF_SAMPLE_BRANCH_NO_FLAGS |
> +                                                   PERF_SAMPLE_BRANCH_NO_CYCLES)) &&
> +           perf_missing_features.lbr_flags)
>                 return true;
> -       } else if (!perf_missing_features.mmap2 && evsel->core.attr.mmap2) {
> -               perf_missing_features.mmap2 = true;
> -               pr_debug2_peo("switching off mmap2\n");
> +
> +       if (evsel->core.attr.inherit && (evsel->core.attr.read_format & PERF_FORMAT_GROUP) &&
> +           perf_missing_features.group_read)
>                 return true;
> -       } else if (evsel->core.attr.exclude_guest || evsel->core.attr.exclude_host) {
> -               if (evsel->pmu == NULL)
> -                       evsel->pmu = evsel__find_pmu(evsel);
> -
> -               if (evsel->pmu)
> -                       evsel->pmu->missing_features.exclude_guest = true;
> -               else {
> -                       /* we cannot find PMU, disable attrs now */
> -                       evsel->core.attr.exclude_host = false;
> -                       evsel->core.attr.exclude_guest = false;
> -               }
>
> -               if (evsel->exclude_GH) {
> -                       pr_debug2_peo("PMU has no exclude_host/guest support, bailing out\n");
> -                       return false;
> -               }
> -               if (!perf_missing_features.exclude_guest) {
> -                       perf_missing_features.exclude_guest = true;
> -                       pr_debug2_peo("switching off exclude_guest, exclude_host\n");
> -               }
> +       if (evsel->core.attr.ksymbol && perf_missing_features.ksymbol)
>                 return true;
> -       } else if (!perf_missing_features.sample_id_all) {
> -               perf_missing_features.sample_id_all = true;
> -               pr_debug2_peo("switching off sample_id_all\n");
> +
> +       if (evsel->core.attr.bpf_event && perf_missing_features.bpf)
>                 return true;
> -       } else if (!perf_missing_features.lbr_flags &&
> -                       (evsel->core.attr.branch_sample_type &
> -                        (PERF_SAMPLE_BRANCH_NO_CYCLES |
> -                         PERF_SAMPLE_BRANCH_NO_FLAGS))) {
> -               perf_missing_features.lbr_flags = true;
> -               pr_debug2_peo("switching off branch sample type no (cycles/flags)\n");
> +
> +       if ((evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_HW_INDEX) &&
> +           perf_missing_features.branch_hw_idx)
>                 return true;
> -       } else if (!perf_missing_features.group_read &&
> -                   evsel->core.attr.inherit &&
> -                  (evsel->core.attr.read_format & PERF_FORMAT_GROUP) &&
> -                  evsel__is_group_leader(evsel)) {
> -               perf_missing_features.group_read = true;
> -               pr_debug2_peo("switching off group read\n");
> +
> +       if (evsel->core.attr.sample_id_all && perf_missing_features.sample_id_all)
>                 return true;
> -       } else {
> -               return false;
> -       }
> +
> +       return false;
>  }
>
>  static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 3e751ea769ac4d3a..ea3140cd91c589fd 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -368,7 +368,6 @@ int evsel__open(struct evsel *evsel, struct perf_cpu_map *cpus,
>  void evsel__close(struct evsel *evsel);
>  int evsel__prepare_open(struct evsel *evsel, struct perf_cpu_map *cpus,
>                 struct perf_thread_map *threads);
> -bool evsel__detect_missing_features(struct evsel *evsel);
>
>  bool evsel__precise_ip_fallback(struct evsel *evsel);
>
> --
> 2.46.1.824.gd892dcdcdd-goog
>