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

["Liang, Kan" <kan.liang@linux.intel.com>]

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