perf test: Parse events workaround for dash/minus

[PATCH v1] perf test: Parse events workaround for dash/minus

Posted by Ian Rogers 3 years, 5 months ago

Skip an event configuration for event names with a dash/minus in them.
Events with a dash/minus in their name cause parsing issues as legacy
encoding of events would use a dash/minus as a separator. The parser
separates events with dashes into prefixes and suffixes and then
recombines them. Unfortunately if an event has part of its name that
matches a legacy token then the recombining fails. This is seen for
branch-brs where branch is a legacy token. branch-brs was introduced
to sysfs in:
https://lore.kernel.org/all/20220322221517.2510440-5-eranian@google.com/
The failure is shown below as well as the workaround to use a config
where the dash/minus isn't treated specially:

```
$ perf stat -e branch-brs true
event syntax error: 'branch-brs'
                           \___ parser error

$ perf stat -e cpu/branch-brs/ true

 Performance counter stats for 'true':

            46,179      cpu/branch-brs/
```

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/parse-events.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 459afdb256a1..3440dd2616b0 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -2237,6 +2237,19 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
 			pr_debug("Test PMU event failed for '%s'", name);
 			ret = combine_test_results(ret, test_ret);
 		}
+		/*
+		 * Names containing '-' are recognized as prefixes and suffixes
+		 * due to '-' being a legacy PMU separator. This fails when the
+		 * prefix or suffix collides with an existing legacy token. For
+		 * example, branch-brs has a prefix (branch) that collides with
+		 * a PE_NAME_CACHE_TYPE token causing a parse error as a suffix
+		 * isn't expected after this. As event names in the config
+		 * slashes are allowed a '-' in the name we check this works
+		 * above.
+		 */
+		if (strchr(ent->d_name, '-'))
+			continue;
+
 		snprintf(name, sizeof(name), "%s:u,cpu/event=%s/u", ent->d_name, ent->d_name);
 		e.name  = name;
 		e.check = test__checkevent_pmu_events_mix;
-- 
2.38.0.rc1.362.ged0d419d3c-goog

Re: [PATCH v1] perf test: Parse events workaround for dash/minus

Posted by John Garry 3 years, 5 months ago

On 13/10/2022 02:12, Ian Rogers wrote:
> Skip an event configuration for event names with a dash/minus in them.
> Events with a dash/minus in their name cause parsing issues as legacy
> encoding of events would use a dash/minus as a separator. The parser
> separates events with dashes into prefixes and suffixes and then
> recombines them. Unfortunately if an event has part of its name that
> matches a legacy token then the recombining fails. This is seen for
> branch-brs where branch is a legacy token. branch-brs was introduced
> to sysfs in:
> https://lore.kernel.org/all/20220322221517.2510440-5-eranian@google.com/
> The failure is shown below as well as the workaround to use a config
> where the dash/minus isn't treated specially:
> 
> ```
> $ perf stat -e branch-brs true
> event syntax error: 'branch-brs'
>                             \___ parser error
> 
> $ perf stat -e cpu/branch-brs/ true
> 
>   Performance counter stats for 'true':
> 
>              46,179      cpu/branch-brs/
> ```
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/tests/parse-events.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 459afdb256a1..3440dd2616b0 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -2237,6 +2237,19 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
>   			pr_debug("Test PMU event failed for '%s'", name);
>   			ret = combine_test_results(ret, test_ret);
>   		

Hi Ian,

}
> +		/*
> +		 * Names containing '-' are recognized as prefixes and suffixes
> +		 * due to '-' being a legacy PMU separator. This fails when the
> +		 * prefix or suffix collides with an existing legacy token. For
> +		 * example, branch-brs has a prefix (branch) that collides with
> +		 * a PE_NAME_CACHE_TYPE token causing a parse error as a suffix
> +		 * isn't expected after this.

OK, so you want to skip anything with '-'. Will we now miss out on 
events which contain a '-' but don't clash with an existing legacy token?

>  As event names in the config
> +		 * slashes are allowed a '-' in the name we check this works
> +		 * above.

Sorry, I can't follow what you mean here. Do you mean that 
"cpu/branch-brs/" works ok and we continue to test it?

> +		 */
> +		if (strchr(ent->d_name, '-'))
> +			continue;

