[PATCH] perf pmu: Fix alias matching

[PATCH] perf pmu: Fix alias matching

[John Garry]

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

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;
+	}
 
 	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;
-- 
2.26.2

Re: [PATCH] perf pmu: Fix alias matching

[Jin, Yao]

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

Re: [PATCH] perf pmu: Fix alias matching

[John Garry]

>>
>> 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

Re: [PATCH] perf pmu: Fix alias matching

[John Garry]

On 21/07/2021 04:07, Jin, Yao wrote:
>>

Hi Arnaldo,

Can you kindly consider picking up this patch?

Thanks

>> 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! :)

Can we take this as a tested-by?

Re: [PATCH] perf pmu: Fix alias matching

[Arnaldo Carvalho de Melo]

Em Tue, Jul 27, 2021 at 11:32:55AM +0100, John Garry escreveu:
> On 21/07/2021 04:07, Jin, Yao wrote:
> > > 
> 
> Hi Arnaldo,
> 
> Can you kindly consider picking up this patch?
> 
> Thanks
> 
> > > 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! :)
> 
> Can we take this as a tested-by?

Processed and added his Tested-by.

- Arnaldo