[PATCH v2] perf: Skip and warn on unknown format 'configN' attrs

[PATCH v2] perf: Skip and warn on unknown format 'configN' attrs

[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 as config3 is
unknown to perf. This causes a compatibility issue between a newer
kernel with older perf tool.

Before this change 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 change, I get:

$ perf record -e arm_spe// -- true
WARNING: format 'inv_event_filter' requires 'config3' which is not supported by this version of perf!
[ perf record: Woken up 2 times to write data ]
[ perf record: Captured and wrote 0.091 MB perf.data ]

To support unknown configN formats, rework the YACC implementation to
pass any config[0-9]+ format to perf_pmu__new_format() to handle with a
warning.

Note that the user will get the warning if *any* PMU has an unsupported
format attr even if that PMU isn't used. This is because perf tool scans
all the PMUs.

Signed-off-by: Rob Herring <robh@kernel.org>
---
v2:
 - Rework YACC code to handle configN formats in C code
 - Add a warning when an unknown configN attr is found

v1: https://lore.kernel.org/all/20220901184709.2179309-1-robh@kernel.org/
---
 tools/perf/util/pmu.c |  6 ++++++
 tools/perf/util/pmu.l |  2 --
 tools/perf/util/pmu.y | 15 ++++-----------
 3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 89655d53117a..6757db7d559c 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1475,6 +1475,12 @@ int perf_pmu__new_format(struct list_head *list, char *name,
 {
 	struct perf_pmu_format *format;
 
+	if (config > PERF_PMU_FORMAT_VALUE_CONFIG2) {
+		pr_warning("WARNING: format '%s' requires 'config%d' which is not supported by this version of perf!\n",
+			   name, config);
+		return 0;
+	}
+
 	format = zalloc(sizeof(*format));
 	if (!format)
 		return -ENOMEM;
diff --git a/tools/perf/util/pmu.l b/tools/perf/util/pmu.l
index a15d9fbd7c0e..58b4926cfaca 100644
--- a/tools/perf/util/pmu.l
+++ b/tools/perf/util/pmu.l
@@ -27,8 +27,6 @@ num_dec         [0-9]+
 
 {num_dec}	{ return value(10); }
 config		{ return PP_CONFIG; }
-config1		{ return PP_CONFIG1; }
-config2		{ return PP_CONFIG2; }
 -		{ return '-'; }
 :		{ return ':'; }
 ,		{ return ','; }
diff --git a/tools/perf/util/pmu.y b/tools/perf/util/pmu.y
index bfd7e8509869..283efe059819 100644
--- a/tools/perf/util/pmu.y
+++ b/tools/perf/util/pmu.y
@@ -20,7 +20,7 @@ do { \
 
 %}
 
-%token PP_CONFIG PP_CONFIG1 PP_CONFIG2
+%token PP_CONFIG
 %token PP_VALUE PP_ERROR
 %type <num> PP_VALUE
 %type <bits> bit_term
@@ -47,18 +47,11 @@ PP_CONFIG ':' bits
 				      $3));
 }
 |
-PP_CONFIG1 ':' bits
+PP_CONFIG PP_VALUE ':' bits
 {
 	ABORT_ON(perf_pmu__new_format(format, name,
-				      PERF_PMU_FORMAT_VALUE_CONFIG1,
-				      $3));
-}
-|
-PP_CONFIG2 ':' bits
-{
-	ABORT_ON(perf_pmu__new_format(format, name,
-				      PERF_PMU_FORMAT_VALUE_CONFIG2,
-				      $3));
+				      $2,
+				      $4));
 }
 
 bits:
-- 
2.34.1

Re: [PATCH v2] perf: Skip and warn on unknown format 'configN' attrs

[Leo Yan]

Hi Rob,

On Fri, Sep 09, 2022 at 03:45:09PM -0500, Rob Herring 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 as config3 is
> unknown to perf. This causes a compatibility issue between a newer
> kernel with older perf tool.
> 
> Before this change 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 change, I get:
> 
> $ perf record -e arm_spe// -- true
> WARNING: format 'inv_event_filter' requires 'config3' which is not supported by this version of perf!
> [ perf record: Woken up 2 times to write data ]
> [ perf record: Captured and wrote 0.091 MB perf.data ]
> 
> To support unknown configN formats, rework the YACC implementation to
> pass any config[0-9]+ format to perf_pmu__new_format() to handle with a
> warning.
> 
> Note that the user will get the warning if *any* PMU has an unsupported
> format attr even if that PMU isn't used. This is because perf tool scans
> all the PMUs.

I think essentially you want to provide a bug fixing and the fixing can
be back ported on long term supported kernels? If this is the case,
it's good to add a fixes tag like below?

Fixes: cd82a32e9924 ("perf tools: Add perf pmu object to access pmu format definition")

> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v2:
>  - Rework YACC code to handle configN formats in C code
>  - Add a warning when an unknown configN attr is found
> 
> v1: https://lore.kernel.org/all/20220901184709.2179309-1-robh@kernel.org/
> ---
>  tools/perf/util/pmu.c |  6 ++++++
>  tools/perf/util/pmu.l |  2 --
>  tools/perf/util/pmu.y | 15 ++++-----------
>  3 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 89655d53117a..6757db7d559c 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1475,6 +1475,12 @@ int perf_pmu__new_format(struct list_head *list, char *name,
>  {
>  	struct perf_pmu_format *format;
>  
> +	if (config > PERF_PMU_FORMAT_VALUE_CONFIG2) {

It's good to add a new macro PERF_PMU_FORMAT_VALUE_CONFIG_END in
util/pmu.h. Then at here we can check the condition:

       if (config >= PERF_PMU_FORMAT_VALUE_CONFIG_END) {

> +		pr_warning("WARNING: format '%s' requires 'config%d' which is not supported by this version of perf!\n",
> +			   name, config);

... so at here we can print log like:

       pr_warning("WARNING: format '%s' requires 'config%d' which is not "
                  "supported by this version of perf (maximum config%d)!\n",
       	          name, config, PERF_PMU_FORMAT_VALUE_CONFIG_END - 1);


The rest of this patch is fine for me.

As a side topic, I know you want to support the SPEv1.2 feature with
config3, seems to me a complete patch set series should include the
changes for supporting config3 as well.  This can give more clear view
for how to fix incompatibility issue between old and new kernels, and
how to support config3 in the latest kernel, but it's fine for me if
you want to split into two patch sets.

Thanks,
Leo

> +		return 0;
> +	}
> +
>  	format = zalloc(sizeof(*format));
>  	if (!format)
>  		return -ENOMEM;
> diff --git a/tools/perf/util/pmu.l b/tools/perf/util/pmu.l
> index a15d9fbd7c0e..58b4926cfaca 100644
> --- a/tools/perf/util/pmu.l
> +++ b/tools/perf/util/pmu.l
> @@ -27,8 +27,6 @@ num_dec         [0-9]+
>  
>  {num_dec}	{ return value(10); }
>  config		{ return PP_CONFIG; }
> -config1		{ return PP_CONFIG1; }
> -config2		{ return PP_CONFIG2; }
>  -		{ return '-'; }
>  :		{ return ':'; }
>  ,		{ return ','; }
> diff --git a/tools/perf/util/pmu.y b/tools/perf/util/pmu.y
> index bfd7e8509869..283efe059819 100644
> --- a/tools/perf/util/pmu.y
> +++ b/tools/perf/util/pmu.y
> @@ -20,7 +20,7 @@ do { \
>  
>  %}
>  
> -%token PP_CONFIG PP_CONFIG1 PP_CONFIG2
> +%token PP_CONFIG
>  %token PP_VALUE PP_ERROR
>  %type <num> PP_VALUE
>  %type <bits> bit_term
> @@ -47,18 +47,11 @@ PP_CONFIG ':' bits
>  				      $3));
>  }
>  |
> -PP_CONFIG1 ':' bits
> +PP_CONFIG PP_VALUE ':' bits
>  {
>  	ABORT_ON(perf_pmu__new_format(format, name,
> -				      PERF_PMU_FORMAT_VALUE_CONFIG1,
> -				      $3));
> -}
> -|
> -PP_CONFIG2 ':' bits
> -{
> -	ABORT_ON(perf_pmu__new_format(format, name,
> -				      PERF_PMU_FORMAT_VALUE_CONFIG2,
> -				      $3));
> +				      $2,
> +				      $4));
>  }
>  
>  bits:
> -- 
> 2.34.1
> 

Re: [PATCH v2] perf: Skip and warn on unknown format 'configN' attrs

[Rob Herring]

On Mon, Sep 12, 2022 at 5:35 AM Leo Yan <leo.yan@linaro.org> wrote:
>
> Hi Rob,
>
> On Fri, Sep 09, 2022 at 03:45:09PM -0500, Rob Herring 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 as config3 is
> > unknown to perf. This causes a compatibility issue between a newer
> > kernel with older perf tool.
> >
> > Before this change 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 change, I get:
> >
> > $ perf record -e arm_spe// -- true
> > WARNING: format 'inv_event_filter' requires 'config3' which is not supported by this version of perf!
> > [ perf record: Woken up 2 times to write data ]
> > [ perf record: Captured and wrote 0.091 MB perf.data ]
> >
> > To support unknown configN formats, rework the YACC implementation to
> > pass any config[0-9]+ format to perf_pmu__new_format() to handle with a
> > warning.
> >
> > Note that the user will get the warning if *any* PMU has an unsupported
> > format attr even if that PMU isn't used. This is because perf tool scans
> > all the PMUs.
>
> I think essentially you want to provide a bug fixing and the fixing can
> be back ported on long term supported kernels?

Yes, certainly. I forgot to note that on this version.

> If this is the case,
> it's good to add a fixes tag like below?
>
> Fixes: cd82a32e9924 ("perf tools: Add perf pmu object to access pmu format definition")

Probably not too important given this is from 2012.