Thanks,
John

> +
>   		snprintf(name, sizeof(name), "%s:u,cpu/event=%s/u", ent->d_name, ent->d_name);
>   		e.name  = name;
>   		e.check = test__checkevent_pmu_events_mix;

Re: [PATCH v1] perf test: Parse events workaround for dash/minus

Posted by Ian Rogers 3 years, 5 months ago

On Thu, Oct 13, 2022 at 5:11 AM John Garry <john.garry@huawei.com> wrote:
>
> On 13/10/2022 02:12, Ian Rogers wrote:
> > Skip an event configuration for event names with a dash/minus in them.
> > Events with a dash/minus in their name cause parsing issues as legacy
> > encoding of events would use a dash/minus as a separator. The parser
> > separates events with dashes into prefixes and suffixes and then
> > recombines them. Unfortunately if an event has part of its name that
> > matches a legacy token then the recombining fails. This is seen for
> > branch-brs where branch is a legacy token. branch-brs was introduced
> > to sysfs in:
> > https://lore.kernel.org/all/20220322221517.2510440-5-eranian@google.com/
> > The failure is shown below as well as the workaround to use a config
> > where the dash/minus isn't treated specially:
> >
> > ```
> > $ perf stat -e branch-brs true
> > event syntax error: 'branch-brs'
> >                             \___ parser error
> >
> > $ perf stat -e cpu/branch-brs/ true
> >
> >   Performance counter stats for 'true':
> >
> >              46,179      cpu/branch-brs/
> > ```
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >   tools/perf/tests/parse-events.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> >
> > diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> > index 459afdb256a1..3440dd2616b0 100644
> > --- a/tools/perf/tests/parse-events.c
> > +++ b/tools/perf/tests/parse-events.c
> > @@ -2237,6 +2237,19 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
> >                       pr_debug("Test PMU event failed for '%s'", name);
> >                       ret = combine_test_results(ret, test_ret);
> >
>
> Hi Ian,
>
> }
> > +             /*
> > +              * Names containing '-' are recognized as prefixes and suffixes
> > +              * due to '-' being a legacy PMU separator. This fails when the
> > +              * prefix or suffix collides with an existing legacy token. For
> > +              * example, branch-brs has a prefix (branch) that collides with
> > +              * a PE_NAME_CACHE_TYPE token causing a parse error as a suffix
> > +              * isn't expected after this.
>
> OK, so you want to skip anything with '-'. Will we now miss out on
> events which contain a '-' but don't clash with an existing legacy token?

Yes. The long term solution here is to ditch the current parse event
code and its legacy support. An idea is to create a simplified parser
as part of libperf2 (libperf but with a libbpf style license), use the
new parser for metrics and events. In the events case, if the new
parser fails then use the legacy parser and its weird handling of
dashes.

> >  As event names in the config
> > +              * slashes are allowed a '-' in the name we check this works
> > +              * above.
>
> Sorry, I can't follow what you mean here. Do you mean that
> "cpu/branch-brs/" works ok and we continue to test it?

Yep, as shown in the commit message. The comment isn't great, it
should be worded something like:

If an event name is in the config slashes, such as cpu/branch-brs/,  a
'-' in the name works as the dash doesn't separate the parts of the
name. This continues to be tested above.

> > +              * above.

Thanks,
Ian

> > +              */
> > +             if (strchr(ent->d_name, '-'))
> > +                     continue;
>
> Thanks,
> John
>
> > +
> >               snprintf(name, sizeof(name), "%s:u,cpu/event=%s/u", ent->d_name, ent->d_name);
> >               e.name  = name;
> >               e.check = test__checkevent_pmu_events_mix;
>

