Messages in this thread Patch in this message | | | Date | Sun, 14 Nov 2021 18:02:04 +0100 | From | Jiri Olsa <> | Subject | Re: [PATCH 2/2] perf parse-events: Architecture specific leader override |
| |
On Fri, Nov 12, 2021 at 11:15:48PM -0800, Ian Rogers wrote: > 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).
why is this actually failing?
> > 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.
would it be better then to move this shuffle to the metric code directly, and make sure it only spits 'good' order of events?
and keep "-e '{topdown-fe-bound,slots}'" to fail if user specifies that on the command line
> > 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 | 16 +++++++++++----- > tools/perf/util/parse-events.h | 4 ++-- > tools/perf/util/parse-events.y | 12 ++++++++---- > 5 files changed, 39 insertions(+), 11 deletions(-) > > 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); > + > + 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..a2f4c086986f 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -1821,22 +1821,28 @@ parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list, > return ret; > } > > -void parse_events__set_leader(char *name, struct list_head *list, > - struct parse_events_state *parse_state) > +__weak struct evsel *arch_evlist__leader(struct list_head *list) > +{ > + return list_entry(list->next, struct evsel, core.node); > +} > + > +struct list_head *parse_events__set_leader(char *name, struct list_head *list, > + struct parse_events_state *parse_state) > { > struct evsel *leader; > > if (list_empty(list)) { > WARN_ONCE(true, "WARNING: failed to set leader: empty list"); > - return; > + return NULL; > } > > if (parse_events__set_leader_for_uncore_aliase(name, list, parse_state)) > - return; > + return NULL; > > - 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; > + return &leader->core.node; > } > > /* list_event is assumed to point to malloc'ed memory */ > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h > index c7fc93f54577..8a6e6b4d5cbe 100644 > --- a/tools/perf/util/parse-events.h > +++ b/tools/perf/util/parse-events.h > @@ -211,8 +211,8 @@ int parse_events_copy_term_list(struct list_head *old, > > enum perf_pmu_event_symbol_type > perf_pmu__parse_check(const char *name); > -void parse_events__set_leader(char *name, struct list_head *list, > - struct parse_events_state *parse_state); > +struct list_head *parse_events__set_leader(char *name, struct list_head *list, > + struct parse_events_state *parse_state); > void parse_events_update_lists(struct list_head *list_event, > struct list_head *list_all); > void parse_events_evlist_error(struct parse_events_state *parse_state, > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y > index 174158982fae..5358c400f938 100644 > --- a/tools/perf/util/parse-events.y > +++ b/tools/perf/util/parse-events.y > @@ -199,20 +199,24 @@ group_def > group_def: > PE_NAME '{' events '}' > { > - struct list_head *list = $3; > + struct list_head *new_head, *list = $3; > > inc_group_count(list, _parse_state); > - parse_events__set_leader($1, list, _parse_state); > + new_head = parse_events__set_leader($1, list, _parse_state); > + if (new_head) > + list_move(new_head, list);
why not put these last 2 lines to parse_events__set_leader as well?
> free($1); > $$ = list; > } > | > '{' events '}' > { > - struct list_head *list = $2; > + struct list_head *new_head, *list = $2; > > inc_group_count(list, _parse_state); > - parse_events__set_leader(NULL, list, _parse_state); > + new_head = parse_events__set_leader(NULL, list, _parse_state); > + if (new_head) > + list_move(new_head, list);
same
thanks, jirka
| |