* [PATCH] perf test: Test libpfm4 support (63) reports error
@ 2021-05-17 14:09 Thomas Richter
2021-05-18 23:30 ` Ian Rogers
0 siblings, 1 reply; 3+ messages in thread
From: Thomas Richter @ 2021-05-17 14:09 UTC (permalink / raw)
To: linux-kernel, linux-perf-users, acme, eranian
Cc: svens, gor, sumanthk, hca, Thomas Richter
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'
# ./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.
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] perf test: Test libpfm4 support (63) reports error
2021-05-17 14:09 [PATCH] perf test: Test libpfm4 support (63) reports error Thomas Richter
@ 2021-05-18 23:30 ` Ian Rogers
2021-05-19 13:29 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 3+ messages in thread
From: Ian Rogers @ 2021-05-18 23:30 UTC (permalink / raw)
To: Thomas Richter
Cc: LKML, linux-perf-users, Arnaldo Carvalho de Melo,
Stephane Eranian, svens, Vasily Gorbik, sumanthk, hca
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.
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] perf test: Test libpfm4 support (63) reports error
2021-05-18 23:30 ` Ian Rogers
@ 2021-05-19 13:29 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 3+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-05-19 13:29 UTC (permalink / raw)
To: Ian Rogers
Cc: Thomas Richter, LKML, linux-perf-users, Stephane Eranian, svens,
Vasily Gorbik, sumanthk, hca
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-19 13:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-17 14:09 [PATCH] perf test: Test libpfm4 support (63) reports error Thomas Richter
2021-05-18 23:30 ` Ian Rogers
2021-05-19 13:29 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).