Re: [PATCH 1/4] perf evsel: Fixes topdown events in a weak group for the hybrid platform

[Ian Rogers <irogers@google.com>]

On Fri, May 13, 2022 at 8:16 AM <kan.liang@linux.intel.com> wrote:
>
> From: Kan Liang <kan.liang@linux.intel.com>
>
> The patch ("perf evlist: Keep topdown counters in weak group") fixes the
> perf metrics topdown event issue when the topdown events are in a weak
> group on a non-hybrid platform. However, it doesn't work for the hybrid
> platform.
>
> $./perf stat -e '{cpu_core/slots/,cpu_core/topdown-bad-spec/,
> cpu_core/topdown-be-bound/,cpu_core/topdown-fe-bound/,
> cpu_core/topdown-retiring/,cpu_core/branch-instructions/,
> cpu_core/branch-misses/,cpu_core/bus-cycles/,cpu_core/cache-misses/,
> cpu_core/cache-references/,cpu_core/cpu-cycles/,cpu_core/instructions/,
> cpu_core/mem-loads/,cpu_core/mem-stores/,cpu_core/ref-cycles/,
> cpu_core/cache-misses/,cpu_core/cache-references/}:W' -a sleep 1
>
>  Performance counter stats for 'system wide':
>
>        751,765,068      cpu_core/slots/                                               (84.07%)
>    <not supported>      cpu_core/topdown-bad-spec/
>    <not supported>      cpu_core/topdown-be-bound/
>    <not supported>      cpu_core/topdown-fe-bound/
>    <not supported>      cpu_core/topdown-retiring/
>         12,398,197      cpu_core/branch-instructions/                                     (84.07%)
>          1,054,218      cpu_core/branch-misses/                                       (84.24%)
>        539,764,637      cpu_core/bus-cycles/                                          (84.64%)
>             14,683      cpu_core/cache-misses/                                        (84.87%)
>          7,277,809      cpu_core/cache-references/                                     (77.30%)
>        222,299,439      cpu_core/cpu-cycles/                                          (77.28%)
>         63,661,714      cpu_core/instructions/                                        (84.85%)
>                  0      cpu_core/mem-loads/                                           (77.29%)
>         12,271,725      cpu_core/mem-stores/                                          (77.30%)
>        542,241,102      cpu_core/ref-cycles/                                          (84.85%)
>              8,854      cpu_core/cache-misses/                                        (76.71%)
>          7,179,013      cpu_core/cache-references/                                     (76.31%)
>
>        1.003245250 seconds time elapsed
>
> A hybrid platform has a different PMU name for the core PMUs, while
> the current perf hard code the PMU name "cpu".
>
> The evsel->pmu_name can be used to replace the "cpu" to fix the issue.
> For a hybrid platform, the pmu_name must be non-NULL. Because there are
> at least two core PMUs. The PMU has to be specified.
> For a non-hybrid platform, the pmu_name may be NULL. Because there is
> only one core PMU, "cpu". For a NULL pmu_name, we can safely assume that
> it is a "cpu" PMU.
>
> With the patch,
>
> $perf stat -e '{cpu_core/slots/,cpu_core/topdown-bad-spec/,
> cpu_core/topdown-be-bound/,cpu_core/topdown-fe-bound/,
> cpu_core/topdown-retiring/,cpu_core/branch-instructions/,
> cpu_core/branch-misses/,cpu_core/bus-cycles/,cpu_core/cache-misses/,
> cpu_core/cache-references/,cpu_core/cpu-cycles/,cpu_core/instructions/,
> cpu_core/mem-loads/,cpu_core/mem-stores/,cpu_core/ref-cycles/,
> cpu_core/cache-misses/,cpu_core/cache-references/}:W' -a sleep 1
>
>  Performance counter stats for 'system wide':
>
>        766,620,266      cpu_core/slots/                                               (84.06%)
>         73,172,129      cpu_core/topdown-bad-spec/ #      9.5% bad speculation         (84.06%)
>        193,443,341      cpu_core/topdown-be-bound/ #     25.0% backend bound           (84.06%)
>        403,940,929      cpu_core/topdown-fe-bound/ #     52.3% frontend bound          (84.06%)
>        102,070,237      cpu_core/topdown-retiring/ #     13.2% retiring                (84.06%)
>         12,364,429      cpu_core/branch-instructions/                                     (84.03%)
>          1,080,124      cpu_core/branch-misses/                                       (84.24%)
>        564,120,383      cpu_core/bus-cycles/                                          (84.65%)
>             36,979      cpu_core/cache-misses/                                        (84.86%)
>          7,298,094      cpu_core/cache-references/                                     (77.30%)
>        227,174,372      cpu_core/cpu-cycles/                                          (77.31%)
>         63,886,523      cpu_core/instructions/                                        (84.87%)
>                  0      cpu_core/mem-loads/                                           (77.31%)
>         12,208,782      cpu_core/mem-stores/                                          (77.31%)
>        566,409,738      cpu_core/ref-cycles/                                          (84.87%)
>             23,118      cpu_core/cache-misses/                                        (76.71%)
>          7,212,602      cpu_core/cache-references/                                     (76.29%)
>
>        1.003228667 seconds time elapsed
>
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> ---
>  tools/perf/arch/x86/util/evsel.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> index 00cb4466b4ca..24510bcb4bf4 100644
> --- a/tools/perf/arch/x86/util/evsel.c
> +++ b/tools/perf/arch/x86/util/evsel.c
> @@ -33,8 +33,9 @@ void arch_evsel__fixup_new_cycles(struct perf_event_attr *attr)
>
>  bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>  {
> -       if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
> -           !pmu_have_event("cpu", "slots"))
> +       const char *pmu_name = evsel->pmu_name ? evsel->pmu_name : "cpu";
> +
> +       if (!pmu_have_event(pmu_name, "slots"))

Playing devil's advocate, if I have a PMU for my network accelerator
and it has an event called "slots" then this test will also be true.

The property that is being tested here is "does this CPU have topdown
events" and so allowing any PMU removes the "does this CPU" part of
the equation. I think ideally we'd have an arch functions something
like:

bool arch_pmu__has_intel_topdown_events(void)
{
  static bool has_topdown_events = pmu_have_event("cpu", "slots") ||
pmu_have_event("cpu_core", "slots");

  return has_topdown_events;
}

bool arch_pmu__supports_intel_topdown_events(const char *pmu_name)
{
  if (!pmu_name)
    return false;
  return arch_pmu__has_intel_topdown_events() && (!strncmp(pmu_name,
"cpu") || !strncmp(pmu_name, "cpu_core"));
}

bool arch_evsel__is_intel_topdown_event(struct evsel *evsel)
{
  if (!arch_pmu__supports_intel_topdown_events(evsel->pmu))
    return false;

  return strcasestr(evsel->name, "slots") || strcasestr(evsel->name, "topdown");
}

This then gives us:

bool arch_evsel__must_be_in_group(const struct evsel *evsel)
{
  return  arch_evsel__is_intel_topdown_event(evsel);
}

These functions can then be reused for the arch_evlist topdown code,
etc. What I don't see in these functions is use of any hybrid
abstraction and so it isn't clear to me how with hybrid something like
this would be plumbed in.

Thanks,
Ian

>                 return false;
>
>         return evsel->name &&
> --
> 2.35.1
>