Re: [PATCH] perf stat: Add arch-specific TopdownL1 check for the default mode

[Ian Rogers]

On Thu, Apr 27, 2023 at 11:31 AM <kan.liang@xxxxxxxxxxxxxxx> wrote:
>
> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>
> The default of perf stat fails on several Intel platforms.
> Skylake:
>
> $ perf stat true
> Error:
> Access to performance monitoring and observability operations is limited.
> Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open
> access to performance monitoring and observability operations for processes
> without CAP_PERFMON, CAP_SYS_PTRACE or CAP_SYS_ADMIN Linux capability.
> More information can be found at 'Perf events and tool security' document:
> https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html
> perf_event_paranoid setting is 2:
>   -1: Allow use of (almost) all events by all users
>       Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
> > = 0: Disallow raw and ftrace function tracepoint access
> > = 1: Disallow CPU event access
> > = 2: Disallow kernel profiling
>
> ADL (hybrid):
>
> ./perf stat
> Segmentation fault (core dumped)
>
> The default of perf stat was switched to TopdownL1 Json metric since
> commit 94b1a603fca7("perf stat: Add TopdownL1 metric as a default if
> present"). But the patch only checks whether the TopdownL1 is present
> in the event list. It doesn't check whether the hardware has the
> capability to provide a clean output for the default mode.
>
> Add arch_has_topdown_metric() to check the hardware capability as well.
> Drop the TopdownL1 support in the defalut mode for pre-ICL and hybrid
> platforms. Users can still use -M TopdownL1 to access the TopdownL1
> on pre-ICL platforms.
>
> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> ---
>
> The patch tries to workaround the serious issues on pre-ICL and hybrid
> platforms with the default mode of perf stat. It could be a temporary
> fix for the upcoming 6.4. So we have more time to look for a proper fix
> for all metrics issues and output issues with 6.5.
>
> Thanks,
> Kan
>
>  tools/perf/arch/x86/util/topdown.c | 14 ++++++++++++++
>  tools/perf/builtin-stat.c          |  2 +-
>  tools/perf/util/stat-display.c     |  2 +-
>  tools/perf/util/topdown.c          |  6 ++++++
>  tools/perf/util/topdown.h          |  2 ++
>  5 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
> index 9ad5e5c7bd27..5d861e851619 100644
> --- a/tools/perf/arch/x86/util/topdown.c
> +++ b/tools/perf/arch/x86/util/topdown.c
> @@ -3,6 +3,7 @@
>  #include "util/evsel.h"
>  #include "util/pmu.h"
>  #include "util/topdown.h"
> +#include "util/metricgroup.h"
>  #include "topdown.h"
>  #include "evsel.h"
>
> @@ -48,3 +49,16 @@ bool arch_topdown_sample_read(struct evsel *leader)
>
>         return false;
>  }
> +
> +bool arch_has_topdown_metric(const char *name)
> +{
> +       /*
> +        * Disable the Topdown events in the default mode
> +        * for hybrid platforms and old platform which
> +        * doesn't support the Topdown metric feature.
> +        */
> +       if (!pmu_have_event("cpu", "slots"))
> +               return false;

The only platform crashing with this are hybrid ones, I think the test
should be:

if (perf_pmu__has_hybrid())
   return false;

> +
> +       return metricgroup__has_metric(name);
> +}
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index efda63f6bf32..0b865155656d 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1885,7 +1885,7 @@ static int add_default_attributes(void)
>                  * Add TopdownL1 metrics if they exist. To minimize
>                  * multiplexing, don't request threshold computation.
>                  */
> -               if (metricgroup__has_metric("TopdownL1") &&
> +               if (arch_has_topdown_metric("TopdownL1") &&
>                     metricgroup__parse_groups(evsel_list, "TopdownL1",
>                                             /*metric_no_group=*/false,
>                                             /*metric_no_merge=*/false,
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index e6035ecbeee8..73b2ff2ddf29 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -747,7 +747,7 @@ static void uniquify_event_name(struct evsel *counter)
>         int ret = 0;
>
>         if (counter->uniquified_name || counter->use_config_name ||
> -           !counter->pmu_name || !strncmp(counter->name, counter->pmu_name,
> +           !counter->pmu_name || !strncmp(evsel__name(counter), counter->pmu_name,

This fix is here:
https://lore.kernel.org/lkml/20230426070050.1315519-1-irogers@xxxxxxxxxx/T/#mfce90d81aac130bbbf4743310b9ab918fc73d012

Thanks,
Ian

>                                            strlen(counter->pmu_name)))
>                 return;
>
> diff --git a/tools/perf/util/topdown.c b/tools/perf/util/topdown.c
> index 18fd5fed5d1a..f3a9ebc52f8b 100644
> --- a/tools/perf/util/topdown.c
> +++ b/tools/perf/util/topdown.c
> @@ -1,8 +1,14 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include "topdown.h"
> +#include "metricgroup.h"
>  #include <linux/kernel.h>
>
>  __weak bool arch_topdown_sample_read(struct evsel *leader __maybe_unused)
>  {
>         return false;
>  }
> +
> +__weak bool arch_has_topdown_metric(const char *name)
> +{
> +       return metricgroup__has_metric(name);
> +}
> diff --git a/tools/perf/util/topdown.h b/tools/perf/util/topdown.h
> index 1996c5fedcd7..7e83c8b247f2 100644
> --- a/tools/perf/util/topdown.h
> +++ b/tools/perf/util/topdown.h
> @@ -8,4 +8,6 @@ struct evsel;
>
>  bool arch_topdown_sample_read(struct evsel *leader);
>
> +bool arch_has_topdown_metric(const char *name);
> +
>  #endif
> --
> 2.35.1
>