[PATCH 0/2] perf pmu: Event parsing and listing fixes

[PATCH 0/2] perf pmu: Event parsing and listing fixes

[James Clark]

The second fix is related to the discussion here [1].

The first fix is unrelated but I just noticed it while fixing the
listing issue.

[1]: https://lore.kernel.org/all/ce31a50b-53db-4c6f-9cb1-242280b0951c@arm.com/

James Clark (2):
  perf pmu: Restore full PMU name wildcard support
  perf pmu: Don't de-duplicate core PMUs

 tools/perf/tests/pmu.c | 78 ++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/pmu.c  | 26 ++++++++++----
 2 files changed, 97 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH 1/2] perf pmu: Restore full PMU name wildcard support

[James Clark]

Commit b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in dynamic
pmu events") gives the following example for wildcarding a subset of
PMUs:

  E.g., in a system with the following dynamic pmus:

        mypmu_0
        mypmu_1
        mypmu_2
        mypmu_4

  perf stat -e mypmu_[01]/<config>/

Since commit f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()"), only
"*" has been supported, removing the ability to subset PMUs, even though
parse-events.l still supports ? and [] characters.

Fix it by using fnmatch() when any glob character is detected and add a
test which covers that and other scenarios of
perf_pmu__match_ignoring_suffix().

Fixes: f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()")
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/tests/pmu.c | 78 ++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/pmu.c  |  2 +-
 2 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
index cc88b5920c3e..fd07331b2d6e 100644
--- a/tools/perf/tests/pmu.c
+++ b/tools/perf/tests/pmu.c
@@ -437,12 +437,90 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
 	return TEST_OK;
 }
 
