| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZvxqcEIVELw9Uets@google.com>
Date: Tue, 1 Oct 2024 14:32:32 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>, Kan Liang <kan.liang@...ux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
linux-perf-users@...r.kernel.org,
Ravi Bangoria <ravi.bangoria@....com>,
Mark Rutland <mark.rutland@....com>,
James Clark <james.clark@....com>, Kajol Jain <kjain@...ux.ibm.com>,
Thomas Richter <tmricht@...ux.ibm.com>,
Atish Patra <atishp@...shpatra.org>,
Palmer Dabbelt <palmer@...osinc.com>,
Mingwei Zhang <mizhang@...gle.com>
Subject: Re: [PATCH 5/8] perf tools: Detect missing kernel features properly
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@...nel.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@...nel.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
Powered by blists - more mailing lists