If the kernel exposes a new perf_event_attr field in a format attr, perf
will return an error stating the specified PMU can't be found. For
example, a format attr with 'config3:0-63' causes an error if config3 is
unknown to perf. This causes a compatibility issue between a newer
kernel and an older perf tool.

The addition here makes any attr string up to the ':' ignored, but
still checks the 'bits' portion.

Signed-off-by: Rob Herring <robh@kernel.org>
---
This is the YACC mud I threw and seems to stick. Maybe there's a better 
way to handle this. It doesn't seem like there's a way to do wildcards 
(i.e. config.*) in YACC.

This is needed for this series[1]. Unfortunately the best we do to avoid 
the issue is applying this to stable. I think there's some time before 
v8.7 h/w is deployed, too.

Rob

[1] https://lore.kernel.org/all/20220825-arm-spe-v8-7-v1-0-c75b8d92e692@kernel.org/

 tools/perf/util/pmu.y | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/pmu.y b/tools/perf/util/pmu.y
index bfd7e8509869..3096864ec9b9 100644
--- a/tools/perf/util/pmu.y
+++ b/tools/perf/util/pmu.y
@@ -60,6 +60,9 @@ PP_CONFIG2 ':' bits
 				      PERF_PMU_FORMAT_VALUE_CONFIG2,
 				      $3));
 }
+|
+error ':' bits
+{}
 
 bits:
 bits ',' bit_term
-- 
2.34.1

Hello,

On Thu, Sep 1, 2022 at 11:55 AM Rob Herring <robh@kernel.org> wrote:
>
> If the kernel exposes a new perf_event_attr field in a format attr, perf
> will return an error stating the specified PMU can't be found. For
> example, a format attr with 'config3:0-63' causes an error if config3 is
> unknown to perf. This causes a compatibility issue between a newer
> kernel and an older perf tool.
>
> The addition here makes any attr string up to the ':' ignored, but
> still checks the 'bits' portion.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> This is the YACC mud I threw and seems to stick. Maybe there's a better
> way to handle this. It doesn't seem like there's a way to do wildcards
> (i.e. config.*) in YACC.
>
> This is needed for this series[1]. Unfortunately the best we do to avoid
> the issue is applying this to stable. I think there's some time before
> v8.7 h/w is deployed, too.

Maybe you could change the format_term rule to take an identifier instead
of PP_CONFIG* directly and pass it to perf_pmu__new_format().  Then
it could check the string and create an appropriate PERF_PMU_FORMAT_VALUE_*
or ignore it according to the PERF_ATTR_SIZE_VER*.

Thanks,
Namhyung


>
> Rob
>
> [1] https://lore.kernel.org/all/20220825-arm-spe-v8-7-v1-0-c75b8d92e692@kernel.org/
>
>  tools/perf/util/pmu.y | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/tools/perf/util/pmu.y b/tools/perf/util/pmu.y
> index bfd7e8509869..3096864ec9b9 100644
> --- a/tools/perf/util/pmu.y
> +++ b/tools/perf/util/pmu.y
> @@ -60,6 +60,9 @@ PP_CONFIG2 ':' bits
>                                       PERF_PMU_FORMAT_VALUE_CONFIG2,
>                                       $3));
>  }
> +|
> +error ':' bits
> +{}
>
>  bits:
>  bits ',' bit_term
> --
> 2.34.1
>

On Fri, Sep 2, 2022 at 1:53 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> On Thu, Sep 1, 2022 at 11:55 AM Rob Herring <robh@kernel.org> wrote:
> >
> > If the kernel exposes a new perf_event_attr field in a format attr, perf
> > will return an error stating the specified PMU can't be found. For
> > example, a format attr with 'config3:0-63' causes an error if config3 is
> > unknown to perf. This causes a compatibility issue between a newer
> > kernel and an older perf tool.
> >
> > The addition here makes any attr string up to the ':' ignored, but
> > still checks the 'bits' portion.
> >
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > This is the YACC mud I threw and seems to stick. Maybe there's a better
> > way to handle this. It doesn't seem like there's a way to do wildcards
> > (i.e. config.*) in YACC.
> >
> > This is needed for this series[1]. Unfortunately the best we do to avoid
> > the issue is applying this to stable. I think there's some time before
> > v8.7 h/w is deployed, too.
>
> Maybe you could change the format_term rule to take an identifier instead
> of PP_CONFIG* directly and pass it to perf_pmu__new_format().  Then
> it could check the string and create an appropriate PERF_PMU_FORMAT_VALUE_*
> or ignore it according to the PERF_ATTR_SIZE_VER*.

That only moves parsing of configN from YACC to strcmp in C. In doing
so, we'd be left with just the 'error' token case which seems a bit
odd (if there's another way to do it, I don't know. yacc is not my
thing). Is that really better? Unless there is some way to retrieve
the PERF_ATTR_SIZE_VER* from the kernel at runtime?

Rob