>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > v2:
> >  - Rework YACC code to handle configN formats in C code
> >  - Add a warning when an unknown configN attr is found
> >
> > v1: https://lore.kernel.org/all/20220901184709.2179309-1-robh@kernel.org/
> > ---
> >  tools/perf/util/pmu.c |  6 ++++++
> >  tools/perf/util/pmu.l |  2 --
> >  tools/perf/util/pmu.y | 15 ++++-----------
> >  3 files changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > index 89655d53117a..6757db7d559c 100644
> > --- a/tools/perf/util/pmu.c
> > +++ b/tools/perf/util/pmu.c
> > @@ -1475,6 +1475,12 @@ int perf_pmu__new_format(struct list_head *list, char *name,
> >  {
> >       struct perf_pmu_format *format;
> >
> > +     if (config > PERF_PMU_FORMAT_VALUE_CONFIG2) {
>
> It's good to add a new macro PERF_PMU_FORMAT_VALUE_CONFIG_END in
> util/pmu.h. Then at here we can check the condition:

Sure.

>
>        if (config >= PERF_PMU_FORMAT_VALUE_CONFIG_END) {
>
> > +             pr_warning("WARNING: format '%s' requires 'config%d' which is not supported by this version of perf!\n",
> > +                        name, config);
>
> ... so at here we can print log like:
>
>        pr_warning("WARNING: format '%s' requires 'config%d' which is not "
>                   "supported by this version of perf (maximum config%d)!\n",
>                   name, config, PERF_PMU_FORMAT_VALUE_CONFIG_END - 1);

I was trying to keep it to one line and given configN isn't expanded
frequently it should be simple enough to figure out what version you
need.

>
> The rest of this patch is fine for me.
>
> As a side topic, I know you want to support the SPEv1.2 feature with
> config3, seems to me a complete patch set series should include the
> changes for supporting config3 as well.  This can give more clear view
> for how to fix incompatibility issue between old and new kernels, and
> how to support config3 in the latest kernel, but it's fine for me if
> you want to split into two patch sets.

I've sent out the SPE kernel support separately. I was told by Arnaldo
to split perf tool and kernel side changes.

Rob

Re: [PATCH v2] perf: Skip and warn on unknown format 'configN' attrs

[Leo Yan]

On Mon, Sep 12, 2022 at 08:55:32AM -0500, Rob Herring wrote:

[...]

> > If this is the case,
> > it's good to add a fixes tag like below?
> >
> > Fixes: cd82a32e9924 ("perf tools: Add perf pmu object to access pmu format definition")
> 
> Probably not too important given this is from 2012.

Which means since 2012 there have no requirement for config3 :)

AFAICT, perf tool should be forward compatible, so I think it's good
for this patch to be landed on (old) LTS kernels.

> > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > ---
> > > v2:
> > >  - Rework YACC code to handle configN formats in C code
> > >  - Add a warning when an unknown configN attr is found
> > >
> > > v1: https://lore.kernel.org/all/20220901184709.2179309-1-robh@kernel.org/
> > > ---
> > >  tools/perf/util/pmu.c |  6 ++++++
> > >  tools/perf/util/pmu.l |  2 --
> > >  tools/perf/util/pmu.y | 15 ++++-----------
> > >  3 files changed, 10 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > > index 89655d53117a..6757db7d559c 100644
> > > --- a/tools/perf/util/pmu.c
> > > +++ b/tools/perf/util/pmu.c
> > > @@ -1475,6 +1475,12 @@ int perf_pmu__new_format(struct list_head *list, char *name,
> > >  {
> > >       struct perf_pmu_format *format;
> > >
> > > +     if (config > PERF_PMU_FORMAT_VALUE_CONFIG2) {
> >
> > It's good to add a new macro PERF_PMU_FORMAT_VALUE_CONFIG_END in
> > util/pmu.h. Then at here we can check the condition:
> 
> Sure.
> 
> >
> >        if (config >= PERF_PMU_FORMAT_VALUE_CONFIG_END) {
> >
> > > +             pr_warning("WARNING: format '%s' requires 'config%d' which is not supported by this version of perf!\n",
> > > +                        name, config);
> >
> > ... so at here we can print log like:
> >
> >        pr_warning("WARNING: format '%s' requires 'config%d' which is not "
> >                   "supported by this version of perf (maximum config%d)!\n",
> >                   name, config, PERF_PMU_FORMAT_VALUE_CONFIG_END - 1);
> 
> I was trying to keep it to one line and given configN isn't expanded
> frequently it should be simple enough to figure out what version you
> need.

It's fine for me.

> > The rest of this patch is fine for me.
> >
> > As a side topic, I know you want to support the SPEv1.2 feature with
> > config3, seems to me a complete patch set series should include the
> > changes for supporting config3 as well.  This can give more clear view
> > for how to fix incompatibility issue between old and new kernels, and
> > how to support config3 in the latest kernel, but it's fine for me if
> > you want to split into two patch sets.
> 
> I've sent out the SPE kernel support separately. I was told by Arnaldo
> to split perf tool and kernel side changes.

To be clear, it's right to split kernel and userspace parts into two
patch set, but for userspace change I cannot find you sent patch
to support config3 in perf tool, something in the [1] is missed,
otherwise I don't think you provide complete support for config3 (and
thus we have no chance to use new event format).

Thanks,
Leo

[1] https://termbin.com/vdph