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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx/
> > > ---
> > >  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