Re: [PATCH 7/8] perf tools: Add fallback for exclude_guest

[James Clark]

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@xxxxxxx>
> > Cc: James Clark <james.clark@xxxxxxxxxx>
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > ---
> >   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:

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",