Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3 12/27] perf parse-events: Support no alias assigned event inside hybrid PMU | From | "Jin, Yao" <> | Date | Thu, 15 Apr 2021 21:36:16 +0800 |
| |
Hi Jiri,
On 4/15/2021 7:03 PM, Jiri Olsa wrote: > 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 >
OK.
>> >> 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; >
OK.
> 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? >
Yes, it should have only one term in head_config.
Now I change the code to:
+ if (!parse_state->fake_pmu && head_config && !found && + perf_pmu__is_hybrid(name)) { + struct parse_events_term *term; + + term = list_first_entry(head_config, struct parse_events_term, + list); + if (term->config) { + return parse_events__with_hybrid_pmu(parse_state, + term->config, + name, list); + } + }
> also we already know the perf_pmu__is_hybrid(name) is true, > so can't we just call: > > return parse_events__with_hybrid_pmu(....) > >
Yes, we can direct 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? >
Hmm, it's not necessary in new code, sorry about that.
>> + >> *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? >
Do you suggest we directly call __parse_events()?
int __parse_events(struct evlist *evlist, const char *str, struct parse_events_error *err, struct perf_pmu *fake_pmu)
struct parse_events_state parse_state = { .list = LIST_HEAD_INIT(parse_state.list), .idx = evlist->core.nr_entries, .error = err, .evlist = evlist, .stoken = PE_START_EVENTS, .fake_pmu = fake_pmu, };
But for parse_events__with_hybrid_pmu, we don't have valid evlist. So if we switch to __parse_events, evlist processing may be a problem.
So could we still keep current parse_events__with_hybrid_pmu()?
> 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 ? >
Yes, the code will be changed to:
+ + if (!ret) { + if (!list_empty(&ps.list)) { + list_splice(&ps.list, list); + parse_state->idx = ps.idx; + } + } >> + } >> + } >> + >> + 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 >
OK, I will use hybrid_pmu_name in next version.
Thanks Jin Yao
> thanks, > jirka >
| |