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>'.
Fixes: b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in dynamic pmu events")
Signed-off-by: Jin Yao <[email protected]>
---
tools/perf/util/parse-events.y | 2 ++
tools/perf/util/pmu.c | 25 ++++++++++++++++++++++++-
tools/perf/util/pmu.h | 1 +
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index aba12a4d488e..7a694c7f7f1a 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -317,6 +317,8 @@ event_pmu_name opt_pmu_config
strncmp($1, "uncore_", 7))
name += 7;
if (!fnmatch(pattern, name, 0)) {
+ if (!perf_pmu__valid_suffix($1, name))
+ continue;
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..78af01959830 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>
@@ -768,7 +769,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;
}
@@ -1872,3 +1873,25 @@ bool perf_pmu__has_hybrid(void)
return !list_empty(&perf_pmu__hybrid_pmus);
}
+
+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;
+}
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index a790ef758171..ebfd2b71532b 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);
+bool perf_pmu__valid_suffix(char *tok, char *pmu_name);
#endif /* __PMU_H */
--
2.17.1
And, though this patch is to fix the uncore_imc/uncore_imc_free_running mismatching issue. We are
also surprised to see it can solve another hybrid PMU issue on Alderlake.
On Alderlake,
# ./perf stat -e cpu/event=0xc2,umask=0x2/ true
Performance counter stats for 'true':
702,246 cpu_core/event=0xc2,umask=0x2/
<not counted> cpu_atom/event=0xc2,umask=0x2/
It should error out with the wrong PMU rather than using 'cpu_core' and'cpu_atom' instead.
This is still the pattern matching issue. The pattern is "cpu*". Both "cpu_core" and "cpu_atom" can
match the pattern "cpu*", so the parser wrongly thinks they are the same PMU type.
Now with this patch,
# ./perf stat -e cpu/cpu-cycles/ true
event syntax error: 'cpu/cpu-cycles/'
\___ Cannot find PMU `cpu'. Missing kernel support?
Run 'perf list' for a list of valid events
Thanks
Jin Yao
On 6/9/2021 12:57 PM, 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>'.
>
> Fixes: b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in dynamic pmu events")
> Signed-off-by: Jin Yao <[email protected]>
> ---
> tools/perf/util/parse-events.y | 2 ++
> tools/perf/util/pmu.c | 25 ++++++++++++++++++++++++-
> tools/perf/util/pmu.h | 1 +
> 3 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index aba12a4d488e..7a694c7f7f1a 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -317,6 +317,8 @@ event_pmu_name opt_pmu_config
> strncmp($1, "uncore_", 7))
> name += 7;
> if (!fnmatch(pattern, name, 0)) {
> + if (!perf_pmu__valid_suffix($1, name))
> + continue;
> 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..78af01959830 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>
> @@ -768,7 +769,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;
> }
> @@ -1872,3 +1873,25 @@ bool perf_pmu__has_hybrid(void)
>
> return !list_empty(&perf_pmu__hybrid_pmus);
> }
> +
> +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;
> +}
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index a790ef758171..ebfd2b71532b 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);
> +bool perf_pmu__valid_suffix(char *tok, char *pmu_name);
>
> #endif /* __PMU_H */
>
Hi Arnaldo, Jiri,
Any comments for this bug fix patch?
The issue does impact some uncore events and even some metrics.
Thanks
Jin Yao
On 6/11/2021 10:54 AM, Jin, Yao wrote:
> And, though this patch is to fix the uncore_imc/uncore_imc_free_running mismatching issue. We are
> also surprised to see it can solve another hybrid PMU issue on Alderlake.
>
> On Alderlake,
>
> # ./perf stat -e cpu/event=0xc2,umask=0x2/ true
>
> Performance counter stats for 'true':
>
> 702,246 cpu_core/event=0xc2,umask=0x2/
> <not counted> cpu_atom/event=0xc2,umask=0x2/
>
> It should error out with the wrong PMU rather than using 'cpu_core' and'cpu_atom' instead.
>
> This is still the pattern matching issue. The pattern is "cpu*". Both "cpu_core" and "cpu_atom" can
> match the pattern "cpu*", so the parser wrongly thinks they are the same PMU type.
>
> Now with this patch,
>
> # ./perf stat -e cpu/cpu-cycles/ true
> event syntax error: 'cpu/cpu-cycles/'
> \___ Cannot find PMU `cpu'. Missing kernel support?
> Run 'perf list' for a list of valid events
>
> Thanks
> Jin Yao
>
> On 6/9/2021 12:57 PM, 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>'.
>>
>> Fixes: b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in dynamic pmu events")
>> Signed-off-by: Jin Yao <[email protected]>
>> ---
>> tools/perf/util/parse-events.y | 2 ++
>> tools/perf/util/pmu.c | 25 ++++++++++++++++++++++++-
>> tools/perf/util/pmu.h | 1 +
>> 3 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
>> index aba12a4d488e..7a694c7f7f1a 100644
>> --- a/tools/perf/util/parse-events.y
>> +++ b/tools/perf/util/parse-events.y
>> @@ -317,6 +317,8 @@ event_pmu_name opt_pmu_config
>> strncmp($1, "uncore_", 7))
>> name += 7;
>> if (!fnmatch(pattern, name, 0)) {
>> + if (!perf_pmu__valid_suffix($1, name))
>> + continue;
>> 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..78af01959830 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>
>> @@ -768,7 +769,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;
>> }
>> @@ -1872,3 +1873,25 @@ bool perf_pmu__has_hybrid(void)
>> return !list_empty(&perf_pmu__hybrid_pmus);
>> }
>> +
>> +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;
>> +}
>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>> index a790ef758171..ebfd2b71532b 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);
>> +bool perf_pmu__valid_suffix(char *tok, char *pmu_name);
>> #endif /* __PMU_H */
>>
On Wed, Jun 23, 2021 at 10:02:01AM +0800, Jin, Yao wrote:
> Hi Arnaldo, Jiri,
>
> Any comments for this bug fix patch?
>
> The issue does impact some uncore events and even some metrics.
sry for delay
SNIP
> > > 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>'.
> > >
> > > Fixes: b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in dynamic pmu events")
> > > Signed-off-by: Jin Yao <[email protected]>
> > > ---
> > > ? tools/perf/util/parse-events.y |? 2 ++
> > > ? tools/perf/util/pmu.c????????? | 25 ++++++++++++++++++++++++-
> > > ? tools/perf/util/pmu.h????????? |? 1 +
> > > ? 3 files changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > > index aba12a4d488e..7a694c7f7f1a 100644
> > > --- a/tools/perf/util/parse-events.y
> > > +++ b/tools/perf/util/parse-events.y
> > > @@ -317,6 +317,8 @@ event_pmu_name opt_pmu_config
> > > ????????????????? strncmp($1, "uncore_", 7))
> > > ????????????????? name += 7;
> > > ????????????? if (!fnmatch(pattern, name, 0)) {
> > > +??????????????? if (!perf_pmu__valid_suffix($1, name))
> > > +??????????????????? continue;
could this be part of the fnmatch's pattern?
> > > ????????????????? 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..78af01959830 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>
> > > @@ -768,7 +769,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;
> > > ????????? }
> > > @@ -1872,3 +1873,25 @@ bool perf_pmu__has_hybrid(void)
> > > ????? return !list_empty(&perf_pmu__hybrid_pmus);
> > > ? }
> > > +
> > > +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;
> > > +}
hum, so we have pattern serch and then another function checking
if that search was ok.. I understand that's convenient, because
it's on 2 different places, but could we have some generic solution,
line one function/search that returns/search for valid pmu name?
thanks,
jirka
Hi Jiri,
On 6/25/2021 6:11 PM, Jiri Olsa wrote:
> On Wed, Jun 23, 2021 at 10:02:01AM +0800, Jin, Yao wrote:
>> Hi Arnaldo, Jiri,
>>
>> Any comments for this bug fix patch?
>>
>> The issue does impact some uncore events and even some metrics.
>
> sry for delay
>
> SNIP
>
>>>> 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>'.
>>>>
>>>> Fixes: b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in dynamic pmu events")
>>>> Signed-off-by: Jin Yao <[email protected]>
>>>> ---
>>>> tools/perf/util/parse-events.y | 2 ++
>>>> tools/perf/util/pmu.c | 25 ++++++++++++++++++++++++-
>>>> tools/perf/util/pmu.h | 1 +
>>>> 3 files changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
>>>> index aba12a4d488e..7a694c7f7f1a 100644
>>>> --- a/tools/perf/util/parse-events.y
>>>> +++ b/tools/perf/util/parse-events.y
>>>> @@ -317,6 +317,8 @@ event_pmu_name opt_pmu_config
>>>> strncmp($1, "uncore_", 7))
>>>> name += 7;
>>>> if (!fnmatch(pattern, name, 0)) {
>>>> + if (!perf_pmu__valid_suffix($1, name))
>>>> + continue;
>
> could this be part of the fnmatch's pattern?
>
Actually I had used the pattern "uncore_imc_[0-9]" before. But for some units, e.g., CHA, they have
more than 10 units. So this simple pattern couldn't satisfy them.
And then I changed the pattern to "uncore_imc_[0-9]+$", which can match the string
"uncore_imc_<integer id>". But unfortunately it didn't work for fnmatch.
I used regex, such as:
asprintf(&pattern, "%s_[0-9]+$", tok);
regcomp(®ex, pattern, REG_EXTENDED);
ret = regexec(®ex, name, 0, NULL, 0);
But the regex approach looks not very simple (a bit heavy), so finally I just keep using fnmatch and
then just check the last character.
>>>> 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..78af01959830 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>
>>>> @@ -768,7 +769,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;
>>>> }
>>>> @@ -1872,3 +1873,25 @@ bool perf_pmu__has_hybrid(void)
>>>> return !list_empty(&perf_pmu__hybrid_pmus);
>>>> }
>>>> +
>>>> +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;
>>>> +}
>
> hum, so we have pattern serch and then another function checking
> if that search was ok..
Yes, that's what this patch does.
I understand that's convenient, because
> it's on 2 different places
Yes, on pmu_uncore_alias_match() and on parse-events.y.
but could we have some generic solution,
> line one function/search that returns/search for valid pmu name?
>
Sorry, I don't understand this idea well. Would you like to further explain?
Or can you accept the regex approach?
> thanks,
> jirka
>
On Mon, Jun 28, 2021 at 09:52:42AM +0800, Jin, Yao wrote:
SNIP
> > > > > +??? /*
> > > > > +???? * 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;
> > > > > +}
> >
> > hum, so we have pattern serch and then another function checking
> > if that search was ok..
>
> Yes, that's what this patch does.
>
> I understand that's convenient, because
> > it's on 2 different places
>
> Yes, on pmu_uncore_alias_match() and on parse-events.y.
>
> but could we have some generic solution,
> > line one function/search that returns/search for valid pmu name?
> >
>
> Sorry, I don't understand this idea well. Would you like to further explain?
>
> Or can you accept the regex approach?
I don't really have any suggestion, just would be great to have
this encapsulated in one function.. but if there's not any good
one, we can go with this change
Andi, Kan, are you ok with this change?
thanks,
jirka
On 6/29/2021 5:15 PM, Jiri Olsa wrote:
> On Mon, Jun 28, 2021 at 09:52:42AM +0800, Jin, Yao wrote:
>
> SNIP
>
>>>>>> + /*
>>>>>> + * 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;
>>>>>> +}
>>>
>>> hum, so we have pattern serch and then another function checking
>>> if that search was ok..
>>
>> Yes, that's what this patch does.
>>
>> I understand that's convenient, because
>>> it's on 2 different places
>>
>> Yes, on pmu_uncore_alias_match() and on parse-events.y.
>>
>> but could we have some generic solution,
>>> line one function/search that returns/search for valid pmu name?
>>>
>>
>> Sorry, I don't understand this idea well. Would you like to further explain?
>>
>> Or can you accept the regex approach?
>
> I don't really have any suggestion, just would be great to have
> this encapsulated in one function..
Yes, I agree. One function is better.
We just changed the design for the uncore PMU on SPR. There will be two
PMU names for each uncore unit, a real name and an alias. The perf tool
should handle both names. So we have to compare both names here.
I think one generic function can facilitate the code rebase.
https://lore.kernel.org/lkml/[email protected]/
Thanks,
Kan
Hi Jiri, Hi Kan,
On 6/30/2021 5:47 AM, Liang, Kan wrote:
>
>
> On 6/29/2021 5:15 PM, Jiri Olsa wrote:
>> On Mon, Jun 28, 2021 at 09:52:42AM +0800, Jin, Yao wrote:
>>
>> SNIP
>>
>>>>>>> + /*
>>>>>>> + * 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;
>>>>>>> +}
>>>>
>>>> hum, so we have pattern serch and then another function checking
>>>> if that search was ok..
>>>
>>> Yes, that's what this patch does.
>>>
>>> I understand that's convenient, because
>>>> it's on 2 different places
>>>
>>> Yes, on pmu_uncore_alias_match() and on parse-events.y.
>>>
>>> but could we have some generic solution,
>>>> line one function/search that returns/search for valid pmu name?
>>>>
>>>
>>> Sorry, I don't understand this idea well. Would you like to further explain?
>>>
>>> Or can you accept the regex approach?
>>
>> I don't really have any suggestion, just would be great to have
>> this encapsulated in one function..
>
> Yes, I agree. One function is better.
>
> We just changed the design for the uncore PMU on SPR. There will be two PMU names for each uncore
> unit, a real name and an alias. The perf tool should handle both names. So we have to compare both
> names here.
> I think one generic function can facilitate the code rebase.
>
> https://lore.kernel.org/lkml/[email protected]/
>
> Thanks,
> Kan
>
Thanks for the comments!
I'm now preparing v2. In v2 version, I will provide a new function 'perf_pmu__pattern_match which
wraps the matching and checking. Something like:
while ((pmu = perf_pmu__scan(pmu)) != NULL) {
if (!perf_pmu__pattern_match(pmu, pattern, $1)) {
...
}
}
I will post v2 soon.
Thanks
Jin Yao