Re: [Patch v5 1/6] perf x86/topdown: Complete topdown slots/metrics events check

[Ian Rogers]

On Thu, Sep 12, 2024 at 10:21 PM Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx> wrote:
>
> It's not complete to check whether an event is a topdown slots or
> topdown metrics event by only comparing the event name since user
> may assign the event by RAW format, e.g.
>
> perf stat -e '{instructions,cpu/r400/,cpu/r8300/}' sleep 1
>
>  Performance counter stats for 'sleep 1':
>
>      <not counted>      instructions
>      <not counted>      cpu/r400/
>    <not supported>      cpu/r8300/
>
>        1.002917796 seconds time elapsed
>
>        0.002955000 seconds user
>        0.000000000 seconds sys
>
> The RAW format slots and topdown-be-bound events are not recognized and
> not regroup the events, and eventually cause error.
>
> Thus add two helpers arch_is_topdown_slots()/arch_is_topdown_metrics()
> to detect whether an event is topdown slots/metrics event by comparing
> the event config directly, and use these two helpers to replace the
> original event name comparisons.
>
> Reviewed-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> Signed-off-by: Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx>
> ---
>  tools/perf/arch/x86/util/evlist.c  |  8 ++---
>  tools/perf/arch/x86/util/evsel.c   |  3 +-
>  tools/perf/arch/x86/util/topdown.c | 48 +++++++++++++++++++++++++++++-
>  tools/perf/arch/x86/util/topdown.h |  2 ++
>  4 files changed, 55 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index cebdd483149e..79799865a62a 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -78,14 +78,14 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>         if (topdown_sys_has_perf_metrics() &&
>             (arch_evsel__must_be_in_group(lhs) || arch_evsel__must_be_in_group(rhs))) {
>                 /* Ensure the topdown slots comes first. */
> -               if (strcasestr(lhs->name, "slots") && !strcasestr(lhs->name, "uops_retired.slots"))
> +               if (arch_is_topdown_slots(lhs))
>                         return -1;
> -               if (strcasestr(rhs->name, "slots") && !strcasestr(rhs->name, "uops_retired.slots"))
> +               if (arch_is_topdown_slots(rhs))
>                         return 1;
>                 /* Followed by topdown events. */
> -               if (strcasestr(lhs->name, "topdown") && !strcasestr(rhs->name, "topdown"))
> +               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>                         return -1;
> -               if (!strcasestr(lhs->name, "topdown") && strcasestr(rhs->name, "topdown"))
> +               if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
>                         return 1;
>         }
>
> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> index 090d0f371891..181f2ba0bb2a 100644
> --- a/tools/perf/arch/x86/util/evsel.c
> +++ b/tools/perf/arch/x86/util/evsel.c
> @@ -6,6 +6,7 @@
>  #include "util/pmu.h"
>  #include "util/pmus.h"
>  #include "linux/string.h"
> +#include "topdown.h"
>  #include "evsel.h"
>  #include "util/debug.h"
>  #include "env.h"
> @@ -44,7 +45,7 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>             strcasestr(evsel->name, "uops_retired.slots"))
>                 return false;
>
> -       return strcasestr(evsel->name, "topdown") || strcasestr(evsel->name, "slots");
> +       return arch_is_topdown_metrics(evsel) || arch_is_topdown_slots(evsel);
>  }
>
>  int arch_evsel__hw_name(struct evsel *evsel, char *bf, size_t size)
> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
> index 3f9a267d4501..49f25d67ed77 100644
> --- a/tools/perf/arch/x86/util/topdown.c
> +++ b/tools/perf/arch/x86/util/topdown.c
> @@ -32,6 +32,52 @@ bool topdown_sys_has_perf_metrics(void)
>  }
>
>  #define TOPDOWN_SLOTS          0x0400
> +bool arch_is_topdown_slots(const struct evsel *evsel)
> +{

Perhaps: assert(evsel__find_pmu(evsel)->is_core);

> +       if (evsel->core.attr.config == TOPDOWN_SLOTS)
> +               return true;
> +
> +       return false;
> +}
> +
> +static int compare_topdown_event(void *vstate, struct pmu_event_info *info)
> +{
> +       int *config = vstate;
> +       int event = 0;
> +       int umask = 0;
> +       char *str;
> +
> +       if (!strcasestr(info->name, "topdown"))
> +               return 0;
> +
> +       str = strcasestr(info->str, "event=");
> +       if (str)
> +               sscanf(str, "event=%x", &event);
> +
> +       str = strcasestr(info->str, "umask=");
> +       if (str)
> +               sscanf(str, "umask=%x", &umask);
> +
> +       if (event == 0 && *config == (event | umask << 8))
> +               return 1;
> +
> +       return 0;
> +}
> +
> +bool arch_is_topdown_metrics(const struct evsel *evsel)
> +{
> +       struct perf_pmu *pmu = evsel__find_pmu(evsel);
> +       int config = evsel->core.attr.config;
> +
> +       if (!pmu || !pmu->is_core)
> +               return false;
> +
> +       if (perf_pmu__for_each_event(pmu, false, &config,
> +                                    compare_topdown_event))
> +               return true;
> +
> +       return false;
> +}