On Fri, Sep 2, 2022 at 8:25 AM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Sep 2, 2022 at 1:53 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hello,
> >
> > On Thu, Sep 1, 2022 at 11:55 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > If the kernel exposes a new perf_event_attr field in a format attr, perf
> > > will return an error stating the specified PMU can't be found. For
> > > example, a format attr with 'config3:0-63' causes an error if config3 is
> > > unknown to perf. This causes a compatibility issue between a newer
> > > kernel and an older perf tool.
> > >
> > > The addition here makes any attr string up to the ':' ignored, but
> > > still checks the 'bits' portion.
> > >
> > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > ---
> > > This is the YACC mud I threw and seems to stick. Maybe there's a better
> > > way to handle this. It doesn't seem like there's a way to do wildcards
> > > (i.e. config.*) in YACC.
> > >
> > > This is needed for this series[1]. Unfortunately the best we do to avoid
> > > the issue is applying this to stable. I think there's some time before
> > > v8.7 h/w is deployed, too.
> >
> > Maybe you could change the format_term rule to take an identifier instead
> > of PP_CONFIG* directly and pass it to perf_pmu__new_format().  Then
> > it could check the string and create an appropriate PERF_PMU_FORMAT_VALUE_*
> > or ignore it according to the PERF_ATTR_SIZE_VER*.
>
> That only moves parsing of configN from YACC to strcmp in C. In doing
> so, we'd be left with just the 'error' token case which seems a bit
> odd (if there's another way to do it, I don't know. yacc is not my
> thing). Is that really better?

I thought we could do more flexible handling and detailed error reporting
in the C code.  But it could be done in the lex/yacc as well..

I think the general idea is that we want to run a more recent version of
perf tools than the kernel.  So if it detects the tool is older, it can show
a warning message like:

"config3 is not in the perf_event_attr.. skipping.
 Maybe you're running on a newer kernel. Please upgrade the perf tool."


> Unless there is some way to retrieve
> the PERF_ATTR_SIZE_VER* from the kernel at runtime?

Even if it can retrieve the info at runtime, perf tool might not know
how to use the new config term.

Thanks,
Namhyung

On Tue, Sep 6, 2022 at 1:16 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Fri, Sep 2, 2022 at 8:25 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Sep 2, 2022 at 1:53 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Hello,
> > >
> > > On Thu, Sep 1, 2022 at 11:55 AM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > If the kernel exposes a new perf_event_attr field in a format attr, perf
> > > > will return an error stating the specified PMU can't be found. For
> > > > example, a format attr with 'config3:0-63' causes an error if config3 is
> > > > unknown to perf. This causes a compatibility issue between a newer
> > > > kernel and an older perf tool.
> > > >
> > > > The addition here makes any attr string up to the ':' ignored, but
> > > > still checks the 'bits' portion.
> > > >
> > > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > > ---
> > > > This is the YACC mud I threw and seems to stick. Maybe there's a better
> > > > way to handle this. It doesn't seem like there's a way to do wildcards
> > > > (i.e. config.*) in YACC.
> > > >
> > > > This is needed for this series[1]. Unfortunately the best we do to avoid
> > > > the issue is applying this to stable. I think there's some time before
> > > > v8.7 h/w is deployed, too.
> > >
> > > Maybe you could change the format_term rule to take an identifier instead
> > > of PP_CONFIG* directly and pass it to perf_pmu__new_format().  Then
> > > it could check the string and create an appropriate PERF_PMU_FORMAT_VALUE_*
> > > or ignore it according to the PERF_ATTR_SIZE_VER*.
> >
> > That only moves parsing of configN from YACC to strcmp in C. In doing
> > so, we'd be left with just the 'error' token case which seems a bit
> > odd (if there's another way to do it, I don't know. yacc is not my
> > thing). Is that really better?
>
> I thought we could do more flexible handling and detailed error reporting
> in the C code.  But it could be done in the lex/yacc as well..
>
> I think the general idea is that we want to run a more recent version of
> perf tools than the kernel.  So if it detects the tool is older, it can show
> a warning message like:
>
> "config3 is not in the perf_event_attr.. skipping.
>  Maybe you're running on a newer kernel. Please upgrade the perf tool."

I figured out how to simplify the yacc code and add a warning.
However, one thing to note is that we'll always get the warning if any
PMU has an unsupported format attr because all the PMUs are scanned.
For example, just this gives a warning even though the SPE PMU is not
used:

perf record -e cycles -- true

So the warning might be misleading. On the flip side, new additions are rare.

Rob

