lkml.org 
[lkml]   [2021]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@redhat.com>.
>
> Reviewed-by: Kajol Jain<kjain@linux.ibm.com>
> Signed-off-by: Ian Rogers <irogers@google.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

\
 
 \ /
  Last update: 2021-11-30 16:22    [W:0.072 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site