From: "Jin, Yao" <yao.jin@linux.intel.com>
To: John Garry <john.garry@huawei.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 11:07:59 +0800 [thread overview]
Message-ID: <0b57fa9b-fba4-8143-bef6-b7c4f2987635@linux.intel.com> (raw)
In-Reply-To: <1626793819-79090-1-git-send-email-john.garry@huawei.com>
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
next prev parent reply other threads:[~2021-07-21 3:09 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-20 15:10 [PATCH] perf pmu: Fix alias matching John Garry
2021-07-21 3:07 ` Jin, Yao [this message]
2021-07-21 7:37 ` John Garry
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=0b57fa9b-fba4-8143-bef6-b7c4f2987635@linux.intel.com \
--to=yao.jin@linux.intel.com \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=irogers@google.com \
--cc=john.garry@huawei.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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).