LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: John Garry <john.garry@huawei.com>
To: "Jin, Yao" <yao.jin@linux.intel.com>, <peterz@infradead.org>,
	<mingo@redhat.com>, <acme@kernel.org>, <mark.rutland@arm.com>,
	<jolsa@redhat.com>, <namhyung@kernel.org>, <kjain@linux.ibm.com>,
	<alexander.shishkin@linux.intel.com>, <irogers@google.com>
Cc: <linux-perf-users@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] perf pmu: Fix alias matching
Date: Wed, 21 Jul 2021 08:37:38 +0100	[thread overview]
Message-ID: <0752a2b1-4770-4614-3b3f-73752a7d962a@huawei.com> (raw)
In-Reply-To: <0b57fa9b-fba4-8143-bef6-b7c4f2987635@linux.intel.com>

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

  reply	other threads:[~2021-07-21  7:41 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-20 15:10 John Garry
2021-07-21  3:07 ` Jin, Yao
2021-07-21  7:37   ` John Garry [this message]
2021-07-27 10:32   ` John Garry
2021-07-27 16:25     ` Arnaldo Carvalho de Melo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0752a2b1-4770-4614-3b3f-73752a7d962a@huawei.com \
    --to=john.garry@huawei.com \
    --cc=acme@kernel.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=irogers@google.com \
    --cc=jolsa@redhat.com \
    --cc=kjain@linux.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    --cc=yao.jin@linux.intel.com \
    --subject='Re: [PATCH] perf pmu: Fix alias matching' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).