Messages in this thread | | | Date | Wed, 19 May 2021 10:29:21 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH] perf test: Test libpfm4 support (63) reports error |
| |
Em Tue, May 18, 2021 at 04:30:35PM -0700, Ian Rogers escreveu: > On Mon, May 17, 2021 at 7:12 AM Thomas Richter <tmricht@linux.ibm.com> wrote: > > > > Compiling perf with make LIBPFM4=1 includes libpfm support and > > enables test case 63 'Test libpfm4 support'. This test reports an error > > on all platforms for subtest 63.2 'test groups of --pfm-events'. > > The reported error message is 'nested event groups not supported' > > The parsing test checks broken and working strings and so errors are > always going to be reported, but agreed this error is wrong. > > > # ./perf test -F 63 > > 63: Test libpfm4 support : > > 63.1: test of individual --pfm-events : > > Error: > > failed to parse event stereolab : event not found > > Error: > > failed to parse event stereolab,instructions : event not found > > Error: > > failed to parse event instructions,stereolab : event not found > > Ok > > 63.2: test groups of --pfm-events : > > Error: > > nested event groups not supported <------ Error message here > > Error: > > failed to parse event {stereolab} : event not found > > Error: > > failed to parse event {instructions,cycles},{instructions,stereolab} :\ > > event not found > > Ok > > # > > > > This patch addresses the error message 'nested event groups not supported'. > > The root cause is function parse_libpfm_events_option() which parses the > > event string '{},{instructions}' and can not handle a leading empty > > group notation '{},...'. > > > > The code detects the first (empty) group indicator '{' but does not > > terminate group processing on the following group closing character '}'. > > So when the second group indicator '{' is detected, the code assumes > > a nested group and returns an error. > > > > With the error message fixed, also change the expected event number to > > one for the test case to succeed. > > > > While at it also fix a memory leak. In good case the function does not > > free the duplicated string given as first parameter. > > > > Output after: > > # ./perf test -F 63 > > 63: Test libpfm4 support : > > 63.1: test of individual --pfm-events : > > Error: > > failed to parse event stereolab : event not found > > Error: > > failed to parse event stereolab,instructions : event not found > > Error: > > failed to parse event instructions,stereolab : event not found > > Ok > > 63.2: test groups of --pfm-events : > > Error: > > failed to parse event {stereolab} : event not found > > Error: > > failed to parse event {instructions,cycles},{instructions,stereolab} : \ > > event not found > > Ok > > # > > Error message 'nested event groups not supported' is gone. > > Acked-By: Ian Rogers <irogers@google.com> > > I wonder if we should add some coverage for the error cases to the pfm > test with something like the following.
Yeah, agreed, please consider sending a patch for that.
Thanks, applied.
- Arnaldo
> Thanks, > Ian > > --- a/tools/perf/tests/pfm.c > +++ b/tools/perf/tests/pfm.c > @@ -155,6 +155,16 @@ static int test__pfm_group(void) > .nr_events = 3, > .nr_groups = 1, > }, > + { > + .events = "instructions}", > + .nr_events = 1, > + .nr_groups = 0, > + }, > + { > + .events = "{{instructions}}", > + .nr_events = 0, > + .nr_groups = 0, > + }, > }; > > for (i = 0; i < ARRAY_SIZE(table); i++) { > > > Signed-off-by: Thomas Richter <tmricht@linux.ibm.com> > > Acked-By: Sumanth Korikkar <sumanthk@linux.ibm.com> > > --- > > tools/perf/tests/pfm.c | 4 ++-- > > tools/perf/util/pfm.c | 11 ++++++++++- > > 2 files changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/tools/perf/tests/pfm.c b/tools/perf/tests/pfm.c > > index 76a53126efdf..d4b0ef74defc 100644 > > --- a/tools/perf/tests/pfm.c > > +++ b/tools/perf/tests/pfm.c > > @@ -131,8 +131,8 @@ static int test__pfm_group(void) > > }, > > { > > .events = "{},{instructions}", > > - .nr_events = 0, > > - .nr_groups = 0, > > + .nr_events = 1, > > + .nr_groups = 1, > > }, > > { > > .events = "{instructions},{instructions}", > > diff --git a/tools/perf/util/pfm.c b/tools/perf/util/pfm.c > > index d735acb6c29c..6eef6dfeaa57 100644 > > --- a/tools/perf/util/pfm.c > > +++ b/tools/perf/util/pfm.c > > @@ -62,8 +62,16 @@ int parse_libpfm_events_option(const struct option *opt, const char *str, > > } > > > > /* no event */ > > - if (*q == '\0') > > + if (*q == '\0') { > > + if (*sep == '}') { > > + if (grp_evt < 0) { > > + ui__error("cannot close a non-existing event group\n"); > > + goto error; > > + } > > + grp_evt--; > > + } > > continue; > > + } > > > > memset(&attr, 0, sizeof(attr)); > > event_attr_init(&attr); > > @@ -107,6 +115,7 @@ int parse_libpfm_events_option(const struct option *opt, const char *str, > > grp_evt = -1; > > } > > } > > + free(p_orig); > > return 0; > > error: > > free(p_orig); > > -- > > 2.31.1 > >
--
- Arnaldo
| |