lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fUwWONnjrgo49cX1fsv4Xn4nT+z_Hz6eXQR23cfXznBdA@mail.gmail.com>
Date: Fri, 18 Jul 2025 09:44:19 -0700
From: Ian Rogers <irogers@...gle.com>
To: Thomas Falcon <thomas.falcon@...el.com>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Ian Rogers <irogers@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>, 
	"Liang, Kan" <kan.liang@...ux.intel.com>, Ravi Bangoria <ravi.bangoria@....com>, 
	James Clark <james.clark@...aro.org>, Dapeng Mi <dapeng1.mi@...ux.intel.com>, 
	Weilin Wang <weilin.wang@...el.com>, Andi Kleen <ak@...ux.intel.com>, linux-kernel@...r.kernel.org, 
	linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v2 2/2] perf parse-events: Fix missing slots for Intel
 topdown metric events

On Fri, Jul 18, 2025 at 6:27 AM Ian Rogers <irogers@...gle.com> wrote:
>
> Topdown metric events require grouping with a slots event. In perf
> metrics this is currently achieved by metrics adding an unnecessary
> "0 * tma_info_thread_slots". New TMA metrics trigger optimizations of
> the metric expression that removes the event and breaks the metric due
> to the missing but required event. Add a pass immediately before
> sorting and fixing parsed events, that insert a slots event if one is
> missing. Update test expectations to match this.
>
> Signed-off-by: Ian Rogers <irogers@...gle.com>
> ---
>  tools/perf/arch/x86/util/evlist.c  | 24 ++++++++++++++++++++++++
>  tools/perf/arch/x86/util/topdown.c | 27 +++++++++++++++++++++++++++
>  tools/perf/arch/x86/util/topdown.h |  2 ++
>  tools/perf/tests/parse-events.c    | 24 ++++++++++++------------
>  tools/perf/util/evlist.h           |  1 +
>  tools/perf/util/parse-events.c     | 10 ++++++++++
>  6 files changed, 76 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index 1969758cc8c1..75e9d00a1494 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -81,3 +81,27 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>         /* Default ordering by insertion index. */
>         return lhs->core.idx - rhs->core.idx;
>  }
> +
> +int arch_evlist__add_required_events(struct list_head *list)
> +{
> +       struct evsel *pos, *metric_event = NULL;
> +       int idx = 0;
> +
> +       if (!topdown_sys_has_perf_metrics())
> +               return 0;
> +
> +       list_for_each_entry(pos, list, core.node) {
> +               if (arch_is_topdown_slots(pos)) {
> +                       /* Slots event already present, nothing to do. */
> +                       return 0;
> +               }
> +               if (metric_event == NULL && arch_is_topdown_metrics(pos))
> +                       metric_event = pos;
> +               idx++;
> +       }
> +       if (metric_event == NULL) {
> +               /* No topdown metric events, nothing to do. */
> +               return 0;
> +       }
> +       return topdown_insert_slots_event(list, idx + 1, metric_event);
> +}
> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
> index 66b231fbf52e..5d53e3d59c0c 100644
> --- a/tools/perf/arch/x86/util/topdown.c
> +++ b/tools/perf/arch/x86/util/topdown.c
> @@ -77,3 +77,30 @@ bool arch_topdown_sample_read(struct evsel *leader)
>
>         return false;
>  }
> +
> +/*
> + * Make a copy of the topdown metric event metric_event with the given index but
> + * change its configuration to be a topdown slots event. Copying from
> + * metric_event ensures modifiers are the same.
> + */
> +int topdown_insert_slots_event(struct list_head *list, int idx, struct evsel *metric_event)
> +{
> +       struct evsel *evsel = evsel__new_idx(&metric_event->core.attr, idx);
> +
> +       if (!evsel)
> +               return -ENOMEM;
> +
> +       evsel->core.attr.config = TOPDOWN_SLOTS;
> +       evsel->core.cpus = perf_cpu_map__get(metric_event->core.cpus);
> +       evsel->core.own_cpus = perf_cpu_map__get(metric_event->core.own_cpus);

Note, this change conflicts with:
https://lore.kernel.org/lkml/20250717210233.1143622-1-irogers@google.com/
(reviewed but not merged) due to own_cpus being renamed to pmu_cpus. The fix is:
```
--- a/tools/perf/arch/x86/util/topdown.c
+++ b/tools/perf/arch/x86/util/topdown.c
@@ -92,7 +92,7 @@ int topdown_insert_slots_event(struct list_head
*list, int idx, struct evsel *me

       evsel->core.attr.config = TOPDOWN_SLOTS;
       evsel->core.cpus = perf_cpu_map__get(metric_event->core.cpus);
-       evsel->core.own_cpus = perf_cpu_map__get(metric_event->core.own_cpus);
+       evsel->core.pmu_cpus = perf_cpu_map__get(metric_event->core.pmu_cpus);
       evsel->core.is_pmu_core = true;
       evsel->pmu = metric_event->pmu;
       evsel->name = strdup("slots");
```
and I can send this fix when:
https://lore.kernel.org/lkml/20250717210233.1143622-1-irogers@google.com/
lands.

Thanks,
Ian

> +       evsel->core.is_pmu_core = true;
> +       evsel->pmu = metric_event->pmu;
> +       evsel->name = strdup("slots");
> +       evsel->precise_max = metric_event->precise_max;
> +       evsel->sample_read = metric_event->sample_read;
> +       evsel->weak_group = metric_event->weak_group;
> +       evsel->bpf_counter = metric_event->bpf_counter;
> +       evsel->retire_lat = metric_event->retire_lat;
> +       list_add_tail(&evsel->core.node, list);
> +       return 0;
> +}
> diff --git a/tools/perf/arch/x86/util/topdown.h b/tools/perf/arch/x86/util/topdown.h
> index 2349536cf882..69035565e649 100644
> --- a/tools/perf/arch/x86/util/topdown.h
> +++ b/tools/perf/arch/x86/util/topdown.h
> @@ -5,9 +5,11 @@
>  #include <stdbool.h>
>
>  struct evsel;
> +struct list_head;
>
>  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);
> +int topdown_insert_slots_event(struct list_head *list, int idx, struct evsel *metric_event);
>
>  #endif
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 5ec2e5607987..bb8004397650 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -719,20 +719,20 @@ static int test__checkevent_pmu_partial_time_callgraph(struct evlist *evlist)
>
>  static int test__checkevent_pmu_events(struct evlist *evlist)
>  {
> -       struct evsel *evsel = evlist__first(evlist);
> +       struct evsel *evsel;
>
> -       TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->core.nr_entries);
> -       TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type ||
> -                                     strcmp(evsel->pmu->name, "cpu"));
> -       TEST_ASSERT_VAL("wrong exclude_user",
> -                       !evsel->core.attr.exclude_user);
> -       TEST_ASSERT_VAL("wrong exclude_kernel",
> -                       evsel->core.attr.exclude_kernel);
> -       TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> -       TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
> -       TEST_ASSERT_VAL("wrong pinned", !evsel->core.attr.pinned);
> -       TEST_ASSERT_VAL("wrong exclusive", !evsel->core.attr.exclusive);
> +       TEST_ASSERT_VAL("wrong number of entries", 1 <= evlist->core.nr_entries);
>
> +       evlist__for_each_entry(evlist, evsel) {
> +               TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type ||
> +                                             strcmp(evsel->pmu->name, "cpu"));
> +               TEST_ASSERT_VAL("wrong exclude_user", !evsel->core.attr.exclude_user);
> +               TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
> +               TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> +               TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
> +               TEST_ASSERT_VAL("wrong pinned", !evsel->core.attr.pinned);
> +               TEST_ASSERT_VAL("wrong exclusive", !evsel->core.attr.exclusive);
> +       }
>         return TEST_OK;
>  }
>
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index fac1a01ba13f..1472d2179be1 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -111,6 +111,7 @@ void evlist__add(struct evlist *evlist, struct evsel *entry);
>  void evlist__remove(struct evlist *evlist, struct evsel *evsel);
>
>  int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs);
> +int arch_evlist__add_required_events(struct list_head *list);
>
>  int evlist__add_dummy(struct evlist *evlist);
>  struct evsel *evlist__add_aux_dummy(struct evlist *evlist, bool system_wide);
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index a59ae5ca0f89..01dab5dd3540 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2132,6 +2132,11 @@ static int evlist__cmp(void *_fg_idx, const struct list_head *l, const struct li
>         return arch_evlist__cmp(lhs, rhs);
>  }
>
> +int __weak arch_evlist__add_required_events(struct list_head *list __always_unused)
> +{
> +       return 0;
> +}
> +
>  static int parse_events__sort_events_and_fix_groups(struct list_head *list)
>  {
>         int idx = 0, force_grouped_idx = -1;
> @@ -2143,6 +2148,11 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
>         struct evsel *force_grouped_leader = NULL;
>         bool last_event_was_forced_leader = false;
>
> +       /* On x86 topdown metrics events require a slots event. */
> +       ret = arch_evlist__add_required_events(list);
> +       if (ret)
> +               return ret;
> +
>         /*
>          * Compute index to insert ungrouped events at. Place them where the
>          * first ungrouped event appears.
> --
> 2.50.0.727.gbf7dc18ff4-goog
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