perf tools: Do not set attr.exclude_guest by default (v3)

[PATCH 01/10] perf tools: Add fallback for exclude_guest

Posted by Namhyung Kim 4 months, 1 week ago

Commit 7b100989b4f6bce70 ("perf evlist: Remove __evlist__add_default")
changed to parse "cycles:P" event instead of creating a new cycles
event for perf record.  But it also changed the way how modifiers are
handled so it doesn't set the exclude_guest bit by default.

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.

Fixes: 7b100989b4f6bce70 ("perf evlist: Remove __evlist__add_default")
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: James Clark <james.clark@linaro.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-stat.c |  3 +--
 tools/perf/util/evsel.c   | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index cf985cdb9a6ee588..d8315dae930184ba 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -639,8 +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 == ENXIO) {
+	    errno == ENOENT || errno == ENXIO) {
 		if (verbose > 0)
 			ui__warning("%s event is not supported by the kernel.\n",
 				    evsel__name(counter));
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 49cc71511c0c8ce8..d59ad76b28758906 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -3244,6 +3244,27 @@ bool evsel__fallback(struct evsel *evsel, struct target *target, int err,
 		evsel->core.attr.exclude_kernel = 1;
 		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%sH", 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;
 	}
 
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 01/10] perf tools: Add fallback for exclude_guest

Posted by Liang, Kan 4 months, 1 week ago


On 2024-09-05 4:24 p.m., Namhyung Kim wrote:
> Commit 7b100989b4f6bce70 ("perf evlist: Remove __evlist__add_default")
> changed to parse "cycles:P" event instead of creating a new cycles
> event for perf record.  But it also changed the way how modifiers are
> handled so it doesn't set the exclude_guest bit by default.
> 
> 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.
> 
> Fixes: 7b100989b4f6bce70 ("perf evlist: Remove __evlist__add_default")
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: James Clark <james.clark@linaro.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-stat.c |  3 +--
>  tools/perf/util/evsel.c   | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index cf985cdb9a6ee588..d8315dae930184ba 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -639,8 +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 == ENXIO) {
> +	    errno == ENOENT || errno == ENXIO) {
>  		if (verbose > 0)
>  			ui__warning("%s event is not supported by the kernel.\n",
>  				    evsel__name(counter));

It seems the behavior for other reasons which trigger the 'EOPNOTSUPP'
is changed as well.
At least, it looks like we don't skip the member event with EOPNOTSUPP
anymore.

I'm not sure if it's a big deal. But I think we'd better mention it in
the change log or the comments.

Thanks,
Kan

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 49cc71511c0c8ce8..d59ad76b28758906 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -3244,6 +3244,27 @@ bool evsel__fallback(struct evsel *evsel, struct target *target, int err,
>  		evsel->core.attr.exclude_kernel = 1;
>  		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%sH", 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;
>  	}
>

Re: [PATCH 01/10] perf tools: Add fallback for exclude_guest

Posted by Namhyung Kim 3 months, 2 weeks ago

On Fri, Sep 06, 2024 at 09:47:53AM -0400, Liang, Kan wrote:
> 
> 
> On 2024-09-05 4:24 p.m., Namhyung Kim wrote:
> > Commit 7b100989b4f6bce70 ("perf evlist: Remove __evlist__add_default")
> > changed to parse "cycles:P" event instead of creating a new cycles
> > event for perf record.  But it also changed the way how modifiers are
> > handled so it doesn't set the exclude_guest bit by default.
> > 
> > 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.
> > 
> > Fixes: 7b100989b4f6bce70 ("perf evlist: Remove __evlist__add_default")
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: James Clark <james.clark@linaro.org>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/builtin-stat.c |  3 +--
> >  tools/perf/util/evsel.c   | 21 +++++++++++++++++++++
> >  2 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index cf985cdb9a6ee588..d8315dae930184ba 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -639,8 +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 == ENXIO) {
> > +	    errno == ENOENT || errno == ENXIO) {
> >  		if (verbose > 0)
> >  			ui__warning("%s event is not supported by the kernel.\n",
> >  				    evsel__name(counter));
> 
> It seems the behavior for other reasons which trigger the 'EOPNOTSUPP'
> is changed as well.
> At least, it looks like we don't skip the member event with EOPNOTSUPP
> anymore.
> 
> I'm not sure if it's a big deal. But I think we'd better mention it in
> the change log or the comments.

Yeah I think it should handle EOPNOTSUPP at the end of the function to
maintain the behavior.  Still it's not exactly the same but I think the
skippable case is ok.  Thanks for pointing this out.

Thanks,
Namhyung

> 
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index 49cc71511c0c8ce8..d59ad76b28758906 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -3244,6 +3244,27 @@ bool evsel__fallback(struct evsel *evsel, struct target *target, int err,
> >  		evsel->core.attr.exclude_kernel = 1;
> >  		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%sH", 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;
> >  	}
> >