Messages in this thread | | | Subject | Re: [PATCH] perf pmu: Fix alias matching | From | John Garry <> | Date | Wed, 21 Jul 2021 08:37:38 +0100 |
| |
>> >> Fixes: c47a5599eda3 ("perf tools: Fix pattern matching for same >> substring in different PMU type") >> Signed-off-by: John Garry <john.garry@huawei.com> >> --- >> @Jin Yao, please test for your scenarios >> > > For x86, the form uncore_pmu_{digits} or the uncore_pmu itself are > supported. We don't have more complex case such as the name in the form > aaa_bbbX_cccY. So my test didn't cover that complex form. >
My next thing to do is to add support for these more complex scenarios in the PMU events self tests
> For my test, your patch works, thanks! :)
Good
> >> Note: >> About any effect in perf_pmu__match() -> perf_pmu__valid_suffix() >> callchain, this seems to be called for wildcard in PMU names in metric >> expressions. We don't have any metrics for arm64 which use feature. >> However, I hacked an existing metric to use a wildcard and it looks ok. >> Also the "DRAM_BW_Use" metric on my broadwell uses this feature, and it >> looks ok. >> >> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c >> index a1bd7007a8b4..fc683bc41715 100644 >> --- a/tools/perf/util/pmu.c >> +++ b/tools/perf/util/pmu.c >> @@ -742,9 +742,13 @@ struct pmu_events_map *__weak >> pmu_events_map__find(void) >> return perf_pmu__find_map(NULL); >> } >> -static bool perf_pmu__valid_suffix(char *pmu_name, char *tok) >> +/* >> + * Suffix must be in form tok_{digits}, or tok{digits}, or same as >> pmu_name >> + * to be valid. >> + */ >> +static bool perf_pmu__valid_suffix(const char *pmu_name, char *tok) >> { >> - char *p; >> + const char *p; >> if (strncmp(pmu_name, tok, strlen(tok))) >> return false; >> @@ -753,12 +757,16 @@ static bool perf_pmu__valid_suffix(char >> *pmu_name, char *tok) >> if (*p == 0) >> return true; >> - if (*p != '_') >> - return false; >> + if (*p == '_') >> + ++p; >> - ++p; >> - if (*p == 0 || !isdigit(*p)) >> - return false; >> + /* Ensure we end in a number */ >> + while (1) { >> + if (!isdigit(*p)) >> + return false; >> + if (*(++p) == 0) >> + break; >> + } > > Do we check *p before first isdigit? For example, > > if (*p == 0) > return false; > > While (*p) { > if (!isdigit(*p) > return false; > ++p; > } > > But maybe isdigit can handle the null string well. I'm just feeling a > bit unsure. >
isdigit() can safely handle 0 and returns 0 for that case, so what I added should be ok
>> return true; >> } >> @@ -789,12 +797,19 @@ bool pmu_uncore_alias_match(const char >> *pmu_name, const char *name) >> * match "socket" in "socketX_pmunameY" and then >> "pmuname" in >> * "pmunameY". >> */ >> - for (; tok; name += strlen(tok), tok = strtok_r(NULL, ",", &tmp)) { >> + while (1) { >> + char *next_tok = strtok_r(NULL, ",", &tmp); >> + >> name = strstr(name, tok); >> - if (!name || !perf_pmu__valid_suffix((char *)name, tok)) { >> + if (!name || >> + (!next_tok && !perf_pmu__valid_suffix(name, tok))) { >> res = false; >> goto out; >> } >> + if (!next_tok) >> + break; >> + tok = next_tok; >> + name += strlen(tok); >> } >> res = true; >> > > My test didn't cover the tokens which were delimited by ','. I assume > you have tested that on arm64 system. :) >
Right Thanks, John
| |