linux-kernel - Re: [PATCH 7/8] perf tools: Add fallback for exclude

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM9d7cgAuYwh+zaEAa3v6V86=6bWgsDpN_LM75OM3JkJ=ZQvaQ@mail.gmail.com>
Date: Wed, 4 Sep 2024 08:52:24 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: James Clark <james.clark@...aro.org>
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>, 
	Athira Rajeev <atrajeev@...ux.vnet.ibm.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>, 
	Ian Rogers <irogers@...gle.com>, Kan Liang <kan.liang@...ux.intel.com>
Subject: Re: [PATCH 7/8] perf tools: Add fallback for exclude_guest

Hello James,

On Wed, Sep 4, 2024 at 6:36 AM James Clark <james.clark@...aro.org> wrote:
>
>
>
> On 04/09/2024 2:28 pm, James Clark wrote:
> >
> >
> > On 04/09/2024 7:41 am, Namhyung Kim wrote:
> >> It seems Apple M1 PMU requires exclude_guest set and returns EOPNOTSUPP
> >> if not.  Let's add a fallback so that it can work with default events.
> >>
> >> Cc: Mark Rutland <mark.rutland@....com>
> >> Cc: James Clark <james.clark@...aro.org>
> >> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> >> ---
> >>   tools/perf/util/evsel.c | 21 +++++++++++++++++++++
> >>   1 file changed, 21 insertions(+)
> >>
> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> >> index 0de0a72947db3f10..8c4d70f7b2f5b880 100644
> >> --- a/tools/perf/util/evsel.c
> >> +++ b/tools/perf/util/evsel.c
> >> @@ -3400,6 +3400,27 @@ bool evsel__fallback(struct evsel *evsel,
> >> struct target *target, int err,
> >>                 "to fall back to excluding hypervisor samples",
> >> paranoid);
> >>           evsel->core.attr.exclude_hv = 1;
> >> +        return true;
> >> +    } else if (err == EOPNOTSUPP && !evsel->core.attr.exclude_guest &&
> >> +           !evsel->exclude_GH) {
> >> +        const char *name = evsel__name(evsel);
> >> +        char *new_name;
> >> +        const char *sep = ":";
> >> +
> >> +        /* Is there already the separator in the name. */
> >> +        if (strchr(name, '/') ||
> >> +            (strchr(name, ':') && !evsel->is_libpfm_event))
> >> +            sep = "";
> >> +
> >> +        if (asprintf(&new_name, "%s%su", name, sep) < 0)
> >> +            return false;
> >> +
> >> +        free(evsel->name);
> >> +        evsel->name = new_name;
> >> +        /* Apple M1 requires exclude_guest */
> >> +        scnprintf(msg, msgsize, "trying to fall back to excluding
> >> guest samples");
> >> +        evsel->core.attr.exclude_guest = 1;
> >> +
> >>           return true;
> >>       }
> >
> > Not sure if this is working, for some reason it doesn't try the
> > fallback. With exclude guest made mandatory in the Arm PMU, then:
> >
>
> Looks like you change this, but it's not obvious why the stat behavior
> is different to perf record anyway:

Right, I think we should consolidate the open code to be used by
both perf record and perf stat.  I'll reorder my patchset to have
exclude_guest fallback first.

Anyway thanks for the test and the fix.  With this, perf stat and
perf record work well on your setup?

Thanks,
Namhyung

>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 8b9889873d3e..6f2ee3032f5f 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -639,7 +639,7 @@ static enum counter_recovery
> stat_handle_error(struct evsel *counter)
>           * (behavior changed with commit b0a873e).
>           */
>          if (errno == EINVAL || errno == ENOSYS ||
> -           errno == ENOENT || errno == EOPNOTSUPP ||
> +           errno == ENOENT ||
>              errno == ENXIO) {
>                  if (verbose > 0)
>                          ui__warning("%s event is not supported by the
> kernel.\n",
>
>