On Fri, Sep 9, 2022 at 1:11 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Sep 6, 2022 at 1:16 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Fri, Sep 2, 2022 at 8:25 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Fri, Sep 2, 2022 at 1:53 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > Hello,
> > > >
> > > > On Thu, Sep 1, 2022 at 11:55 AM Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > If the kernel exposes a new perf_event_attr field in a format attr, perf
> > > > > will return an error stating the specified PMU can't be found. For
> > > > > example, a format attr with 'config3:0-63' causes an error if config3 is
> > > > > unknown to perf. This causes a compatibility issue between a newer
> > > > > kernel and an older perf tool.
> > > > >
> > > > > The addition here makes any attr string up to the ':' ignored, but
> > > > > still checks the 'bits' portion.
> > > > >
> > > > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > > > ---
> > > > > This is the YACC mud I threw and seems to stick. Maybe there's a better
> > > > > way to handle this. It doesn't seem like there's a way to do wildcards
> > > > > (i.e. config.*) in YACC.
> > > > >
> > > > > This is needed for this series[1]. Unfortunately the best we do to avoid
> > > > > the issue is applying this to stable. I think there's some time before
> > > > > v8.7 h/w is deployed, too.
> > > >
> > > > Maybe you could change the format_term rule to take an identifier instead
> > > > of PP_CONFIG* directly and pass it to perf_pmu__new_format().  Then
> > > > it could check the string and create an appropriate PERF_PMU_FORMAT_VALUE_*
> > > > or ignore it according to the PERF_ATTR_SIZE_VER*.
> > >
> > > That only moves parsing of configN from YACC to strcmp in C. In doing
> > > so, we'd be left with just the 'error' token case which seems a bit
> > > odd (if there's another way to do it, I don't know. yacc is not my
> > > thing). Is that really better?
> >
> > I thought we could do more flexible handling and detailed error reporting
> > in the C code.  But it could be done in the lex/yacc as well..
> >
> > I think the general idea is that we want to run a more recent version of
> > perf tools than the kernel.  So if it detects the tool is older, it can show
> > a warning message like:
> >
> > "config3 is not in the perf_event_attr.. skipping.
> >  Maybe you're running on a newer kernel. Please upgrade the perf tool."
>
> I figured out how to simplify the yacc code and add a warning.
> However, one thing to note is that we'll always get the warning if any
> PMU has an unsupported format attr because all the PMUs are scanned.

Right, I think we need to change this behavior.


> For example, just this gives a warning even though the SPE PMU is not
> used:
>
> perf record -e cycles -- true
>
> So the warning might be misleading. On the flip side, new additions are rare.

Yeah, we should not warn at parsing, do when it's actually used.

Thanks,
Namhyung

On September 1, 2022 3:47:10 PM GMT-03:00, Rob Herring <robh@kernel.org> wrote:
>If the kernel exposes a new perf_event_attr field in a format attr, perf
>will return an error stating the specified PMU can't be found. For
>example, a format attr with 'config3:0-63' causes an error if config3 is
>unknown to perf. This causes a compatibility issue between a newer
>kernel and an older perf tool.
>
>The addition here makes any attr string up to the ':' ignored, but
>still checks the 'bits' portion.

So, can you please show what is the behavior of the tool, with an actual command line and it's output, before and after your patch?

- Arnaldo

>
>Signed-off-by: Rob Herring <robh@kernel.org>
>---
>This is the YACC mud I threw and seems to stick. Maybe there's a better 
>way to handle this. It doesn't seem like there's a way to do wildcards 
>(i.e. config.*) in YACC.
>
>This is needed for this series[1]. Unfortunately the best we do to avoid 
>the issue is applying this to stable. I think there's some time before 
>v8.7 h/w is deployed, too.
>
>Rob
>
>[1] https://lore.kernel.org/all/20220825-arm-spe-v8-7-v1-0-c75b8d92e692@kernel.org/
>
> tools/perf/util/pmu.y | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/tools/perf/util/pmu.y b/tools/perf/util/pmu.y
>index bfd7e8509869..3096864ec9b9 100644
>--- a/tools/perf/util/pmu.y
>+++ b/tools/perf/util/pmu.y
>@@ -60,6 +60,9 @@ PP_CONFIG2 ':' bits
> 				      PERF_PMU_FORMAT_VALUE_CONFIG2,
> 				      $3));
> }
>+|
>+error ':' bits
>+{}
> 
> bits:
> bits ',' bit_term

On Thu, Sep 1, 2022 at 2:22 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
>
>
> On September 1, 2022 3:47:10 PM GMT-03:00, Rob Herring <robh@kernel.org> wrote:
> >If the kernel exposes a new perf_event_attr field in a format attr, perf
> >will return an error stating the specified PMU can't be found. For
> >example, a format attr with 'config3:0-63' causes an error if config3 is
> >unknown to perf. This causes a compatibility issue between a newer
> >kernel and an older perf tool.
> >
> >The addition here makes any attr string up to the ':' ignored, but
> >still checks the 'bits' portion.
>
> So, can you please show what is the behavior of the tool, with an actual command line and it's output, before and after your patch?

Before this patch with a kernel adding 'config3' I get:

# perf record -e arm_spe// -- true
event syntax error: 'arm_spe//'
                     \___ Cannot find PMU `arm_spe'. Missing kernel support?
Run 'perf list' for a list of valid events

 Usage: perf record [<options>] [<command>]
    or: perf record [<options>] -- <command> [<options>]

    -e, --event <event>   event selector. use 'perf list' to list
available events

After this patch, I get:

# perf record -e arm_spe// -- true
[ perf record: Woken up 2 times to write data ]
[ perf record: Captured and wrote 0.091 MB perf.data ]

Rob