* [PATCH v2 1/2] perf pmu: Add pmu name wildcards matching function
@ 2021-06-30 12:09 Jin Yao
2021-06-30 12:09 ` [PATCH v2 2/2] perf tools: Fix pattern matching for same substring in different pmu type Jin Yao
0 siblings, 1 reply; 4+ messages in thread
From: Jin Yao @ 2021-06-30 12:09 UTC (permalink / raw)
To: acme, jolsa, peterz, mingo, alexander.shishkin
Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao
Wildcards is supported on pmu name for dynamic pmu events
(e.g. uncore events). Now move the pmu name wildcards matching
to a new function perf_pmu__pattern_match().
Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
tools/perf/util/parse-events.y | 7 +------
tools/perf/util/pmu.c | 16 ++++++++++++++++
tools/perf/util/pmu.h | 1 +
3 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index aba12a4d488e..6d467ce745d8 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -311,12 +311,7 @@ event_pmu_name opt_pmu_config
CLEANUP_YYABORT;
while ((pmu = perf_pmu__scan(pmu)) != NULL) {
- char *name = pmu->name;
-
- if (!strncmp(name, "uncore_", 7) &&
- strncmp($1, "uncore_", 7))
- name += 7;
- if (!fnmatch(pattern, name, 0)) {
+ if (!perf_pmu__pattern_match(pmu, pattern, $1)) {
if (parse_events_copy_term_list(orig_terms, &terms))
CLEANUP_YYABORT;
if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true, false))
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 88c8ecdc60b0..96f5ff9b5440 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -17,6 +17,7 @@
#include <locale.h>
#include <regex.h>
#include <perf/cpumap.h>
+#include <fnmatch.h>
#include "debug.h"
#include "evsel.h"
#include "pmu.h"
@@ -1872,3 +1873,18 @@ bool perf_pmu__has_hybrid(void)
return !list_empty(&perf_pmu__hybrid_pmus);
}
+
+int perf_pmu__pattern_match(struct perf_pmu *pmu, char *pattern, char *tok)
+{
+ char *name = pmu->name;
+
+ if (!strncmp(name, "uncore_", 7) &&
+ strncmp(tok, "uncore_", 7)) {
+ name += 7;
+ }
+
+ if (fnmatch(pattern, name, 0))
+ return -1;
+
+ return 0;
+}
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index a790ef758171..1b08e706d6e9 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -133,5 +133,6 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
char *name);
bool perf_pmu__has_hybrid(void);
+int perf_pmu__pattern_match(struct perf_pmu *pmu, char *pattern, char *tok);
#endif /* __PMU_H */
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2 2/2] perf tools: Fix pattern matching for same substring in different pmu type
2021-06-30 12:09 [PATCH v2 1/2] perf pmu: Add pmu name wildcards matching function Jin Yao
@ 2021-06-30 12:09 ` Jin Yao
2021-06-30 19:18 ` Liang, Kan
0 siblings, 1 reply; 4+ messages in thread
From: Jin Yao @ 2021-06-30 12:09 UTC (permalink / raw)
To: acme, jolsa, peterz, mingo, alexander.shishkin
Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao
Some different pmu types may have same substring. For example,
on Icelake server, we have pmu types "uncore_imc" and
"uncore_imc_free_running". Both pmu types have substring "uncore_imc".
But the parser would wrongly think they are the same pmu type.
We enable an imc event,
perf stat -e uncore_imc/event=0xe3/ -a -- sleep 1
Perf actually expands the event to:
uncore_imc_0/event=0xe3/
uncore_imc_1/event=0xe3/
uncore_imc_2/event=0xe3/
uncore_imc_3/event=0xe3/
uncore_imc_4/event=0xe3/
uncore_imc_5/event=0xe3/
uncore_imc_6/event=0xe3/
uncore_imc_7/event=0xe3/
uncore_imc_free_running_0/event=0xe3/
uncore_imc_free_running_1/event=0xe3/
uncore_imc_free_running_3/event=0xe3/
uncore_imc_free_running_4/event=0xe3/
That's because the "uncore_imc_free_running" matches the
pattern "uncore_imc*".
Now we check that the last characters of pmu name is
'_<digit>'.
For pattern "uncore_imc*", "uncore_imc_0" is parsed ok,
but "uncore_imc_free_running_0" is failed.
Fixes: b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in dynamic pmu events")
Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
tools/perf/util/pmu.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 96f5ff9b5440..9ee123d77e6d 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -3,6 +3,7 @@
#include <linux/compiler.h>
#include <linux/string.h>
#include <linux/zalloc.h>
+#include <linux/ctype.h>
#include <subcmd/pager.h>
#include <sys/types.h>
#include <errno.h>
@@ -741,6 +742,28 @@ struct pmu_events_map *__weak pmu_events_map__find(void)
return perf_pmu__find_map(NULL);
}
+static bool perf_pmu__valid_suffix(char *tok, char *pmu_name)
+{
+ char *p;
+
+ /*
+ * The pmu_name has substring tok. If the format of
+ * pmu_name is tok or tok_digit, return true.
+ */
+ p = pmu_name + strlen(tok);
+ if (*p == 0)
+ return true;
+
+ if (*p != '_')
+ return false;
+
+ ++p;
+ if (*p == 0 || !isdigit(*p))
+ return false;
+
+ return true;
+}
+
bool pmu_uncore_alias_match(const char *pmu_name, const char *name)
{
char *tmp = NULL, *tok, *str;
@@ -769,7 +792,7 @@ bool pmu_uncore_alias_match(const char *pmu_name, const char *name)
*/
for (; tok; name += strlen(tok), tok = strtok_r(NULL, ",", &tmp)) {
name = strstr(name, tok);
- if (!name) {
+ if (!name || !perf_pmu__valid_suffix(tok, (char *)name)) {
res = false;
goto out;
}
@@ -1886,5 +1909,8 @@ int perf_pmu__pattern_match(struct perf_pmu *pmu, char *pattern, char *tok)
if (fnmatch(pattern, name, 0))
return -1;
+ if (!perf_pmu__valid_suffix(tok, name))
+ return -1;
+
return 0;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 2/2] perf tools: Fix pattern matching for same substring in different pmu type
2021-06-30 12:09 ` [PATCH v2 2/2] perf tools: Fix pattern matching for same substring in different pmu type Jin Yao
@ 2021-06-30 19:18 ` Liang, Kan
2021-07-01 1:22 ` Jin, Yao
0 siblings, 1 reply; 4+ messages in thread
From: Liang, Kan @ 2021-06-30 19:18 UTC (permalink / raw)
To: Jin Yao, acme, jolsa, peterz, mingo, alexander.shishkin
Cc: Linux-kernel, ak, kan.liang, yao.jin
On 6/30/2021 8:09 AM, Jin Yao wrote:
> Some different pmu types may have same substring. For example,
> on Icelake server, we have pmu types "uncore_imc" and
> "uncore_imc_free_running". Both pmu types have substring "uncore_imc".
> But the parser would wrongly think they are the same pmu type.
>
> We enable an imc event,
> perf stat -e uncore_imc/event=0xe3/ -a -- sleep 1
>
> Perf actually expands the event to:
> uncore_imc_0/event=0xe3/
> uncore_imc_1/event=0xe3/
> uncore_imc_2/event=0xe3/
> uncore_imc_3/event=0xe3/
> uncore_imc_4/event=0xe3/
> uncore_imc_5/event=0xe3/
> uncore_imc_6/event=0xe3/
> uncore_imc_7/event=0xe3/
> uncore_imc_free_running_0/event=0xe3/
> uncore_imc_free_running_1/event=0xe3/
> uncore_imc_free_running_3/event=0xe3/
> uncore_imc_free_running_4/event=0xe3/
>
> That's because the "uncore_imc_free_running" matches the
> pattern "uncore_imc*".
>
> Now we check that the last characters of pmu name is
> '_<digit>'.
>
> For pattern "uncore_imc*", "uncore_imc_0" is parsed ok,
> but "uncore_imc_free_running_0" is failed.
>
> Fixes: b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in dynamic pmu events")
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
> tools/perf/util/pmu.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 96f5ff9b5440..9ee123d77e6d 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -3,6 +3,7 @@
> #include <linux/compiler.h>
> #include <linux/string.h>
> #include <linux/zalloc.h>
> +#include <linux/ctype.h>
> #include <subcmd/pager.h>
> #include <sys/types.h>
> #include <errno.h>
> @@ -741,6 +742,28 @@ struct pmu_events_map *__weak pmu_events_map__find(void)
> return perf_pmu__find_map(NULL);
> }
>
> +static bool perf_pmu__valid_suffix(char *tok, char *pmu_name)
> +{
> + char *p;
> +
> + /*
> + * The pmu_name has substring tok. If the format of
The uncore PMU may have two names, e.g., uncore_cha_Y or
uncore_type_X_Y. User can use either name. I don't think we can assume
that the pmu_name has substring tok. I think we should add a check as below.
@@ -746,6 +746,8 @@ static bool perf_pmu__valid_suffix(char *tok, char
*pmu_name)
{
char *p;
+ if (strncmp(pmu_name, tok, strlen(tok)))
+ return false;
/*
* The pmu_name has substring tok. If the format of
* pmu_name is tok or tok_digit, return true.
> + * pmu_name is tok or tok_digit, return true.
> + */
> + p = pmu_name + strlen(tok);
> + if (*p == 0)
> + return true;
> +
> + if (*p != '_')
> + return false;
> +
> + ++p;
> + if (*p == 0 || !isdigit(*p))
> + return false;
> +
> + return true;
> +}
> +
> bool pmu_uncore_alias_match(const char *pmu_name, const char *name)
> {
> char *tmp = NULL, *tok, *str;
> @@ -769,7 +792,7 @@ bool pmu_uncore_alias_match(const char *pmu_name, const char *name)
> */
> for (; tok; name += strlen(tok), tok = strtok_r(NULL, ",", &tmp)) {
> name = strstr(name, tok);
> - if (!name) {
> + if (!name || !perf_pmu__valid_suffix(tok, (char *)name)) {
> res = false;
> goto out;
> }
> @@ -1886,5 +1909,8 @@ int perf_pmu__pattern_match(struct perf_pmu *pmu, char *pattern, char *tok)
> if (fnmatch(pattern, name, 0))
> return -1;
>
> + if (!perf_pmu__valid_suffix(tok, name))
> + return -1;
> +
They are still two functions. I'm wondering if we can merge the two
functions to one function, e.g., perf_pmu_match()?
So my patch just need to simply do
if (!perf_pmu_match(tok, name) && !perf_pmu_match(tok,
pmu->alias_name)) return -1;
Thanks,
Kan
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 2/2] perf tools: Fix pattern matching for same substring in different pmu type
2021-06-30 19:18 ` Liang, Kan
@ 2021-07-01 1:22 ` Jin, Yao
0 siblings, 0 replies; 4+ messages in thread
From: Jin, Yao @ 2021-07-01 1:22 UTC (permalink / raw)
To: Liang, Kan, acme, jolsa, peterz, mingo, alexander.shishkin
Cc: Linux-kernel, ak, kan.liang, yao.jin
Hi Kan,
On 7/1/2021 3:18 AM, Liang, Kan wrote:
>
>
> On 6/30/2021 8:09 AM, Jin Yao wrote:
>> Some different pmu types may have same substring. For example,
>> on Icelake server, we have pmu types "uncore_imc" and
>> "uncore_imc_free_running". Both pmu types have substring "uncore_imc".
>> But the parser would wrongly think they are the same pmu type.
>>
>> We enable an imc event,
>> perf stat -e uncore_imc/event=0xe3/ -a -- sleep 1
>>
>> Perf actually expands the event to:
>> uncore_imc_0/event=0xe3/
>> uncore_imc_1/event=0xe3/
>> uncore_imc_2/event=0xe3/
>> uncore_imc_3/event=0xe3/
>> uncore_imc_4/event=0xe3/
>> uncore_imc_5/event=0xe3/
>> uncore_imc_6/event=0xe3/
>> uncore_imc_7/event=0xe3/
>> uncore_imc_free_running_0/event=0xe3/
>> uncore_imc_free_running_1/event=0xe3/
>> uncore_imc_free_running_3/event=0xe3/
>> uncore_imc_free_running_4/event=0xe3/
>>
>> That's because the "uncore_imc_free_running" matches the
>> pattern "uncore_imc*".
>>
>> Now we check that the last characters of pmu name is
>> '_<digit>'.
>>
>> For pattern "uncore_imc*", "uncore_imc_0" is parsed ok,
>> but "uncore_imc_free_running_0" is failed.
>>
>> Fixes: b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in dynamic pmu events")
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>> tools/perf/util/pmu.c | 28 +++++++++++++++++++++++++++-
>> 1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 96f5ff9b5440..9ee123d77e6d 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -3,6 +3,7 @@
>> #include <linux/compiler.h>
>> #include <linux/string.h>
>> #include <linux/zalloc.h>
>> +#include <linux/ctype.h>
>> #include <subcmd/pager.h>
>> #include <sys/types.h>
>> #include <errno.h>
>> @@ -741,6 +742,28 @@ struct pmu_events_map *__weak pmu_events_map__find(void)
>> return perf_pmu__find_map(NULL);
>> }
>> +static bool perf_pmu__valid_suffix(char *tok, char *pmu_name)
>> +{
>> + char *p;
>> +
>> + /*
>> + * The pmu_name has substring tok. If the format of
>
> The uncore PMU may have two names, e.g., uncore_cha_Y or uncore_type_X_Y. User can use either name.
> I don't think we can assume that the pmu_name has substring tok. I think we should add a check as
> below.
>
>
> @@ -746,6 +746,8 @@ static bool perf_pmu__valid_suffix(char *tok, char *pmu_name)
> {
> char *p;
>
> + if (strncmp(pmu_name, tok, strlen(tok)))
> + return false;
> /*
> * The pmu_name has substring tok. If the format of
> * pmu_name is tok or tok_digit, return true.
>
Before calling perf_pmu__valid_suffix(), we either called the fnmatch() or called strstr(), so the
tok must be the substring of pmu_name.
>> + * pmu_name is tok or tok_digit, return true.
>> + */
>> + p = pmu_name + strlen(tok);
>> + if (*p == 0)
>> + return true;
>> +
>> + if (*p != '_')
>> + return false;
>> +
>> + ++p;
>> + if (*p == 0 || !isdigit(*p))
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> bool pmu_uncore_alias_match(const char *pmu_name, const char *name)
>> {
>> char *tmp = NULL, *tok, *str;
>> @@ -769,7 +792,7 @@ bool pmu_uncore_alias_match(const char *pmu_name, const char *name)
>> */
>> for (; tok; name += strlen(tok), tok = strtok_r(NULL, ",", &tmp)) {
>> name = strstr(name, tok);
>> - if (!name) {
>> + if (!name || !perf_pmu__valid_suffix(tok, (char *)name)) {
>> res = false;
>> goto out;
>> }
>> @@ -1886,5 +1909,8 @@ int perf_pmu__pattern_match(struct perf_pmu *pmu, char *pattern, char *tok)
>> if (fnmatch(pattern, name, 0))
>> return -1;
>> + if (!perf_pmu__valid_suffix(tok, name))
>> + return -1;
>> +
>
> They are still two functions. I'm wondering if we can merge the two functions to one function, e.g.,
> perf_pmu_match()?
>
Sorry, why do you say they are still two functions? Is it because fnmatch + perf_pmu__valid_suffix?
But as what I explained before, we can't use fnmatch to match the pattern such as "[tok]_[digit]",
we have to use an function to check the last characters for '_' and digits.
Or I still misunderstand for the two functions here?
> So my patch just need to simply do
> if (!perf_pmu_match(tok, name) && !perf_pmu_match(tok, pmu->alias_name)) return -1;
>
I see your patch is using:
(https://lore.kernel.org/lkml/1624990443-168533-7-git-send-email-kan.liang@linux.intel.com/)
if (!fnmatch(pattern, name, 0) ||
(pmu->alias_name && !fnmatch(pattern, pmu->alias_name, 0))) {
}
So change the lines to:
if (!perf_pmu__match(pattern, name, NULL) ||
(pmu->alias_name && !perf_pmu__match(pattern, pmu->alias_name, NULL))) {
}
int perf_pmu__match(char *pattern, char *name, char *tok)
{
if (fnmatch(pattern, name, 0))
return -1;
if (tok && !perf_pmu__valid_suffix(tok, name))
return -1;
return 0;
}
Is that OK?
Thanks
Jin Yao
> Thanks,
> Kan
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-07-01 1:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-30 12:09 [PATCH v2 1/2] perf pmu: Add pmu name wildcards matching function Jin Yao
2021-06-30 12:09 ` [PATCH v2 2/2] perf tools: Fix pattern matching for same substring in different pmu type Jin Yao
2021-06-30 19:18 ` Liang, Kan
2021-07-01 1:22 ` Jin, Yao
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).