Re: [PATCH v1] perf test: Parse events workaround for dash/minus

Posted by Ian Rogers 3 years, 5 months ago

On Mon, Oct 17, 2022 at 4:07 PM Ian Rogers <irogers@google.com> wrote:
>
> On Thu, Oct 13, 2022 at 5:11 AM John Garry <john.garry@huawei.com> wrote:
> >
> > On 13/10/2022 02:12, Ian Rogers wrote:
> > > Skip an event configuration for event names with a dash/minus in them.
> > > Events with a dash/minus in their name cause parsing issues as legacy
> > > encoding of events would use a dash/minus as a separator. The parser
> > > separates events with dashes into prefixes and suffixes and then
> > > recombines them. Unfortunately if an event has part of its name that
> > > matches a legacy token then the recombining fails. This is seen for
> > > branch-brs where branch is a legacy token. branch-brs was introduced
> > > to sysfs in:
> > > https://lore.kernel.org/all/20220322221517.2510440-5-eranian@google.com/
> > > The failure is shown below as well as the workaround to use a config
> > > where the dash/minus isn't treated specially:
> > >
> > > ```
> > > $ perf stat -e branch-brs true
> > > event syntax error: 'branch-brs'
> > >                             \___ parser error
> > >
> > > $ perf stat -e cpu/branch-brs/ true
> > >
> > >   Performance counter stats for 'true':
> > >
> > >              46,179      cpu/branch-brs/
> > > ```
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >   tools/perf/tests/parse-events.c | 13 +++++++++++++
> > >   1 file changed, 13 insertions(+)
> > >
> > > diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> > > index 459afdb256a1..3440dd2616b0 100644
> > > --- a/tools/perf/tests/parse-events.c
> > > +++ b/tools/perf/tests/parse-events.c
> > > @@ -2237,6 +2237,19 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
> > >                       pr_debug("Test PMU event failed for '%s'", name);
> > >                       ret = combine_test_results(ret, test_ret);
> > >
> >
> > Hi Ian,
> >
> > }
> > > +             /*
> > > +              * Names containing '-' are recognized as prefixes and suffixes
> > > +              * due to '-' being a legacy PMU separator. This fails when the
> > > +              * prefix or suffix collides with an existing legacy token. For
> > > +              * example, branch-brs has a prefix (branch) that collides with
> > > +              * a PE_NAME_CACHE_TYPE token causing a parse error as a suffix
> > > +              * isn't expected after this.
> >
> > OK, so you want to skip anything with '-'. Will we now miss out on
> > events which contain a '-' but don't clash with an existing legacy token?
>
> Yes. The long term solution here is to ditch the current parse event
> code and its legacy support. An idea is to create a simplified parser
> as part of libperf2 (libperf but with a libbpf style license), use the
> new parser for metrics and events. In the events case, if the new
> parser fails then use the legacy parser and its weird handling of
> dashes.
>
> > >  As event names in the config
> > > +              * slashes are allowed a '-' in the name we check this works
> > > +              * above.
> >
> > Sorry, I can't follow what you mean here. Do you mean that
> > "cpu/branch-brs/" works ok and we continue to test it?
>
> Yep, as shown in the commit message. The comment isn't great, it
> should be worded something like:
>
> If an event name is in the config slashes, such as cpu/branch-brs/,  a
> '-' in the name works as the dash doesn't separate the parts of the
> name. This continues to be tested above.

Ping. It'd be nice to merge this as otherwise we have a perf test
failure on AMD.

Thanks,
Ian

> > > +              * above.
>
> Thanks,
> Ian
>
> > > +              */
> > > +             if (strchr(ent->d_name, '-'))
> > > +                     continue;
> >
> > Thanks,
> > John
> >
> > > +
> > >               snprintf(name, sizeof(name), "%s:u,cpu/event=%s/u", ent->d_name, ent->d_name);
> > >               e.name  = name;
> > >               e.check = test__checkevent_pmu_events_mix;
> >

