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@kernel.org>
---
 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)
+{
+	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;
+	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

> -	} 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");
> -		}

Shall we get rid of:

  perf_missing_features.exclude_guest
  pmu->missing_features.exclude_guest

they don't seem to be used anywhere after the patch.

Thanks,
Ravi

On Tue, Oct 15, 2024 at 09:49:52AM +0530, Ravi Bangoria wrote:
> > -	} 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");
> > -		}
> 
> Shall we get rid of:
> 
>   perf_missing_features.exclude_guest
>   pmu->missing_features.exclude_guest
> 
> they don't seem to be used anywhere after the patch.

Hmm.. it's my bad.  I think we should keep it for ancient kernels.

Thanks,
Namhyung

On Mon, Sep 30, 2024 at 5:20 PM Namhyung Kim <namhyung@kernel.org> 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@kernel.org>
> ---
>  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
>

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@kernel.org> 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@kernel.org>
> > ---
> >  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