+/**
+ * Test perf_pmu__match() that's used to search for a PMU given a name passed
+ * on the command line. The name that's passed may also be a filename type glob
+ * match.
+ */
+static int test__pmu_match(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
+{
+	struct perf_pmu test_pmu;
+
+	test_pmu.name = "pmuname";
+	TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu, "pmuname"),	     true);
+	TEST_ASSERT_EQUAL("Longer token", perf_pmu__match(&test_pmu, "longertoken"), false);
+	TEST_ASSERT_EQUAL("Shorter token", perf_pmu__match(&test_pmu, "pmu"),	     false);
+
+	test_pmu.name = "pmuname_10";
+	TEST_ASSERT_EQUAL("Diff suffix_", perf_pmu__match(&test_pmu, "pmuname_2"),  false);
+	TEST_ASSERT_EQUAL("Sub suffix_",  perf_pmu__match(&test_pmu, "pmuname_1"),  true);
+	TEST_ASSERT_EQUAL("Same suffix_", perf_pmu__match(&test_pmu, "pmuname_10"), true);
+	TEST_ASSERT_EQUAL("No suffix_",   perf_pmu__match(&test_pmu, "pmuname"),    true);
+	TEST_ASSERT_EQUAL("Underscore_",  perf_pmu__match(&test_pmu, "pmuname_"),   true);
+	TEST_ASSERT_EQUAL("Substring_",   perf_pmu__match(&test_pmu, "pmuna"),      false);
+
+	test_pmu.name = "pmuname_ab23";
+	TEST_ASSERT_EQUAL("Diff suffix hex_", perf_pmu__match(&test_pmu, "pmuname_2"),    false);
+	TEST_ASSERT_EQUAL("Sub suffix hex_",  perf_pmu__match(&test_pmu, "pmuname_ab"),   true);
+	TEST_ASSERT_EQUAL("Same suffix hex_", perf_pmu__match(&test_pmu, "pmuname_ab23"), true);
+	TEST_ASSERT_EQUAL("No suffix hex_",   perf_pmu__match(&test_pmu, "pmuname"),      true);
+	TEST_ASSERT_EQUAL("Underscore hex_",  perf_pmu__match(&test_pmu, "pmuname_"),     true);
+	TEST_ASSERT_EQUAL("Substring hex_",   perf_pmu__match(&test_pmu, "pmuna"),	 false);
+
+	test_pmu.name = "pmuname10";
+	TEST_ASSERT_EQUAL("Diff suffix", perf_pmu__match(&test_pmu, "pmuname2"),  false);
+	TEST_ASSERT_EQUAL("Sub suffix",  perf_pmu__match(&test_pmu, "pmuname1"),  true);
+	TEST_ASSERT_EQUAL("Same suffix", perf_pmu__match(&test_pmu, "pmuname10"), true);
+	TEST_ASSERT_EQUAL("No suffix",   perf_pmu__match(&test_pmu, "pmuname"),   true);
+	TEST_ASSERT_EQUAL("Underscore",  perf_pmu__match(&test_pmu, "pmuname_"),  false);
+	TEST_ASSERT_EQUAL("Substring",   perf_pmu__match(&test_pmu, "pmuna"),     false);
+
+	test_pmu.name = "pmunameab23";
+	TEST_ASSERT_EQUAL("Diff suffix hex", perf_pmu__match(&test_pmu, "pmuname2"),    false);
+	TEST_ASSERT_EQUAL("Sub suffix hex",  perf_pmu__match(&test_pmu, "pmunameab"),   true);
+	TEST_ASSERT_EQUAL("Same suffix hex", perf_pmu__match(&test_pmu, "pmunameab23"), true);
+	TEST_ASSERT_EQUAL("No suffix hex",   perf_pmu__match(&test_pmu, "pmuname"),     true);
+	TEST_ASSERT_EQUAL("Underscore hex",  perf_pmu__match(&test_pmu, "pmuname_"),    false);
+	TEST_ASSERT_EQUAL("Substring hex",   perf_pmu__match(&test_pmu, "pmuna"),	false);
+
+	/*
+	 * 2 hex chars or less are not considered suffixes so it shouldn't be
+	 * possible to wildcard by skipping the suffix. Therefore there are more
+	 * false results here than above.
+	 */
+	test_pmu.name = "pmuname_a3";
+	TEST_ASSERT_EQUAL("Diff suffix 2 hex_", perf_pmu__match(&test_pmu, "pmuname_2"),  false);
+	/*
+	 * This one should be false, but because pmuname_a3 ends in 3 which is
+	 * decimal, it's not possible to determine if it's a short hex suffix or
+	 * a normal decimal suffix following text. And we want to match on any
+	 * length of decimal suffix. Run the test anyway and expect the wrong
+	 * result. And slightly fuzzy matching shouldn't do too much harm.
+	 */
+	TEST_ASSERT_EQUAL("Sub suffix 2 hex_",  perf_pmu__match(&test_pmu, "pmuname_a"),  true);
+	TEST_ASSERT_EQUAL("Same suffix 2 hex_", perf_pmu__match(&test_pmu, "pmuname_a3"), true);
+	TEST_ASSERT_EQUAL("No suffix 2 hex_",   perf_pmu__match(&test_pmu, "pmuname"),    false);
+	TEST_ASSERT_EQUAL("Underscore 2 hex_",  perf_pmu__match(&test_pmu, "pmuname_"),   false);
+	TEST_ASSERT_EQUAL("Substring 2 hex_",   perf_pmu__match(&test_pmu, "pmuna"),	  false);
+
+	test_pmu.name = "pmuname_5";
+	TEST_ASSERT_EQUAL("Glob 1", perf_pmu__match(&test_pmu, "pmu*"),		   true);
+	TEST_ASSERT_EQUAL("Glob 2", perf_pmu__match(&test_pmu, "nomatch*"),	   false);
+	TEST_ASSERT_EQUAL("Seq 1",  perf_pmu__match(&test_pmu, "pmuname_[12345]"), true);
+	TEST_ASSERT_EQUAL("Seq 2",  perf_pmu__match(&test_pmu, "pmuname_[67890]"), false);
+	TEST_ASSERT_EQUAL("? 1",    perf_pmu__match(&test_pmu, "pmuname_?"),	   true);
+	TEST_ASSERT_EQUAL("? 2",    perf_pmu__match(&test_pmu, "pmuname_1?"),	   false);
+
+	return TEST_OK;
+}
+
 static struct test_case tests__pmu[] = {
 	TEST_CASE("Parsing with PMU format directory", pmu_format),
 	TEST_CASE("Parsing with PMU event", pmu_events),
 	TEST_CASE("PMU event names", pmu_event_names),
 	TEST_CASE("PMU name combining", name_len),
 	TEST_CASE("PMU name comparison", name_cmp),
+	TEST_CASE("PMU cmdline match", pmu_match),
 	{	.name = NULL, }
 };
 
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index c94a91645b21..97d74fe6d816 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -2150,7 +2150,7 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
 bool perf_pmu__match(const struct perf_pmu *pmu, const char *tok)
 {
 	const char *name = pmu->name;
-	bool need_fnmatch = strchr(tok, '*') != NULL;
+	bool need_fnmatch = strisglob(tok);
 
 	if (!strncmp(tok, "uncore_", 7))
 		tok += 7;
-- 
2.34.1

Re: [PATCH 1/2] perf pmu: Restore full PMU name wildcard support

[James Clark]

On 17/06/2024 22:25, Ian Rogers wrote:
> On Mon, Jun 17, 2024, 6:44 AM James Clark <james.clark@arm.com> wrote:
> 
>> Commit b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in dynamic
>> pmu events") gives the following example for wildcarding a subset of
>> PMUs:
>>
>>   E.g., in a system with the following dynamic pmus:
>>
>>         mypmu_0
>>         mypmu_1
>>         mypmu_2
>>         mypmu_4
>>
>>   perf stat -e mypmu_[01]/<config>/
>>
>> Since commit f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()"), only
>> "*" has been supported, removing the ability to subset PMUs, even though
>> parse-events.l still supports ? and [] characters.
>>
>> Fix it by using fnmatch() when any glob character is detected and add a
>> test which covers that and other scenarios of
>> perf_pmu__match_ignoring_suffix().
>>
>> Fixes: f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()")
>> Signed-off-by: James Clark <james.clark@arm.com>
>>
> 
> We use regular expression matching elsewhere rather than fnmatch. We can
> also precompile the matchers using lex. I'm not sure we shouldn't be
> looking for an opportunity to remove fnmatch rather than expand upon it.
> 
> Thanks,
> Ian
> 

Presumably you mean we can do the removal of fnmatch after this fix goes in,
rather than instead of? Because this is a user facing change in behaviour
but the removal of fnmatch would be an non-user facing code refactor?

It's technically not an "expansion" because we always used fnmatch and the
linked commit hasn't made it to a release yet.

> 
> ---
>>  tools/perf/tests/pmu.c | 78 ++++++++++++++++++++++++++++++++++++++++++
>>  tools/perf/util/pmu.c  |  2 +-
>>  2 files changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
>> index cc88b5920c3e..fd07331b2d6e 100644
>> --- a/tools/perf/tests/pmu.c
>> +++ b/tools/perf/tests/pmu.c
>> @@ -437,12 +437,90 @@ static int test__name_cmp(struct test_suite *test
>> __maybe_unused, int subtest __
>>         return TEST_OK;
>>  }
>>
>> +/**
>> + * Test perf_pmu__match() that's used to search for a PMU given a name
>> passed
>> + * on the command line. The name that's passed may also be a filename
>> type glob
>> + * match.
>> + */
>> +static int test__pmu_match(struct test_suite *test __maybe_unused, int
>> subtest __maybe_unused)
>> +{
>> +       struct perf_pmu test_pmu;
>> +
>> +       test_pmu.name = "pmuname";
>> +       TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu,
>> "pmuname"),      true);
>> +       TEST_ASSERT_EQUAL("Longer token", perf_pmu__match(&test_pmu,
>> "longertoken"), false);
>> +       TEST_ASSERT_EQUAL("Shorter token", perf_pmu__match(&test_pmu,
>> "pmu"),        false);
>> +
>> +       test_pmu.name = "pmuname_10";
>> +       TEST_ASSERT_EQUAL("Diff suffix_", perf_pmu__match(&test_pmu,
>> "pmuname_2"),  false);
>> +       TEST_ASSERT_EQUAL("Sub suffix_",  perf_pmu__match(&test_pmu,
>> "pmuname_1"),  true);
>> +       TEST_ASSERT_EQUAL("Same suffix_", perf_pmu__match(&test_pmu,
>> "pmuname_10"), true);
>> +       TEST_ASSERT_EQUAL("No suffix_",   perf_pmu__match(&test_pmu,
>> "pmuname"),    true);
>> +       TEST_ASSERT_EQUAL("Underscore_",  perf_pmu__match(&test_pmu,
>> "pmuname_"),   true);
>> +       TEST_ASSERT_EQUAL("Substring_",   perf_pmu__match(&test_pmu,
>> "pmuna"),      false);
>> +
>> +       test_pmu.name = "pmuname_ab23";
>> +       TEST_ASSERT_EQUAL("Diff suffix hex_", perf_pmu__match(&test_pmu,
>> "pmuname_2"),    false);
>> +       TEST_ASSERT_EQUAL("Sub suffix hex_",  perf_pmu__match(&test_pmu,
>> "pmuname_ab"),   true);
>> +       TEST_ASSERT_EQUAL("Same suffix hex_", perf_pmu__match(&test_pmu,
>> "pmuname_ab23"), true);
>> +       TEST_ASSERT_EQUAL("No suffix hex_",   perf_pmu__match(&test_pmu,
>> "pmuname"),      true);
>> +       TEST_ASSERT_EQUAL("Underscore hex_",  perf_pmu__match(&test_pmu,
>> "pmuname_"),     true);
>> +       TEST_ASSERT_EQUAL("Substring hex_",   perf_pmu__match(&test_pmu,
>> "pmuna"),       false);
>> +
>> +       test_pmu.name = "pmuname10";
>> +       TEST_ASSERT_EQUAL("Diff suffix", perf_pmu__match(&test_pmu,
>> "pmuname2"),  false);
>> +       TEST_ASSERT_EQUAL("Sub suffix",  perf_pmu__match(&test_pmu,
>> "pmuname1"),  true);
>> +       TEST_ASSERT_EQUAL("Same suffix", perf_pmu__match(&test_pmu,
>> "pmuname10"), true);
>> +       TEST_ASSERT_EQUAL("No suffix",   perf_pmu__match(&test_pmu,
>> "pmuname"),   true);
>> +       TEST_ASSERT_EQUAL("Underscore",  perf_pmu__match(&test_pmu,
>> "pmuname_"),  false);
>> +       TEST_ASSERT_EQUAL("Substring",   perf_pmu__match(&test_pmu,
>> "pmuna"),     false);
>> +
>> +       test_pmu.name = "pmunameab23";
>> +       TEST_ASSERT_EQUAL("Diff suffix hex", perf_pmu__match(&test_pmu,
>> "pmuname2"),    false);
>> +       TEST_ASSERT_EQUAL("Sub suffix hex",  perf_pmu__match(&test_pmu,
>> "pmunameab"),   true);
>> +       TEST_ASSERT_EQUAL("Same suffix hex", perf_pmu__match(&test_pmu,
>> "pmunameab23"), true);
>> +       TEST_ASSERT_EQUAL("No suffix hex",   perf_pmu__match(&test_pmu,
>> "pmuname"),     true);
>> +       TEST_ASSERT_EQUAL("Underscore hex",  perf_pmu__match(&test_pmu,
>> "pmuname_"),    false);
>> +       TEST_ASSERT_EQUAL("Substring hex",   perf_pmu__match(&test_pmu,
>> "pmuna"),       false);
>> +
>> +       /*
>> +        * 2 hex chars or less are not considered suffixes so it shouldn't
>> be
>> +        * possible to wildcard by skipping the suffix. Therefore there
>> are more
>> +        * false results here than above.
>> +        */
>> +       test_pmu.name = "pmuname_a3";
>> +       TEST_ASSERT_EQUAL("Diff suffix 2 hex_", perf_pmu__match(&test_pmu,
>> "pmuname_2"),  false);
>> +       /*
>> +        * This one should be false, but because pmuname_a3 ends in 3
>> which is
>> +        * decimal, it's not possible to determine if it's a short hex
>> suffix or
>> +        * a normal decimal suffix following text. And we want to match on
>> any
>> +        * length of decimal suffix. Run the test anyway and expect the
>> wrong
>> +        * result. And slightly fuzzy matching shouldn't do too much harm.
>> +        */
>> +       TEST_ASSERT_EQUAL("Sub suffix 2 hex_",  perf_pmu__match(&test_pmu,
>> "pmuname_a"),  true);
>> +       TEST_ASSERT_EQUAL("Same suffix 2 hex_", perf_pmu__match(&test_pmu,
>> "pmuname_a3"), true);
>> +       TEST_ASSERT_EQUAL("No suffix 2 hex_",   perf_pmu__match(&test_pmu,
>> "pmuname"),    false);
>> +       TEST_ASSERT_EQUAL("Underscore 2 hex_",  perf_pmu__match(&test_pmu,
>> "pmuname_"),   false);
>> +       TEST_ASSERT_EQUAL("Substring 2 hex_",   perf_pmu__match(&test_pmu,
>> "pmuna"),      false);
>> +
>> +       test_pmu.name = "pmuname_5";
>> +       TEST_ASSERT_EQUAL("Glob 1", perf_pmu__match(&test_pmu, "pmu*"),
>>         true);
>> +       TEST_ASSERT_EQUAL("Glob 2", perf_pmu__match(&test_pmu,
>> "nomatch*"),        false);
>> +       TEST_ASSERT_EQUAL("Seq 1",  perf_pmu__match(&test_pmu,
>> "pmuname_[12345]"), true);
>> +       TEST_ASSERT_EQUAL("Seq 2",  perf_pmu__match(&test_pmu,
>> "pmuname_[67890]"), false);
>> +       TEST_ASSERT_EQUAL("? 1",    perf_pmu__match(&test_pmu,
>> "pmuname_?"),       true);
>> +       TEST_ASSERT_EQUAL("? 2",    perf_pmu__match(&test_pmu,
>> "pmuname_1?"),      false);
>> +
>> +       return TEST_OK;
>> +}
>> +
>>  static struct test_case tests__pmu[] = {
>>         TEST_CASE("Parsing with PMU format directory", pmu_format),
>>         TEST_CASE("Parsing with PMU event", pmu_events),
>>         TEST_CASE("PMU event names", pmu_event_names),
>>         TEST_CASE("PMU name combining", name_len),
>>         TEST_CASE("PMU name comparison", name_cmp),
>> +       TEST_CASE("PMU cmdline match", pmu_match),
>>         {       .name = NULL, }
>>  };
>>
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index c94a91645b21..97d74fe6d816 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -2150,7 +2150,7 @@ void perf_pmu__warn_invalid_config(struct perf_pmu
>> *pmu, __u64 config,
>>  bool perf_pmu__match(const struct perf_pmu *pmu, const char *tok)
>>  {
>>         const char *name = pmu->name;
>> -       bool need_fnmatch = strchr(tok, '*') != NULL;
>> +       bool need_fnmatch = strisglob(tok);
>>
>>         if (!strncmp(tok, "uncore_", 7))
>>                 tok += 7;
>> --
>> 2.34.1
>>
>>
> 

Re: [PATCH 1/2] perf pmu: Restore full PMU name wildcard support

[James Clark]

On 18/06/2024 15:23, Ian Rogers wrote:
> On Tue, Jun 18, 2024, 3:59 AM James Clark <james.clark@arm.com> wrote:
> 
>>
>>
>> On 17/06/2024 22:25, Ian Rogers wrote:
>>> On Mon, Jun 17, 2024, 6:44 AM James Clark <james.clark@arm.com> wrote:
>>>
>>>> Commit b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in dynamic
>>>> pmu events") gives the following example for wildcarding a subset of
>>>> PMUs:
>>>>
>>>>   E.g., in a system with the following dynamic pmus:
>>>>
>>>>         mypmu_0
>>>>         mypmu_1
>>>>         mypmu_2
>>>>         mypmu_4
>>>>
>>>>   perf stat -e mypmu_[01]/<config>/
>>>>
>>>> Since commit f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()"), only
>>>> "*" has been supported, removing the ability to subset PMUs, even though
>>>> parse-events.l still supports ? and [] characters.
>>>>
>>>> Fix it by using fnmatch() when any glob character is detected and add a
>>>> test which covers that and other scenarios of
>>>> perf_pmu__match_ignoring_suffix().
>>>>
>>>> Fixes: f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()")
>>>> Signed-off-by: James Clark <james.clark@arm.com>
>>>>
>>>
>>> We use regular expression matching elsewhere rather than fnmatch. We can
>>> also precompile the matchers using lex. I'm not sure we shouldn't be
>>> looking for an opportunity to remove fnmatch rather than expand upon it.
>>>
>>> Thanks,
>>> Ian
>>>
>>
>> Presumably you mean we can do the removal of fnmatch after this fix goes
>> in,
>> rather than instead of? Because this is a user facing change in behaviour
>> but the removal of fnmatch would be an non-user facing code refactor?
>>
>> It's technically not an "expansion" because we always used fnmatch and the
>> linked commit hasn't made it to a release yet.
>>
> 
> The main place the expansion gets added is parse-events.c, previously
> parse-events.y. If we're adding the expansion ourselves then we can choose
> the form we add it. Some coming servers will have 100s of PMUs and so I'm
> worried about the scanning cost when a PMU isn't specified.
> 
> Thanks,
> Ian
> 

I think I might not be following. If a PMU isn't specified then
perf_pmu__match() is never called so no cost is incurred. I also don't
add any new calls to fnmatch().

I only updated the gate on whether the existing fnmatch() is called from
"*" to "*[?". So it only happens when one of those characters is in the
PMU name, but it already happens when '*' is in the name.

But yes I agree about the potential performance issues. With a large
number of PMUs if regex is faster we can update it to use that instead.
But we should make sure the regex supports the same wildcarding options
as before (unless we want to remove the range options, but that's a
separate thing I suppose).

>>>
>>> ---
>>>>  tools/perf/tests/pmu.c | 78 ++++++++++++++++++++++++++++++++++++++++++
>>>>  tools/perf/util/pmu.c  |  2 +-
>>>>  2 files changed, 79 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
>>>> index cc88b5920c3e..fd07331b2d6e 100644
>>>> --- a/tools/perf/tests/pmu.c
>>>> +++ b/tools/perf/tests/pmu.c
>>>> @@ -437,12 +437,90 @@ static int test__name_cmp(struct test_suite *test
>>>> __maybe_unused, int subtest __
>>>>         return TEST_OK;
>>>>  }
>>>>
>>>> +/**
>>>> + * Test perf_pmu__match() that's used to search for a PMU given a name
>>>> passed
>>>> + * on the command line. The name that's passed may also be a filename
>>>> type glob
>>>> + * match.
>>>> + */
>>>> +static int test__pmu_match(struct test_suite *test __maybe_unused, int
>>>> subtest __maybe_unused)
>>>> +{
>>>> +       struct perf_pmu test_pmu;
>>>> +
>>>> +       test_pmu.name = "pmuname";
>>>> +       TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu,
>>>> "pmuname"),      true);
>>>> +       TEST_ASSERT_EQUAL("Longer token", perf_pmu__match(&test_pmu,
>>>> "longertoken"), false);
>>>> +       TEST_ASSERT_EQUAL("Shorter token", perf_pmu__match(&test_pmu,
>>>> "pmu"),        false);
>>>> +
>>>> +       test_pmu.name = "pmuname_10";
>>>> +       TEST_ASSERT_EQUAL("Diff suffix_", perf_pmu__match(&test_pmu,
>>>> "pmuname_2"),  false);
>>>> +       TEST_ASSERT_EQUAL("Sub suffix_",  perf_pmu__match(&test_pmu,
>>>> "pmuname_1"),  true);
>>>> +       TEST_ASSERT_EQUAL("Same suffix_", perf_pmu__match(&test_pmu,
>>>> "pmuname_10"), true);
>>>> +       TEST_ASSERT_EQUAL("No suffix_",   perf_pmu__match(&test_pmu,
>>>> "pmuname"),    true);
>>>> +       TEST_ASSERT_EQUAL("Underscore_",  perf_pmu__match(&test_pmu,
>>>> "pmuname_"),   true);
>>>> +       TEST_ASSERT_EQUAL("Substring_",   perf_pmu__match(&test_pmu,
>>>> "pmuna"),      false);
>>>> +
>>>> +       test_pmu.name = "pmuname_ab23";
>>>> +       TEST_ASSERT_EQUAL("Diff suffix hex_", perf_pmu__match(&test_pmu,
>>>> "pmuname_2"),    false);
>>>> +       TEST_ASSERT_EQUAL("Sub suffix hex_",  perf_pmu__match(&test_pmu,
>>>> "pmuname_ab"),   true);
>>>> +       TEST_ASSERT_EQUAL("Same suffix hex_", perf_pmu__match(&test_pmu,
>>>> "pmuname_ab23"), true);
>>>> +       TEST_ASSERT_EQUAL("No suffix hex_",   perf_pmu__match(&test_pmu,
>>>> "pmuname"),      true);
>>>> +       TEST_ASSERT_EQUAL("Underscore hex_",  perf_pmu__match(&test_pmu,
>>>> "pmuname_"),     true);
>>>> +       TEST_ASSERT_EQUAL("Substring hex_",   perf_pmu__match(&test_pmu,
>>>> "pmuna"),       false);
>>>> +
>>>> +       test_pmu.name = "pmuname10";
>>>> +       TEST_ASSERT_EQUAL("Diff suffix", perf_pmu__match(&test_pmu,
>>>> "pmuname2"),  false);
>>>> +       TEST_ASSERT_EQUAL("Sub suffix",  perf_pmu__match(&test_pmu,
>>>> "pmuname1"),  true);
>>>> +       TEST_ASSERT_EQUAL("Same suffix", perf_pmu__match(&test_pmu,
>>>> "pmuname10"), true);
>>>> +       TEST_ASSERT_EQUAL("No suffix",   perf_pmu__match(&test_pmu,
>>>> "pmuname"),   true);
>>>> +       TEST_ASSERT_EQUAL("Underscore",  perf_pmu__match(&test_pmu,
>>>> "pmuname_"),  false);
>>>> +       TEST_ASSERT_EQUAL("Substring",   perf_pmu__match(&test_pmu,
>>>> "pmuna"),     false);
>>>> +
>>>> +       test_pmu.name = "pmunameab23";
>>>> +       TEST_ASSERT_EQUAL("Diff suffix hex", perf_pmu__match(&test_pmu,
>>>> "pmuname2"),    false);
>>>> +       TEST_ASSERT_EQUAL("Sub suffix hex",  perf_pmu__match(&test_pmu,
>>>> "pmunameab"),   true);
>>>> +       TEST_ASSERT_EQUAL("Same suffix hex", perf_pmu__match(&test_pmu,
>>>> "pmunameab23"), true);
>>>> +       TEST_ASSERT_EQUAL("No suffix hex",   perf_pmu__match(&test_pmu,
>>>> "pmuname"),     true);
>>>> +       TEST_ASSERT_EQUAL("Underscore hex",  perf_pmu__match(&test_pmu,
>>>> "pmuname_"),    false);
>>>> +       TEST_ASSERT_EQUAL("Substring hex",   perf_pmu__match(&test_pmu,
>>>> "pmuna"),       false);
>>>> +
>>>> +       /*
>>>> +        * 2 hex chars or less are not considered suffixes so it
>> shouldn't
>>>> be
>>>> +        * possible to wildcard by skipping the suffix. Therefore there
>>>> are more
>>>> +        * false results here than above.
>>>> +        */
>>>> +       test_pmu.name = "pmuname_a3";
>>>> +       TEST_ASSERT_EQUAL("Diff suffix 2 hex_",
>> perf_pmu__match(&test_pmu,
>>>> "pmuname_2"),  false);
>>>> +       /*
>>>> +        * This one should be false, but because pmuname_a3 ends in 3
>>>> which is
>>>> +        * decimal, it's not possible to determine if it's a short hex
>>>> suffix or
>>>> +        * a normal decimal suffix following text. And we want to match
>> on
>>>> any
>>>> +        * length of decimal suffix. Run the test anyway and expect the
>>>> wrong
>>>> +        * result. And slightly fuzzy matching shouldn't do too much
>> harm.
>>>> +        */
>>>> +       TEST_ASSERT_EQUAL("Sub suffix 2 hex_",
>> perf_pmu__match(&test_pmu,
>>>> "pmuname_a"),  true);
>>>> +       TEST_ASSERT_EQUAL("Same suffix 2 hex_",
>> perf_pmu__match(&test_pmu,
>>>> "pmuname_a3"), true);
>>>> +       TEST_ASSERT_EQUAL("No suffix 2 hex_",
>>  perf_pmu__match(&test_pmu,
>>>> "pmuname"),    false);
>>>> +       TEST_ASSERT_EQUAL("Underscore 2 hex_",
>> perf_pmu__match(&test_pmu,
>>>> "pmuname_"),   false);
>>>> +       TEST_ASSERT_EQUAL("Substring 2 hex_",
>>  perf_pmu__match(&test_pmu,
>>>> "pmuna"),      false);
>>>> +
>>>> +       test_pmu.name = "pmuname_5";
>>>> +       TEST_ASSERT_EQUAL("Glob 1", perf_pmu__match(&test_pmu, "pmu*"),
>>>>         true);
>>>> +       TEST_ASSERT_EQUAL("Glob 2", perf_pmu__match(&test_pmu,
>>>> "nomatch*"),        false);
>>>> +       TEST_ASSERT_EQUAL("Seq 1",  perf_pmu__match(&test_pmu,
>>>> "pmuname_[12345]"), true);
>>>> +       TEST_ASSERT_EQUAL("Seq 2",  perf_pmu__match(&test_pmu,
>>>> "pmuname_[67890]"), false);
>>>> +       TEST_ASSERT_EQUAL("? 1",    perf_pmu__match(&test_pmu,
>>>> "pmuname_?"),       true);
>>>> +       TEST_ASSERT_EQUAL("? 2",    perf_pmu__match(&test_pmu,
>>>> "pmuname_1?"),      false);
>>>> +
>>>> +       return TEST_OK;
>>>> +}
>>>> +
>>>>  static struct test_case tests__pmu[] = {
>>>>         TEST_CASE("Parsing with PMU format directory", pmu_format),
>>>>         TEST_CASE("Parsing with PMU event", pmu_events),
>>>>         TEST_CASE("PMU event names", pmu_event_names),
>>>>         TEST_CASE("PMU name combining", name_len),
>>>>         TEST_CASE("PMU name comparison", name_cmp),
>>>> +       TEST_CASE("PMU cmdline match", pmu_match),
>>>>         {       .name = NULL, }
>>>>  };
>>>>
>>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>>>> index c94a91645b21..97d74fe6d816 100644
>>>> --- a/tools/perf/util/pmu.c
>>>> +++ b/tools/perf/util/pmu.c
>>>> @@ -2150,7 +2150,7 @@ void perf_pmu__warn_invalid_config(struct perf_pmu
>>>> *pmu, __u64 config,
>>>>  bool perf_pmu__match(const struct perf_pmu *pmu, const char *tok)
>>>>  {
>>>>         const char *name = pmu->name;
>>>> -       bool need_fnmatch = strchr(tok, '*') != NULL;
>>>> +       bool need_fnmatch = strisglob(tok);
>>>>
>>>>         if (!strncmp(tok, "uncore_", 7))
>>>>                 tok += 7;
>>>> --
>>>> 2.34.1
>>>>
>>>>
>>>
>>
> 

Re: [PATCH 1/2] perf pmu: Restore full PMU name wildcard support

[James Clark]

On 18/06/2024 16:25, Ian Rogers wrote:
> On Tue, Jun 18, 2024, 8:06 AM James Clark <james.clark@arm.com> wrote:
> 
>>
>>
>> On 18/06/2024 15:23, Ian Rogers wrote:
>>> On Tue, Jun 18, 2024, 3:59 AM James Clark <james.clark@arm.com> wrote:
>>>
>>>>
>>>>
>>>> On 17/06/2024 22:25, Ian Rogers wrote:
>>>>> On Mon, Jun 17, 2024, 6:44 AM James Clark <james.clark@arm.com> wrote:
>>>>>
>>>>>> Commit b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in
>> dynamic
>>>>>> pmu events") gives the following example for wildcarding a subset of
>>>>>> PMUs:
>>>>>>
>>>>>>   E.g., in a system with the following dynamic pmus:
>>>>>>
>>>>>>         mypmu_0
>>>>>>         mypmu_1
>>>>>>         mypmu_2
>>>>>>         mypmu_4
>>>>>>
>>>>>>   perf stat -e mypmu_[01]/<config>/
>>>>>>
>>>>>> Since commit f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()"),
>> only
>>>>>> "*" has been supported, removing the ability to subset PMUs, even
>> though
>>>>>> parse-events.l still supports ? and [] characters.
>>>>>>
>>>>>> Fix it by using fnmatch() when any glob character is detected and add
>> a
>>>>>> test which covers that and other scenarios of
>>>>>> perf_pmu__match_ignoring_suffix().
>>>>>>
>>>>>> Fixes: f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()")
>>>>>> Signed-off-by: James Clark <james.clark@arm.com>
>>>>>>
>>>>>
>>>>> We use regular expression matching elsewhere rather than fnmatch. We
>> can
>>>>> also precompile the matchers using lex. I'm not sure we shouldn't be
>>>>> looking for an opportunity to remove fnmatch rather than expand upon
>> it.
>>>>>
>>>>> Thanks,
>>>>> Ian
>>>>>
>>>>
>>>> Presumably you mean we can do the removal of fnmatch after this fix goes
>>>> in,
>>>> rather than instead of? Because this is a user facing change in
>> behaviour
>>>> but the removal of fnmatch would be an non-user facing code refactor?
>>>>
>>>> It's technically not an "expansion" because we always used fnmatch and
>> the
>>>> linked commit hasn't made it to a release yet.
>>>>
>>>
>>> The main place the expansion gets added is parse-events.c, previously
>>> parse-events.y. If we're adding the expansion ourselves then we can
>> choose
>>> the form we add it. Some coming servers will have 100s of PMUs and so I'm
>>> worried about the scanning cost when a PMU isn't specified.
>>>
>>> Thanks,
>>> Ian
>>>
>>
>> I think I might not be following. If a PMU isn't specified then
>> perf_pmu__match() is never called so no cost is incurred. I also don't
>> add any new calls to fnmatch().
>>
>> I only updated the gate on whether the existing fnmatch() is called from
>> "*" to "*[?". So it only happens when one of those characters is in the
>> PMU name, but it already happens when '*' is in the name.
>>
> 
> Right. I'm not saying there is anything wrong in the change or an
> additional cost, what the issue is is that currently only really '*'
> requires fnmatch and that's because the event parsing adds it. It could

It's not only '*' that requires it, see the example I added in the
commit message:

  ./perf stat -e mypmu_[01]/<config>/

'?' was supported before as well which could be useful.

> similarly add '.*' if we did regular expression matching. By expanding what
> we pass to fnmatch from the command line the more committed we are to
> fnmatch rather than regular expressions - which is what we use everywhere
> else in the code. So maybe it was a feature that this wasn't working.
> 

But we haven't had a release of Perf yet where more is passed to
fnmatch(). Before f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()")
everything was passed to fnmatch(). After that unreleased commit only
things with '*' are. Now with this change only "*?[", so it's less not more.

I don't think there is any commitment to keep it, we can always remove
fnmatch in the future. But it looks like a mistake to me because the
title says "refactor" when it actually removes a feature.

> Thanks,
> Ian


> 
> 
> But yes I agree about the potential performance issues. With a large
>> number of PMUs if regex is faster we can update it to use that instead.
>> But we should make sure the regex supports the same wildcarding options
>> as before (unless we want to remove the range options, but that's a
>> separate thing I suppose).
>>
>>>>>
>>>>> ---
>>>>>>  tools/perf/tests/pmu.c | 78
>> ++++++++++++++++++++++++++++++++++++++++++
>>>>>>  tools/perf/util/pmu.c  |  2 +-
>>>>>>  2 files changed, 79 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
>>>>>> index cc88b5920c3e..fd07331b2d6e 100644
>>>>>> --- a/tools/perf/tests/pmu.c
>>>>>> +++ b/tools/perf/tests/pmu.c
>>>>>> @@ -437,12 +437,90 @@ static int test__name_cmp(struct test_suite
>> *test
>>>>>> __maybe_unused, int subtest __
>>>>>>         return TEST_OK;
>>>>>>  }
>>>>>>
>>>>>> +/**
>>>>>> + * Test perf_pmu__match() that's used to search for a PMU given a
>> name
>>>>>> passed
>>>>>> + * on the command line. The name that's passed may also be a filename
>>>>>> type glob
>>>>>> + * match.
>>>>>> + */
>>>>>> +static int test__pmu_match(struct test_suite *test __maybe_unused,
>> int
>>>>>> subtest __maybe_unused)
>>>>>> +{
>>>>>> +       struct perf_pmu test_pmu;
>>>>>> +
>>>>>> +       test_pmu.name = "pmuname";
>>>>>> +       TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu,
>>>>>> "pmuname"),      true);
>>>>>> +       TEST_ASSERT_EQUAL("Longer token", perf_pmu__match(&test_pmu,
>>>>>> "longertoken"), false);
>>>>>> +       TEST_ASSERT_EQUAL("Shorter token", perf_pmu__match(&test_pmu,
>>>>>> "pmu"),        false);
>>>>>> +
>>>>>> +       test_pmu.name = "pmuname_10";
>>>>>> +       TEST_ASSERT_EQUAL("Diff suffix_", perf_pmu__match(&test_pmu,
>>>>>> "pmuname_2"),  false);
>>>>>> +       TEST_ASSERT_EQUAL("Sub suffix_",  perf_pmu__match(&test_pmu,
>>>>>> "pmuname_1"),  true);
>>>>>> +       TEST_ASSERT_EQUAL("Same suffix_", perf_pmu__match(&test_pmu,
>>>>>> "pmuname_10"), true);
>>>>>> +       TEST_ASSERT_EQUAL("No suffix_",   perf_pmu__match(&test_pmu,
>>>>>> "pmuname"),    true);
>>>>>> +       TEST_ASSERT_EQUAL("Underscore_",  perf_pmu__match(&test_pmu,
>>>>>> "pmuname_"),   true);
>>>>>> +       TEST_ASSERT_EQUAL("Substring_",   perf_pmu__match(&test_pmu,
>>>>>> "pmuna"),      false);
>>>>>> +
>>>>>> +       test_pmu.name = "pmuname_ab23";
>>>>>> +       TEST_ASSERT_EQUAL("Diff suffix hex_",
>> perf_pmu__match(&test_pmu,
>>>>>> "pmuname_2"),    false);
>>>>>> +       TEST_ASSERT_EQUAL("Sub suffix hex_",
>> perf_pmu__match(&test_pmu,
>>>>>> "pmuname_ab"),   true);
>>>>>> +       TEST_ASSERT_EQUAL("Same suffix hex_",
>> perf_pmu__match(&test_pmu,
>>>>>> "pmuname_ab23"), true);
>>>>>> +       TEST_ASSERT_EQUAL("No suffix hex_",
>>  perf_pmu__match(&test_pmu,
>>>>>> "pmuname"),      true);
>>>>>> +       TEST_ASSERT_EQUAL("Underscore hex_",
>> perf_pmu__match(&test_pmu,
>>>>>> "pmuname_"),     true);
>>>>>> +       TEST_ASSERT_EQUAL("Substring hex_",
>>  perf_pmu__match(&test_pmu,
>>>>>> "pmuna"),       false);
>>>>>> +
>>>>>> +       test_pmu.name = "pmuname10";
>>>>>> +       TEST_ASSERT_EQUAL("Diff suffix", perf_pmu__match(&test_pmu,
>>>>>> "pmuname2"),  false);
>>>>>> +       TEST_ASSERT_EQUAL("Sub suffix",  perf_pmu__match(&test_pmu,
>>>>>> "pmuname1"),  true);
>>>>>> +       TEST_ASSERT_EQUAL("Same suffix", perf_pmu__match(&test_pmu,
>>>>>> "pmuname10"), true);
>>>>>> +       TEST_ASSERT_EQUAL("No suffix",   perf_pmu__match(&test_pmu,
>>>>>> "pmuname"),   true);
>>>>>> +       TEST_ASSERT_EQUAL("Underscore",  perf_pmu__match(&test_pmu,
>>>>>> "pmuname_"),  false);
>>>>>> +       TEST_ASSERT_EQUAL("Substring",   perf_pmu__match(&test_pmu,
>>>>>> "pmuna"),     false);
>>>>>> +
>>>>>> +       test_pmu.name = "pmunameab23";
>>>>>> +       TEST_ASSERT_EQUAL("Diff suffix hex",
>> perf_pmu__match(&test_pmu,
>>>>>> "pmuname2"),    false);
>>>>>> +       TEST_ASSERT_EQUAL("Sub suffix hex",
>> perf_pmu__match(&test_pmu,
>>>>>> "pmunameab"),   true);
>>>>>> +       TEST_ASSERT_EQUAL("Same suffix hex",
>> perf_pmu__match(&test_pmu,
>>>>>> "pmunameab23"), true);
>>>>>> +       TEST_ASSERT_EQUAL("No suffix hex",
>>  perf_pmu__match(&test_pmu,
>>>>>> "pmuname"),     true);
>>>>>> +       TEST_ASSERT_EQUAL("Underscore hex",
>> perf_pmu__match(&test_pmu,
>>>>>> "pmuname_"),    false);
>>>>>> +       TEST_ASSERT_EQUAL("Substring hex",
>>  perf_pmu__match(&test_pmu,
>>>>>> "pmuna"),       false);
>>>>>> +
>>>>>> +       /*
>>>>>> +        * 2 hex chars or less are not considered suffixes so it
>>>> shouldn't
>>>>>> be
>>>>>> +        * possible to wildcard by skipping the suffix. Therefore
>> there
>>>>>> are more
>>>>>> +        * false results here than above.
>>>>>> +        */
>>>>>> +       test_pmu.name = "pmuname_a3";
>>>>>> +       TEST_ASSERT_EQUAL("Diff suffix 2 hex_",
>>>> perf_pmu__match(&test_pmu,
>>>>>> "pmuname_2"),  false);
>>>>>> +       /*
>>>>>> +        * This one should be false, but because pmuname_a3 ends in 3
>>>>>> which is
>>>>>> +        * decimal, it's not possible to determine if it's a short hex
>>>>>> suffix or
>>>>>> +        * a normal decimal suffix following text. And we want to
>> match
>>>> on
>>>>>> any
>>>>>> +        * length of decimal suffix. Run the test anyway and expect
>> the
>>>>>> wrong
>>>>>> +        * result. And slightly fuzzy matching shouldn't do too much
>>>> harm.
>>>>>> +        */
>>>>>> +       TEST_ASSERT_EQUAL("Sub suffix 2 hex_",
>>>> perf_pmu__match(&test_pmu,
>>>>>> "pmuname_a"),  true);
>>>>>> +       TEST_ASSERT_EQUAL("Same suffix 2 hex_",
>>>> perf_pmu__match(&test_pmu,
>>>>>> "pmuname_a3"), true);
>>>>>> +       TEST_ASSERT_EQUAL("No suffix 2 hex_",
>>>>  perf_pmu__match(&test_pmu,
>>>>>> "pmuname"),    false);
>>>>>> +       TEST_ASSERT_EQUAL("Underscore 2 hex_",
>>>> perf_pmu__match(&test_pmu,
>>>>>> "pmuname_"),   false);
>>>>>> +       TEST_ASSERT_EQUAL("Substring 2 hex_",
>>>>  perf_pmu__match(&test_pmu,
>>>>>> "pmuna"),      false);
>>>>>> +
>>>>>> +       test_pmu.name = "pmuname_5";
>>>>>> +       TEST_ASSERT_EQUAL("Glob 1", perf_pmu__match(&test_pmu,
>> "pmu*"),
>>>>>>         true);
>>>>>> +       TEST_ASSERT_EQUAL("Glob 2", perf_pmu__match(&test_pmu,
>>>>>> "nomatch*"),        false);
>>>>>> +       TEST_ASSERT_EQUAL("Seq 1",  perf_pmu__match(&test_pmu,
>>>>>> "pmuname_[12345]"), true);
>>>>>> +       TEST_ASSERT_EQUAL("Seq 2",  perf_pmu__match(&test_pmu,
>>>>>> "pmuname_[67890]"), false);
>>>>>> +       TEST_ASSERT_EQUAL("? 1",    perf_pmu__match(&test_pmu,
>>>>>> "pmuname_?"),       true);
>>>>>> +       TEST_ASSERT_EQUAL("? 2",    perf_pmu__match(&test_pmu,
>>>>>> "pmuname_1?"),      false);
>>>>>> +
>>>>>> +       return TEST_OK;
>>>>>> +}
>>>>>> +
>>>>>>  static struct test_case tests__pmu[] = {
>>>>>>         TEST_CASE("Parsing with PMU format directory", pmu_format),
>>>>>>         TEST_CASE("Parsing with PMU event", pmu_events),
>>>>>>         TEST_CASE("PMU event names", pmu_event_names),
>>>>>>         TEST_CASE("PMU name combining", name_len),
>>>>>>         TEST_CASE("PMU name comparison", name_cmp),
>>>>>> +       TEST_CASE("PMU cmdline match", pmu_match),
>>>>>>         {       .name = NULL, }
>>>>>>  };
>>>>>>
>>>>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>>>>>> index c94a91645b21..97d74fe6d816 100644
>>>>>> --- a/tools/perf/util/pmu.c
>>>>>> +++ b/tools/perf/util/pmu.c
>>>>>> @@ -2150,7 +2150,7 @@ void perf_pmu__warn_invalid_config(struct
>> perf_pmu
>>>>>> *pmu, __u64 config,
>>>>>>  bool perf_pmu__match(const struct perf_pmu *pmu, const char *tok)
>>>>>>  {
>>>>>>         const char *name = pmu->name;
>>>>>> -       bool need_fnmatch = strchr(tok, '*') != NULL;
>>>>>> +       bool need_fnmatch = strisglob(tok);
>>>>>>
>>>>>>         if (!strncmp(tok, "uncore_", 7))
>>>>>>                 tok += 7;
>>>>>> --
>>>>>> 2.34.1
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 

Re: [PATCH 1/2] perf pmu: Restore full PMU name wildcard support

[Namhyung Kim]

On Tue, Jun 18, 2024 at 04:47:01PM +0100, James Clark wrote:
> 
> 
> On 18/06/2024 16:25, Ian Rogers wrote:
> > On Tue, Jun 18, 2024, 8:06 AM James Clark <james.clark@arm.com> wrote:
> > 
> >>
> >>
> >> On 18/06/2024 15:23, Ian Rogers wrote:
> >>> On Tue, Jun 18, 2024, 3:59 AM James Clark <james.clark@arm.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 17/06/2024 22:25, Ian Rogers wrote:
> >>>>> On Mon, Jun 17, 2024, 6:44 AM James Clark <james.clark@arm.com> wrote:
> >>>>>
> >>>>>> Commit b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in
> >> dynamic
> >>>>>> pmu events") gives the following example for wildcarding a subset of
> >>>>>> PMUs:
> >>>>>>
> >>>>>>   E.g., in a system with the following dynamic pmus:
> >>>>>>
> >>>>>>         mypmu_0
> >>>>>>         mypmu_1
> >>>>>>         mypmu_2
> >>>>>>         mypmu_4
> >>>>>>
> >>>>>>   perf stat -e mypmu_[01]/<config>/
> >>>>>>
> >>>>>> Since commit f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()"),
> >> only
> >>>>>> "*" has been supported, removing the ability to subset PMUs, even
> >> though
> >>>>>> parse-events.l still supports ? and [] characters.
> >>>>>>
> >>>>>> Fix it by using fnmatch() when any glob character is detected and add
> >> a
> >>>>>> test which covers that and other scenarios of
> >>>>>> perf_pmu__match_ignoring_suffix().
> >>>>>>
> >>>>>> Fixes: f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()")
> >>>>>> Signed-off-by: James Clark <james.clark@arm.com>
> >>>>>>
> >>>>>
> >>>>> We use regular expression matching elsewhere rather than fnmatch. We
> >> can
> >>>>> also precompile the matchers using lex. I'm not sure we shouldn't be
> >>>>> looking for an opportunity to remove fnmatch rather than expand upon
> >> it.
> >>>>>
> >>>>> Thanks,
> >>>>> Ian
> >>>>>
> >>>>
> >>>> Presumably you mean we can do the removal of fnmatch after this fix goes
> >>>> in,
> >>>> rather than instead of? Because this is a user facing change in
> >> behaviour
> >>>> but the removal of fnmatch would be an non-user facing code refactor?
> >>>>
> >>>> It's technically not an "expansion" because we always used fnmatch and
> >> the
> >>>> linked commit hasn't made it to a release yet.
> >>>>
> >>>
> >>> The main place the expansion gets added is parse-events.c, previously
> >>> parse-events.y. If we're adding the expansion ourselves then we can
> >> choose
> >>> the form we add it. Some coming servers will have 100s of PMUs and so I'm
> >>> worried about the scanning cost when a PMU isn't specified.
> >>>
> >>> Thanks,
> >>> Ian
> >>>
> >>
> >> I think I might not be following. If a PMU isn't specified then
> >> perf_pmu__match() is never called so no cost is incurred. I also don't
> >> add any new calls to fnmatch().
> >>
> >> I only updated the gate on whether the existing fnmatch() is called from
> >> "*" to "*[?". So it only happens when one of those characters is in the
> >> PMU name, but it already happens when '*' is in the name.
> >>
> > 
> > Right. I'm not saying there is anything wrong in the change or an
> > additional cost, what the issue is is that currently only really '*'
> > requires fnmatch and that's because the event parsing adds it. It could
> 
> It's not only '*' that requires it, see the example I added in the
> commit message:
> 
>   ./perf stat -e mypmu_[01]/<config>/
> 
> '?' was supported before as well which could be useful.
> 
> > similarly add '.*' if we did regular expression matching. By expanding what
> > we pass to fnmatch from the command line the more committed we are to
> > fnmatch rather than regular expressions - which is what we use everywhere
> > else in the code. So maybe it was a feature that this wasn't working.
> > 
> 
> But we haven't had a release of Perf yet where more is passed to
> fnmatch(). Before f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()")
> everything was passed to fnmatch(). After that unreleased commit only
> things with '*' are. Now with this change only "*?[", so it's less not more.
> 
> I don't think there is any commitment to keep it, we can always remove
> fnmatch in the future. But it looks like a mistake to me because the
> title says "refactor" when it actually removes a feature.

Ian, are you ok with this now?

Thanks,
Namhyung

Re: [PATCH 1/2] perf pmu: Restore full PMU name wildcard support

[James Clark]

On 26/06/2024 05:13, Namhyung Kim wrote:
> On Tue, Jun 18, 2024 at 04:47:01PM +0100, James Clark wrote:
>>
>>
>> On 18/06/2024 16:25, Ian Rogers wrote:
>>> On Tue, Jun 18, 2024, 8:06 AM James Clark <james.clark@arm.com> wrote:
>>>
>>>>
>>>>
>>>> On 18/06/2024 15:23, Ian Rogers wrote:
>>>>> On Tue, Jun 18, 2024, 3:59 AM James Clark <james.clark@arm.com> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On 17/06/2024 22:25, Ian Rogers wrote:
>>>>>>> On Mon, Jun 17, 2024, 6:44 AM James Clark <james.clark@arm.com> wrote:
>>>>>>>
>>>>>>>> Commit b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in
>>>> dynamic
>>>>>>>> pmu events") gives the following example for wildcarding a subset of
>>>>>>>> PMUs:
>>>>>>>>
>>>>>>>>   E.g., in a system with the following dynamic pmus:
>>>>>>>>
>>>>>>>>         mypmu_0
>>>>>>>>         mypmu_1
>>>>>>>>         mypmu_2
>>>>>>>>         mypmu_4
>>>>>>>>
>>>>>>>>   perf stat -e mypmu_[01]/<config>/
>>>>>>>>
>>>>>>>> Since commit f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()"),
>>>> only
>>>>>>>> "*" has been supported, removing the ability to subset PMUs, even
>>>> though
>>>>>>>> parse-events.l still supports ? and [] characters.
>>>>>>>>
>>>>>>>> Fix it by using fnmatch() when any glob character is detected and add
>>>> a
>>>>>>>> test which covers that and other scenarios of
>>>>>>>> perf_pmu__match_ignoring_suffix().
>>>>>>>>
>>>>>>>> Fixes: f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()")
>>>>>>>> Signed-off-by: James Clark <james.clark@arm.com>
>>>>>>>>
>>>>>>>
>>>>>>> We use regular expression matching elsewhere rather than fnmatch. We
>>>> can
>>>>>>> also precompile the matchers using lex. I'm not sure we shouldn't be
>>>>>>> looking for an opportunity to remove fnmatch rather than expand upon
>>>> it.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ian
>>>>>>>
>>>>>>
>>>>>> Presumably you mean we can do the removal of fnmatch after this fix goes
>>>>>> in,
>>>>>> rather than instead of? Because this is a user facing change in
>>>> behaviour
>>>>>> but the removal of fnmatch would be an non-user facing code refactor?
>>>>>>
>>>>>> It's technically not an "expansion" because we always used fnmatch and
>>>> the
>>>>>> linked commit hasn't made it to a release yet.
>>>>>>
>>>>>
>>>>> The main place the expansion gets added is parse-events.c, previously
>>>>> parse-events.y. If we're adding the expansion ourselves then we can
>>>> choose
>>>>> the form we add it. Some coming servers will have 100s of PMUs and so I'm
>>>>> worried about the scanning cost when a PMU isn't specified.
>>>>>
>>>>> Thanks,
>>>>> Ian
>>>>>
>>>>
>>>> I think I might not be following. If a PMU isn't specified then
>>>> perf_pmu__match() is never called so no cost is incurred. I also don't
>>>> add any new calls to fnmatch().
>>>>
>>>> I only updated the gate on whether the existing fnmatch() is called from
>>>> "*" to "*[?". So it only happens when one of those characters is in the
>>>> PMU name, but it already happens when '*' is in the name.
>>>>
>>>
>>> Right. I'm not saying there is anything wrong in the change or an
>>> additional cost, what the issue is is that currently only really '*'
>>> requires fnmatch and that's because the event parsing adds it. It could
>>
>> It's not only '*' that requires it, see the example I added in the
>> commit message:
>>
>>   ./perf stat -e mypmu_[01]/<config>/
>>
>> '?' was supported before as well which could be useful.
>>
>>> similarly add '.*' if we did regular expression matching. By expanding what
>>> we pass to fnmatch from the command line the more committed we are to
>>> fnmatch rather than regular expressions - which is what we use everywhere
>>> else in the code. So maybe it was a feature that this wasn't working.
>>>
>>
>> But we haven't had a release of Perf yet where more is passed to
>> fnmatch(). Before f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()")
>> everything was passed to fnmatch(). After that unreleased commit only
>> things with '*' are. Now with this change only "*?[", so it's less not more.
>>
>> I don't think there is any commitment to keep it, we can always remove
>> fnmatch in the future. But it looks like a mistake to me because the
>> title says "refactor" when it actually removes a feature.
> 
> Ian, are you ok with this now?
> 
> Thanks,
> Namhyung
> 

I don't feel too strongly about this one, unless anyone really is still
using my_pmu[01].

For patch 2 I was going to resend with Ian's suggested-by tag, but I
think we should try to get that one in as a test is failing.

[PATCH 2/2] perf pmu: Don't de-duplicate core PMUs

[James Clark]

Arm PMUs have a suffix, either a single decimal (armv8_pmuv3_0) or 3 hex
digits which (armv8_cortex_a53) which Perf assumes are both strippable
suffixes for the purposes of deduplication. S390 "cpum_cf" is a
similarly suffixed core PMU but is only two characters so is not treated
as strippable because the rules are a minimum of 3 hex characters or 1
decimal character.

There are two paths involved in listing PMU events:

 * HW/cache event printing assumes core PMUs don't have suffixes so
   doesn't try to strip.
 * Sysfs PMU events share the printing function with uncore PMUs which
   strips.

This results in slightly inconsistent Perf list behavior if a core PMU
has a suffix:

  # perf list
  ...
  armv8_pmuv3_0/branch-load-misses/
  armv8_pmuv3/l3d_cache_wb/          [Kernel PMU event]
  ...

Fix it by partially reverting back to the old list behavior where
stripping was only done for uncore PMUs. For example commit 8d9f5146f5da
("perf pmus: Sort pmus by name then suffix") mentions that only PMUs
starting 'uncore_' are considered to have a potential suffix. This
change doesn't go back that far, but does only strip PMUs that are
!is_core. This keeps the desirable behavior where the many possibly
duplicated uncore PMUs aren't repeated, but it doesn't break listing for
core PMUs.

Searching for a PMU continues to use the new stripped comparison
functions, meaning that it's still possible to request an event by
specifying the common part of a PMU name, or even open events on
multiple similarly named PMUs. For example:

  # perf stat -e armv8_cortex/inst_retired/

  5777173628      armv8_cortex_a53/inst_retired/          (99.93%)
  7469626951      armv8_cortex_a57/inst_retired/          (49.88%)

Fixes: 3241d46f5f54 ("perf pmus: Sort/merge/aggregate PMUs like mrvl_ddr_pmu")
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/pmu.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 97d74fe6d816..b73946ba9d05 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -847,6 +847,22 @@ __weak const struct pmu_metrics_table *pmu_metrics_table__find(void)
 	return perf_pmu__find_metrics_table(NULL);
 }
 
+/**
+ * Return the length of the PMU name not including the suffix for uncore PMUs.
+ *
+ * We want to deduplicate many similar uncore PMUs by stripping their suffixes,
+ * but there are never going to be too many core PMUs and the suffixes might be
+ * interesting. "arm_cortex_a53" vs "arm_cortex_a57" or "cpum_cf" for example.
+ *
+ * @skip_duplicate_pmus: False in verbose mode so all uncore PMUs are visible
+ */
+static size_t pmu_deduped_name_len(const struct perf_pmu *pmu, bool skip_duplicate_pmus)
+{
+	return skip_duplicate_pmus && !pmu->is_core
+		? pmu_name_len_no_suffix(pmu->name)
+		: strlen(pmu->name);
+}
+
 /**
  * perf_pmu__match_ignoring_suffix - Does the pmu_name match tok ignoring any
  *                                   trailing suffix? The Suffix must be in form
@@ -1796,9 +1812,7 @@ static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
 			  const struct perf_pmu_alias *alias, bool skip_duplicate_pmus)
 {
 	struct parse_events_term *term;
-	size_t pmu_name_len = skip_duplicate_pmus
-		? pmu_name_len_no_suffix(pmu->name)
-		: strlen(pmu->name);
+	size_t pmu_name_len = pmu_deduped_name_len(pmu, skip_duplicate_pmus);
 	int used = snprintf(buf, len, "%.*s/%s", (int)pmu_name_len, pmu->name, alias->name);
 
 	list_for_each_entry(term, &alias->terms.terms, list) {
@@ -1839,9 +1853,7 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
 		size_t buf_used, pmu_name_len;
 
 		info.pmu_name = event->pmu_name ?: pmu->name;
-		pmu_name_len = skip_duplicate_pmus
-			? pmu_name_len_no_suffix(info.pmu_name)
-			: strlen(info.pmu_name);
+		pmu_name_len = pmu_deduped_name_len(pmu, skip_duplicate_pmus);
 		info.alias = NULL;
 		if (event->desc) {
 			info.name = event->name;
-- 
2.34.1

Re: [PATCH 2/2] perf pmu: Don't de-duplicate core PMUs

[James Clark]

On 17/06/2024 22:30, Ian Rogers wrote:
> On Mon, Jun 17, 2024, 6:44 AM James Clark <james.clark@arm.com> wrote:
> 
>> Arm PMUs have a suffix, either a single decimal (armv8_pmuv3_0) or 3 hex
>> digits which (armv8_cortex_a53) which Perf assumes are both strippable
>> suffixes for the purposes of deduplication. S390 "cpum_cf" is a
>> similarly suffixed core PMU but is only two characters so is not treated
>> as strippable because the rules are a minimum of 3 hex characters or 1
>> decimal character.
>>
>> There are two paths involved in listing PMU events:
>>
>>  * HW/cache event printing assumes core PMUs don't have suffixes so
>>    doesn't try to strip.
>>  * Sysfs PMU events share the printing function with uncore PMUs which
>>    strips.
>>
>> This results in slightly inconsistent Perf list behavior if a core PMU
>> has a suffix:
>>
>>   # perf list
>>   ...
>>   armv8_pmuv3_0/branch-load-misses/
>>   armv8_pmuv3/l3d_cache_wb/          [Kernel PMU event]
>>   ...
>>
>> Fix it by partially reverting back to the old list behavior where
>> stripping was only done for uncore PMUs. For example commit 8d9f5146f5da
>> ("perf pmus: Sort pmus by name then suffix") mentions that only PMUs
>> starting 'uncore_' are considered to have a potential suffix. This
>> change doesn't go back that far, but does only strip PMUs that are
>> !is_core. This keeps the desirable behavior where the many possibly
>> duplicated uncore PMUs aren't repeated, but it doesn't break listing for
>> core PMUs.
>>
>> Searching for a PMU continues to use the new stripped comparison
>> functions, meaning that it's still possible to request an event by
>> specifying the common part of a PMU name, or even open events on
>> multiple similarly named PMUs. For example:
>>
>>   # perf stat -e armv8_cortex/inst_retired/
>>
>>   5777173628      armv8_cortex_a53/inst_retired/          (99.93%)
>>   7469626951      armv8_cortex_a57/inst_retired/          (49.88%)
>>
>> Fixes: 3241d46f5f54 ("perf pmus: Sort/merge/aggregate PMUs like
>> mrvl_ddr_pmu")
>> Signed-off-by: James Clark <james.clark@arm.com>
>>
> 
> Should this have my suggested by tag?
> 
> Thanks,
> Ian

Yep, you're right I can add it

> 
> ---
>>  tools/perf/util/pmu.c | 24 ++++++++++++++++++------
>>  1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 97d74fe6d816..b73946ba9d05 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -847,6 +847,22 @@ __weak const struct pmu_metrics_table
>> *pmu_metrics_table__find(void)
>>         return perf_pmu__find_metrics_table(NULL);
>>  }
>>
>> +/**
>> + * Return the length of the PMU name not including the suffix for uncore
>> PMUs.
>> + *
>> + * We want to deduplicate many similar uncore PMUs by stripping their
>> suffixes,
>> + * but there are never going to be too many core PMUs and the suffixes
>> might be
>> + * interesting. "arm_cortex_a53" vs "arm_cortex_a57" or "cpum_cf" for
>> example.
>> + *
>> + * @skip_duplicate_pmus: False in verbose mode so all uncore PMUs are
>> visible
>> + */
>> +static size_t pmu_deduped_name_len(const struct perf_pmu *pmu, bool
>> skip_duplicate_pmus)
>> +{
>> +       return skip_duplicate_pmus && !pmu->is_core
>> +               ? pmu_name_len_no_suffix(pmu->name)
>> +               : strlen(pmu->name);
>> +}
>> +
>>  /**
>>   * perf_pmu__match_ignoring_suffix - Does the pmu_name match tok ignoring
>> any
>>   *                                   trailing suffix? The Suffix must be
>> in form
>> @@ -1796,9 +1812,7 @@ static char *format_alias(char *buf, int len, const
>> struct perf_pmu *pmu,
>>                           const struct perf_pmu_alias *alias, bool
>> skip_duplicate_pmus)
>>  {
>>         struct parse_events_term *term;
>> -       size_t pmu_name_len = skip_duplicate_pmus
>> -               ? pmu_name_len_no_suffix(pmu->name)
>> -               : strlen(pmu->name);
>> +       size_t pmu_name_len = pmu_deduped_name_len(pmu,
>> skip_duplicate_pmus);
>>         int used = snprintf(buf, len, "%.*s/%s", (int)pmu_name_len,
>> pmu->name, alias->name);
>>
>>         list_for_each_entry(term, &alias->terms.terms, list) {
>> @@ -1839,9 +1853,7 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu,
>> bool skip_duplicate_pmus,
>>                 size_t buf_used, pmu_name_len;
>>
>>                 info.pmu_name = event->pmu_name ?: pmu->name;
>> -               pmu_name_len = skip_duplicate_pmus
>> -                       ? pmu_name_len_no_suffix(info.pmu_name)
>> -                       : strlen(info.pmu_name);
>> +               pmu_name_len = pmu_deduped_name_len(pmu,
>> skip_duplicate_pmus);
>>                 info.alias = NULL;
>>                 if (event->desc) {
>>                         info.name = event->name;
>> --
>> 2.34.1
>>
>>
>