* [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).