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

Namhyung Kim posted 8 patches 1 year, 5 months ago
[PATCH 7/8] perf tools: Add fallback for exclude_guest
Posted by Namhyung Kim 1 year, 5 months ago
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@arm.com>
Cc: James Clark <james.clark@linaro.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.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;
 	}
 
-- 
2.46.0.469.g59c65b2a67-goog
Re: [PATCH 7/8] perf tools: Add fallback for exclude_guest
Posted by James Clark 1 year, 5 months ago

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@arm.com>
> Cc: James Clark <james.clark@linaro.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.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:

  $ perf stat -e cycles -vvv -- true

   Control descriptor is not initialized
   Opening: cycles
   ------------------------------------------------------------
   perf_event_attr:
     type                             0 (PERF_TYPE_HARDWARE)
     size                             136
     config                           0xb00000000
     sample_type                      IDENTIFIER
     read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
     disabled                         1
     inherit                          1
     enable_on_exec                   1
   ------------------------------------------------------------
   sys_perf_event_open: pid 698  cpu -1  group_fd -1  flags 0x8
   sys_perf_event_open failed, error -95
   Warning:
   cycles event is not supported by the kernel.
   Opening: cycles
   ------------------------------------------------------------
   perf_event_attr:
     type                             0 (PERF_TYPE_HARDWARE)
     size                             136
     config                           0xa00000000
     sample_type                      IDENTIFIER
     read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
     disabled                         1
     inherit                          1
     enable_on_exec                   1
   ------------------------------------------------------------
   sys_perf_event_open: pid 698  cpu -1  group_fd -1  flags 0x8
   sys_perf_event_open failed, error -95
   Warning:
   cycles event is not supported by the kernel.
   failed to read counter cycles
   failed to read counter cycles

    Performance counter stats for 'true':

      <not supported>      armv8_cortex_a53/cycles/ 

      <not supported>      armv8_cortex_a57/cycles/ 




Other than that, all the tests are passing on Juno (without the 
exclude_guest requirement).
Re: [PATCH 7/8] perf tools: Add fallback for exclude_guest
Posted by James Clark 1 year, 5 months ago

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@arm.com>
>> Cc: James Clark <james.clark@linaro.org>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.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:

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


Re: [PATCH 7/8] perf tools: Add fallback for exclude_guest
Posted by Namhyung Kim 1 year, 5 months ago
Hello James,

On Wed, Sep 4, 2024 at 6:36 AM James Clark <james.clark@linaro.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@arm.com>
> >> Cc: James Clark <james.clark@linaro.org>
> >> Signed-off-by: Namhyung Kim <namhyung@kernel.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",
>
>
Re: [PATCH 7/8] perf tools: Add fallback for exclude_guest
Posted by James Clark 1 year, 5 months ago

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@arm.com>
>> Cc: James Clark <james.clark@linaro.org>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.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:
> 
>   $ perf stat -e cycles -vvv -- true
> 
>    Control descriptor is not initialized
>    Opening: cycles
>    ------------------------------------------------------------
>    perf_event_attr:
>      type                             0 (PERF_TYPE_HARDWARE)
>      size                             136
>      config                           0xb00000000
>      sample_type                      IDENTIFIER
>      read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>      disabled                         1
>      inherit                          1
>      enable_on_exec                   1
>    ------------------------------------------------------------
>    sys_perf_event_open: pid 698  cpu -1  group_fd -1  flags 0x8
>    sys_perf_event_open failed, error -95
>    Warning:
>    cycles event is not supported by the kernel.
>    Opening: cycles
>    ------------------------------------------------------------
>    perf_event_attr:
>      type                             0 (PERF_TYPE_HARDWARE)
>      size                             136
>      config                           0xa00000000
>      sample_type                      IDENTIFIER
>      read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>      disabled                         1
>      inherit                          1
>      enable_on_exec                   1
>    ------------------------------------------------------------
>    sys_perf_event_open: pid 698  cpu -1  group_fd -1  flags 0x8
>    sys_perf_event_open failed, error -95
>    Warning:
>    cycles event is not supported by the kernel.
>    failed to read counter cycles
>    failed to read counter cycles
> 
>     Performance counter stats for 'true':
> 
>       <not supported>      armv8_cortex_a53/cycles/
>       <not supported>      armv8_cortex_a57/cycles/
> 
> 
> 
> Other than that, all the tests are passing on Juno (without the 
> exclude_guest requirement).

Sorry one other thing, I think this commit would also need to come 
before the change to the exclude_guest default to keep bisect working.