[PATCH] perf: Ignore format attributes with an unknown perf_event_attr field

[PATCH] perf: Ignore format attributes with an unknown perf_event_attr field

[Rob Herring]

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

Re: [PATCH] perf: Ignore format attributes with an unknown perf_event_attr field

[Arnaldo Carvalho de Melo]

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

Re: [PATCH] perf: Ignore format attributes with an unknown perf_event_attr field

[Rob Herring]

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

Re: [PATCH] perf: Ignore format attributes with an unknown perf_event_attr field

[Namhyung Kim]

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
>

Re: [PATCH] perf: Ignore format attributes with an unknown perf_event_attr field

[Rob Herring]

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

Re: [PATCH] perf: Ignore format attributes with an unknown perf_event_attr field

[Namhyung Kim]

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

Re: [PATCH] perf: Ignore format attributes with an unknown perf_event_attr field

[Rob Herring]

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

Re: [PATCH] perf: Ignore format attributes with an unknown perf_event_attr field

[Namhyung Kim]

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