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 | Mon, 12 Apr 2021 10:51:14 +0800 |
| |
Hi Jiri,
On 4/9/2021 9:47 PM, Jiri Olsa wrote: > On Mon, Mar 29, 2021 at 03:00:31PM +0800, Jin Yao wrote: > > SNIP > >> + struct parse_events_state *parse_state) >> { >> struct perf_event_attr attr; >> LIST_HEAD(config_terms); >> @@ -521,7 +526,7 @@ int parse_events_add_cache(struct list_head *list, int *idx, >> >> i = parse_events__add_cache_hybrid(list, idx, &attr, >> config_name ? : name, &config_terms, >> - &hybrid); >> + &hybrid, parse_state); >> if (hybrid) >> return i; >> >> @@ -1481,7 +1486,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state, >> struct perf_pmu *pmu; >> struct evsel *evsel; >> struct parse_events_error *err = parse_state->error; >> - bool use_uncore_alias; >> + bool use_uncore_alias, found = false; >> LIST_HEAD(config_terms); >> >> if (verbose > 1) { >> @@ -1530,8 +1535,28 @@ int parse_events_add_pmu(struct parse_events_state *parse_state, >> } >> } >> >> - if (!parse_state->fake_pmu && perf_pmu__check_alias(pmu, head_config, &info)) >> + if (!parse_state->fake_pmu && >> + perf_pmu__check_alias(pmu, head_config, &info, &found)) { >> return -EINVAL; >> + } >> + > > ok, let's not polute surronding functions and make strict check > on what we want in here.. we are after following events: > > cpu_xxx/L1-dcache/ > cpu_xxx/l1-d|/ > ... > right? >
Yes, we only focus on the cache events now.
> so we are after events with single term in head_config that has name in: > > L1-dcache|l1-d|l1d|L1-data | > L1-icache|l1-i|l1i|L1-instruction | > LLC|L2 | > dTLB|d-tlb|Data-TLB | > iTLB|i-tlb|Instruction-TLB | > branch|branches|bpu|btb|bpc | > node > > I think that with such direct check the code will be more straight > forward, also let's move it to parse-events-hybrid >
Do you suggest we just use string comparison for doing the direct check?
e.g.
if (strstr(term->config, "L1-dcache")) ...
Of course, we can define a string array first and use a loop for string comparison.
>> + 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); > > do we need to call the parsing again? could we just call > parse_events__add_cache_hybrid? > > jirka > >
If we do the direct check for cache events, I think we don't need the parsing again.
As I mentioned above, we need to define a string array and compare with term->config one by one.
I'm OK for this solution. :)
Thanks Jin Yao
>> + if (found) >> + return ret; >> + } >> + } >> >> 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; >> > > SNIP >
| |