Messages in this thread Patch in this message | | | Subject | Re: [PATCH] perf pmu: Fix alias matching | From | "Jin, Yao" <> | Date | Wed, 21 Jul 2021 11:07:59 +0800 |
| |
Hi Garry,
On 7/20/2021 11:10 PM, John Garry wrote: > Commit c47a5599eda32 ("perf tools: Fix pattern matching for same substring > in different PMU type"), may have fixed some alias matching, but has broken > some others. > > Firstly it cannot handle the simple scenario of PMU name in form > pmu_name{digits} - it can only handle pmu_name_{digits}. > > Secondly it cannot handle more complex matching in the case where we have > multiple tokens. In this scenario, the code failed to realise that we > may examine multiple substrings in the PMU name. > > Fix in two ways: > - Change perf_pmu__valid_suffix() to accept a PMU name without '_' in the > suffix > - Only pay attention to perf_pmu__valid_suffix() for the final token > > Also add const qualifiers as necessary to avoid casting. > > 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.
For my test, your patch works, thanks! :)
> 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.
> 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. :)
Thanks Jin Yao
| |