Doing a linear search over every event is painful perf_pmu__have_event
will try to binary search. The search rejects all events without
"topdown" in their name, it then sees if the event code is 0 and the
event|umask match the sysfs/json event. Is there ever a case the
"topdown" string isn't at the beginning of the string? If it is it
should be possible to binary search to the start of the topdown
events.

Strictly you shouldn't hard code the config positions of event and
umask, they are in the format list:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.h?h=perf-tools-next#n113
There is code doing similar to this here:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.c?h=perf-tools-next#n2285
but it avoids the scanf, uses formats...
It seems reasonable this code should reject all non-zero configs
before searching all core PMU events. It could also use
perf_pmu__name_from_config. So:
```
bool evsel__is_topdown_metric_event(const struct evsel *evsel)
{
   struct perf_pmu *pmu;
   const char *name_from_config;

   if (evsel->core.attr.config & 0xFF != 0) /* All topdown events have
an event code of 0. */
     return false;

  pmu  = evsel__find_pmu(evsel);
  if (!pmu || !pmu->is_core)
     return false;

  name_from_config = perf_pmu__name_from_config(pmu, config);
  return name_from_config && !strcasestr(name_from_config, "topdown);
}
```
We could tweak perf_pmu__name_from_config to be pased a  "filter". In
this case the filter would skip events without topdown in their name,
without doing a config comparison.

If later we change perf_pmu__name_from_config to say sort events in a
list by config, then this code could take advantage of that. Perhaps
for now there should just be an optional "prefix" that can be used to
binary search to the events.
```
  name_from_config = perf_pmu__name_from_config(pmu, config,
/*prefix=*/"topdown");
```

Thanks,
Ian

>  /*
>   * Check whether a topdown group supports sample-read.
> @@ -44,7 +90,7 @@ bool arch_topdown_sample_read(struct evsel *leader)
>         if (!evsel__sys_has_perf_metrics(leader))
>                 return false;
>
> -       if (leader->core.attr.config == TOPDOWN_SLOTS)
> +       if (arch_is_topdown_slots(leader))
>                 return true;
>
>         return false;
> diff --git a/tools/perf/arch/x86/util/topdown.h b/tools/perf/arch/x86/util/topdown.h
> index 46bf9273e572..1bae9b1822d7 100644
> --- a/tools/perf/arch/x86/util/topdown.h
> +++ b/tools/perf/arch/x86/util/topdown.h
> @@ -3,5 +3,7 @@
>  #define _TOPDOWN_H 1
>
>  bool topdown_sys_has_perf_metrics(void);
> +bool arch_is_topdown_slots(const struct evsel *evsel);
> +bool arch_is_topdown_metrics(const struct evsel *evsel);
>
>  #endif
> --
> 2.40.1
>