| 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
| ||
|
Message-ID: <YaY91Ytf3x4DGgJ+@kernel.org>
Date: Tue, 30 Nov 2021 12:05:57 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Andi Kleen <ak@...ux.intel.com>, Jiri Olsa <jolsa@...hat.com>,
Namhyung Kim <namhyung@...nel.org>,
John Garry <john.garry@...wei.com>,
Kajol Jain <kjain@...ux.ibm.com>,
"Paul A . Clarke" <pc@...ibm.com>,
Riccardo Mancini <rickyman7@...il.com>,
Kan Liang <kan.liang@...ux.intel.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
Vineet Singh <vineet.singh@...el.com>, eranian@...gle.com
Subject: Re: [PATCH v2 2/2] perf parse-events: Architecture specific leader
override
Em Thu, Nov 18, 2021 at 02:06:47PM -0800, Ian Rogers escreveu:
> Currently topdown events must appear after a slots event:
>
> $ perf stat -e '{slots,topdown-fe-bound}' /bin/true
>
> Performance counter stats for '/bin/true':
>
> 3,183,090 slots
> 986,133 topdown-fe-bound
>
> Reversing the events yields:
>
> $ perf stat -e '{topdown-fe-bound,slots}' /bin/true
> Error:
> The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (topdown-fe-bound).
>
> For metrics the order of events is determined by iterating over a
> hashmap, and so slots isn't guaranteed to be first which can yield this
> error.
>
> Change the set_leader in parse-events, called when a group is closed, so
> that rather than always making the first event the leader, if the slots
> event exists then it is made the leader. It is then moved to the head of
> the evlist otherwise it won't be opened in the correct order.
>
> The result is:
>
> $ perf stat -e '{topdown-fe-bound,slots}' /bin/true
>
> Performance counter stats for '/bin/true':
>
> 3,274,795 slots
> 1,001,702 topdown-fe-bound
>
> A problem with this approach is the slots event is identified by name,
> names can be overwritten like 'cpu/slots,name=foo/' and this causes the
> leader change to fail.
>
> The change also modifies and fixes mixed groups like, with the change:
>
> $ perf stat -e '{instructions,slots,topdown-fe-bound}' -a -- sleep 2
>
> Performance counter stats for 'system wide':
>
> 5574985410 slots
> 971981616 instructions
> 1348461887 topdown-fe-bound
>
> 2.001263120 seconds time elapsed
>
> Without the change:
>
> $ perf stat -e '{instructions,slots,topdown-fe-bound}' -a -- sleep 2
>
> Performance counter stats for 'system wide':
>
> <not counted> instructions
> <not counted> slots
> <not supported> topdown-fe-bound
>
> 2.006247990 seconds time elapsed
>
> Something that may be undesirable here is that the events are reordered
> in the output.
>
> v2. Move the list_move operation into parse_events__set_leader as
> suggested by Jiri Olsa <jolsa@...hat.com>.
>
> Reviewed-by: Kajol Jain<kjain@...ux.ibm.com>
> Signed-off-by: Ian Rogers <irogers@...gle.com>
> ---
> tools/perf/arch/x86/util/evlist.c | 17 +++++++++++++++++
> tools/perf/util/evlist.h | 1 +
> tools/perf/util/parse-events.c | 8 +++++++-
> 3 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index 0b0951030a2f..1624372b2da2 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -17,3 +17,20 @@ int arch_evlist__add_default_attrs(struct evlist *evlist)
> else
> return parse_events(evlist, TOPDOWN_L1_EVENTS, NULL);
> }
> +
> +struct evsel *arch_evlist__leader(struct list_head *list)
> +{
> + struct evsel *evsel, *first;
> +
> + first = list_entry(list->next, struct evsel, core.node);
Can we make this:
struct *evsel, *first = list_first_entry(list, struct evsel, core.node);
?
> +
> + if (!pmu_have_event("cpu", "slots"))
> + return first;
> +
> + __evlist__for_each_entry(list, evsel) {
> + if (evsel->pmu_name && !strcmp(evsel->pmu_name, "cpu") &&
> + evsel->name && strstr(evsel->name, "slots"))
> + return evsel;
> + }
> + return first;
> +}
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index 97bfb8d0be4f..993437ffe429 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -110,6 +110,7 @@ int __evlist__add_default_attrs(struct evlist *evlist,
> __evlist__add_default_attrs(evlist, array, ARRAY_SIZE(array))
>
> int arch_evlist__add_default_attrs(struct evlist *evlist);
> +struct evsel *arch_evlist__leader(struct list_head *list);
>
> int evlist__add_dummy(struct evlist *evlist);
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 6308ba739d19..774c163eae9c 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1821,6 +1821,11 @@ parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
> return ret;
> }
>
> +__weak struct evsel *arch_evlist__leader(struct list_head *list)
> +{
> + return list_entry(list->next, struct evsel, core.node);
> +}
> +
> void parse_events__set_leader(char *name, struct list_head *list,
> struct parse_events_state *parse_state)
> {
> @@ -1834,9 +1839,10 @@ void parse_events__set_leader(char *name, struct list_head *list,
> if (parse_events__set_leader_for_uncore_aliase(name, list, parse_state))
> return;
>
> - leader = list_entry(list->next, struct evsel, core.node);
> + leader = arch_evlist__leader(list);
> __perf_evlist__set_leader(list, &leader->core);
> leader->group_name = name ? strdup(name) : NULL;
> + list_move(&leader->core.node, list);
> }
>
> /* list_event is assumed to point to malloc'ed memory */
> --
> 2.34.0.rc2.393.gf8c9666880-goog
--
- Arnaldo
Powered by blists - more mailing lists