Messages in this thread | | | Subject | Re: [PATCH v1] perf tools: Fix pattern matching for same substring used in different pmu type | From | "Jin, Yao" <> | Date | Wed, 23 Jun 2021 10:02:01 +0800 |
| |
Hi Arnaldo, Jiri,
Any comments for this bug fix patch?
The issue does impact some uncore events and even some metrics.
Thanks Jin Yao
On 6/11/2021 10:54 AM, Jin, Yao wrote: > And, though this patch is to fix the uncore_imc/uncore_imc_free_running mismatching issue. We are > also surprised to see it can solve another hybrid PMU issue on Alderlake. > > On Alderlake, > > # ./perf stat -e cpu/event=0xc2,umask=0x2/ true > > Performance counter stats for 'true': > > 702,246 cpu_core/event=0xc2,umask=0x2/ > <not counted> cpu_atom/event=0xc2,umask=0x2/ > > It should error out with the wrong PMU rather than using 'cpu_core' and'cpu_atom' instead. > > This is still the pattern matching issue. The pattern is "cpu*". Both "cpu_core" and "cpu_atom" can > match the pattern "cpu*", so the parser wrongly thinks they are the same PMU type. > > Now with this patch, > > # ./perf stat -e cpu/cpu-cycles/ true > event syntax error: 'cpu/cpu-cycles/' > \___ Cannot find PMU `cpu'. Missing kernel support? > Run 'perf list' for a list of valid events > > Thanks > Jin Yao > > On 6/9/2021 12:57 PM, Jin Yao wrote: >> Some different pmu types may have same substring. For example, >> on Icelake server, we have pmu types "uncore_imc" and >> "uncore_imc_free_running". Both pmu types have substring "uncore_imc". >> But the parser would wrongly think they are the same pmu type. >> >> We enable an imc event, >> perf stat -e uncore_imc/event=0xe3/ -a -- sleep 1 >> >> Perf actually expands the event to: >> uncore_imc_0/event=0xe3/ >> uncore_imc_1/event=0xe3/ >> uncore_imc_2/event=0xe3/ >> uncore_imc_3/event=0xe3/ >> uncore_imc_4/event=0xe3/ >> uncore_imc_5/event=0xe3/ >> uncore_imc_6/event=0xe3/ >> uncore_imc_7/event=0xe3/ >> uncore_imc_free_running_0/event=0xe3/ >> uncore_imc_free_running_1/event=0xe3/ >> uncore_imc_free_running_3/event=0xe3/ >> uncore_imc_free_running_4/event=0xe3/ >> >> That's because the "uncore_imc_free_running" matches the >> pattern "uncore_imc*". >> >> Now we check that the last characters of pmu name is >> '_<digit>'. >> >> Fixes: b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in dynamic pmu events") >> Signed-off-by: Jin Yao <yao.jin@linux.intel.com> >> --- >> tools/perf/util/parse-events.y | 2 ++ >> tools/perf/util/pmu.c | 25 ++++++++++++++++++++++++- >> tools/perf/util/pmu.h | 1 + >> 3 files changed, 27 insertions(+), 1 deletion(-) >> >> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y >> index aba12a4d488e..7a694c7f7f1a 100644 >> --- a/tools/perf/util/parse-events.y >> +++ b/tools/perf/util/parse-events.y >> @@ -317,6 +317,8 @@ event_pmu_name opt_pmu_config >> strncmp($1, "uncore_", 7)) >> name += 7; >> if (!fnmatch(pattern, name, 0)) { >> + if (!perf_pmu__valid_suffix($1, name)) >> + continue; >> if (parse_events_copy_term_list(orig_terms, &terms)) >> CLEANUP_YYABORT; >> if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true, false)) >> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c >> index 88c8ecdc60b0..78af01959830 100644 >> --- a/tools/perf/util/pmu.c >> +++ b/tools/perf/util/pmu.c >> @@ -3,6 +3,7 @@ >> #include <linux/compiler.h> >> #include <linux/string.h> >> #include <linux/zalloc.h> >> +#include <linux/ctype.h> >> #include <subcmd/pager.h> >> #include <sys/types.h> >> #include <errno.h> >> @@ -768,7 +769,7 @@ bool pmu_uncore_alias_match(const char *pmu_name, const char *name) >> */ >> for (; tok; name += strlen(tok), tok = strtok_r(NULL, ",", &tmp)) { >> name = strstr(name, tok); >> - if (!name) { >> + if (!name || !perf_pmu__valid_suffix(tok, (char *)name)) { >> res = false; >> goto out; >> } >> @@ -1872,3 +1873,25 @@ bool perf_pmu__has_hybrid(void) >> return !list_empty(&perf_pmu__hybrid_pmus); >> } >> + >> +bool perf_pmu__valid_suffix(char *tok, char *pmu_name) >> +{ >> + char *p; >> + >> + /* >> + * The pmu_name has substring tok. If the format of >> + * pmu_name is <tok> or <tok>_<digit>, return true. >> + */ >> + p = pmu_name + strlen(tok); >> + if (*p == 0) >> + return true; >> + >> + if (*p != '_') >> + return false; >> + >> + ++p; >> + if (*p == 0 || !isdigit(*p)) >> + return false; >> + >> + return true; >> +} >> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h >> index a790ef758171..ebfd2b71532b 100644 >> --- a/tools/perf/util/pmu.h >> +++ b/tools/perf/util/pmu.h >> @@ -133,5 +133,6 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config, >> char *name); >> bool perf_pmu__has_hybrid(void); >> +bool perf_pmu__valid_suffix(char *tok, char *pmu_name); >> #endif /* __PMU_H */ >>
| |