Re: [PATCH v1] perf test: Parse events workaround for dash/minus

Posted by Arnaldo Carvalho de Melo 3 years, 5 months ago

Em Fri, Oct 28, 2022 at 10:55:14AM -0700, Ian Rogers escreveu:
> On Mon, Oct 17, 2022 at 4:07 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Thu, Oct 13, 2022 at 5:11 AM John Garry <john.garry@huawei.com> wrote:
> > >
> > > On 13/10/2022 02:12, Ian Rogers wrote:
> > > > Skip an event configuration for event names with a dash/minus in them.
> > > > Events with a dash/minus in their name cause parsing issues as legacy
> > > > encoding of events would use a dash/minus as a separator. The parser
> > > > separates events with dashes into prefixes and suffixes and then
> > > > recombines them. Unfortunately if an event has part of its name that
> > > > matches a legacy token then the recombining fails. This is seen for
> > > > branch-brs where branch is a legacy token. branch-brs was introduced
> > > > to sysfs in:
> > > > https://lore.kernel.org/all/20220322221517.2510440-5-eranian@google.com/
> > > > The failure is shown below as well as the workaround to use a config
> > > > where the dash/minus isn't treated specially:
> > > >
> > > > ```
> > > > $ perf stat -e branch-brs true
> > > > event syntax error: 'branch-brs'
> > > >                             \___ parser error
> > > >
> > > > $ perf stat -e cpu/branch-brs/ true
> > > >
> > > >   Performance counter stats for 'true':
> > > >
> > > >              46,179      cpu/branch-brs/
> > > > ```
> > > >
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > >   tools/perf/tests/parse-events.c | 13 +++++++++++++
> > > >   1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> > > > index 459afdb256a1..3440dd2616b0 100644
> > > > --- a/tools/perf/tests/parse-events.c
> > > > +++ b/tools/perf/tests/parse-events.c
> > > > @@ -2237,6 +2237,19 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
> > > >                       pr_debug("Test PMU event failed for '%s'", name);
> > > >                       ret = combine_test_results(ret, test_ret);
> > > >
> > >
> > > Hi Ian,
> > >
> > > }
> > > > +             /*
> > > > +              * Names containing '-' are recognized as prefixes and suffixes
> > > > +              * due to '-' being a legacy PMU separator. This fails when the
> > > > +              * prefix or suffix collides with an existing legacy token. For
> > > > +              * example, branch-brs has a prefix (branch) that collides with
> > > > +              * a PE_NAME_CACHE_TYPE token causing a parse error as a suffix
> > > > +              * isn't expected after this.
> > >
> > > OK, so you want to skip anything with '-'. Will we now miss out on
> > > events which contain a '-' but don't clash with an existing legacy token?
> >
> > Yes. The long term solution here is to ditch the current parse event
> > code and its legacy support. An idea is to create a simplified parser
> > as part of libperf2 (libperf but with a libbpf style license), use the
> > new parser for metrics and events. In the events case, if the new
> > parser fails then use the legacy parser and its weird handling of
> > dashes.
> >
> > > >  As event names in the config
> > > > +              * slashes are allowed a '-' in the name we check this works
> > > > +              * above.
> > >
> > > Sorry, I can't follow what you mean here. Do you mean that
> > > "cpu/branch-brs/" works ok and we continue to test it?
> >
> > Yep, as shown in the commit message. The comment isn't great, it
> > should be worded something like:
> >
> > If an event name is in the config slashes, such as cpu/branch-brs/,  a
> > '-' in the name works as the dash doesn't separate the parts of the
> > name. This continues to be tested above.
> 
> Ping. It'd be nice to merge this as otherwise we have a perf test
> failure on AMD.

I added it for perf/core, i.e. 6.2.

Thanks,

- Arnaldo