Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210517140931.2559364-1-tmricht@linux.ibm.com>
In-Reply-To: <20210517140931.2559364-1-tmricht@linux.ibm.com>
From:   Ian Rogers <irogers@google.com>
Date:   Tue, 18 May 2021 16:30:35 -0700
Message-ID: <CAP-5=fV-2J1rxHAaGv_GwFzY-mAKKogEvQf1A87PUvZxT7wzUQ@mail.gmail.com>
Subject: Re: [PATCH] perf test: Test libpfm4 support (63) reports error
To:     Thomas Richter <tmricht@linux.ibm.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        linux-perf-users <linux-perf-users@vger.kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Stephane Eranian <eranian@google.com>, svens@linux.ibm.com,
        Vasily Gorbik <gor@linux.ibm.com>, sumanthk@linux.ibm.com,
        hca@linux.ibm.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>