Messages in this thread Patch in this message | | | Date | Thu, 15 Apr 2021 13:03:39 +0200 | From | Jiri Olsa <> | Subject | Re: [PATCH v3 12/27] perf parse-events: Support no alias assigned event inside hybrid PMU |
| |
On Mon, Mar 29, 2021 at 03:00:31PM +0800, Jin Yao wrote:
SNIP
> --- > v3: > - Rename the patch: > 'perf parse-events: Support hardware events inside PMU' --> > 'perf parse-events: Support no alias assigned event inside hybrid PMU' > > - Major code is moved to parse-events-hybrid.c. > - Refine the code. > > tools/perf/util/parse-events-hybrid.c | 18 +++++- > tools/perf/util/parse-events-hybrid.h | 3 +- > tools/perf/util/parse-events.c | 80 +++++++++++++++++++++++++-- > tools/perf/util/parse-events.h | 4 +- > tools/perf/util/parse-events.y | 9 ++- > tools/perf/util/pmu.c | 4 +- > tools/perf/util/pmu.h | 2 +- > 7 files changed, 108 insertions(+), 12 deletions(-)
please move the support to pass pmu_name and filter on it within hybrid code in to separate patch
> > diff --git a/tools/perf/util/parse-events-hybrid.c b/tools/perf/util/parse-events-hybrid.c > index 8a630cbab8f3..5bf176b55573 100644 > --- a/tools/perf/util/parse-events-hybrid.c > +++ b/tools/perf/util/parse-events-hybrid.c > @@ -64,6 +64,11 @@ static int add_hw_hybrid(struct parse_events_state *parse_state, > int ret; > > perf_pmu__for_each_hybrid_pmu(pmu) { > + if (parse_state->pmu_name && > + strcmp(parse_state->pmu_name, pmu->name)) { > + continue;
please add this check to separate function
if (pmu_cmp(parse_stat)) continue;
SNIP
> + if (!parse_state->fake_pmu && head_config && !found && > + perf_pmu__is_hybrid(name)) { > + struct parse_events_term *term; > + int ret; > + > + list_for_each_entry(term, head_config, list) { > + if (!term->config) > + continue; > + > + ret = parse_events__with_hybrid_pmu(parse_state, > + term->config, > + name, &found, > + list); > + if (found) > + return ret;
what if there are more terms in head_config? should we make sure there's just one term and fail if there's more?
also we already know the perf_pmu__is_hybrid(name) is true, so can't we just call:
return parse_events__with_hybrid_pmu(....)
> + } > + } > > if (verbose > 1) { > fprintf(stderr, "After aliases, add event pmu '%s' with '", > @@ -1605,6 +1630,15 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state, > struct perf_pmu *pmu = NULL; > int ok = 0; > > + if (parse_state->pmu_name) { > + list = malloc(sizeof(struct list_head)); > + if (!list) > + return -1; > + INIT_LIST_HEAD(list); > + *listp = list; > + return 0; > + }
hum, why is this needed?
> + > *listp = NULL; > /* Add it for all PMUs that support the alias */ > list = malloc(sizeof(struct list_head)); > @@ -2176,6 +2210,44 @@ int parse_events_terms(struct list_head *terms, const char *str) > return ret; > } > > +static int list_entries_nr(struct list_head *list) > +{ > + struct list_head *pos; > + int n = 0; > + > + list_for_each(pos, list) > + n++; > + > + return n; > +} > + > +static int parse_events__with_hybrid_pmu(struct parse_events_state *parse_state, > + const char *str, char *pmu_name, > + bool *found, struct list_head *list) > +{ > + struct parse_events_state ps = { > + .list = LIST_HEAD_INIT(ps.list), > + .stoken = PE_START_EVENTS, > + .pmu_name = pmu_name, > + .idx = parse_state->idx, > + };
could we add this pmu_name directly to __parse_events?
it duplicates the code plus there are some extra checks you don't do in here and which might be needed, like last->cmdline_group_boundary setup
> + int ret; > + > + *found = false; > + ret = parse_events__scanner(str, &ps); > + perf_pmu__parse_cleanup(); > + > + if (!ret) { > + if (!list_empty(&ps.list)) { > + *found = true; > + list_splice(&ps.list, list); > + parse_state->idx = list_entries_nr(list);
could you just use ps.idx instead of list_entries_nr ?
> + } > + } > + > + return ret; > +} > + > int __parse_events(struct evlist *evlist, const char *str, > struct parse_events_error *err, struct perf_pmu *fake_pmu) > { > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h > index c4f2f96304ce..f9d8e8e41c38 100644 > --- a/tools/perf/util/parse-events.h > +++ b/tools/perf/util/parse-events.h > @@ -138,6 +138,7 @@ struct parse_events_state { > struct list_head *terms; > int stoken; > struct perf_pmu *fake_pmu; > + char *pmu_name;
so it's hybrid specific, we should name it like hybrid_pmu_name or such
thanks, jirka
| |