Messages in this thread | | | Date | Tue, 30 Nov 2021 12:05:57 -0300 | From | Arnaldo Carvalho de Melo <> | 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@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
| |