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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 Oct 2022 16:07:04 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     John Garry <john.garry@...wei.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ravi Bangoria <ravi.bangoria@....com>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v1] perf test: Parse events workaround for dash/minus

On Thu, Oct 13, 2022 at 5:11 AM John Garry <john.garry@...wei.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@...gle.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;
>