Re: [PATCH v3 1/3] perf: Skip and warn on unknown format 'configN' attrs

[Namhyung Kim]

On Thu, Sep 29, 2022 at 1:55 PM Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Thu, Sep 29, 2022 at 1:54 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> >
> > On Mon, Sep 26, 2022 at 6:32 AM Rob Herring <robh@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Sep 16, 2022 at 1:12 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > > >
> > > > On Wed, Sep 14, 2022 at 1:09 PM Rob Herring <robh@xxxxxxxxxx> 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: 'arm_spe_0' format 'inv_event_filter' requires 'perf_event_attr::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.
> > > >
> > > > It only handles configN formats but it might add a completely different
> > > > name later, right?
> > >
> > > Right. An unknown configN is a warning. An unknown name is still an
> > > error as before. Given that sysfs format attrs are for mapping fields
> > > which could be anything to "generic" perf_event_attr fields, how would
> > > we ever have anything other than configN?
> >
> > I'm not sure I'm following.  It could be anything other than configN.
>
> It's possible, yes, but likely or necessary? Probably not.
>
> Let me try again. perf_event_attr:configX fields are pmu specific and
> sysfs format files provide the mapping of their specific usage to
> configX bits. If we add something to perf_event_attr that's not PMU
> specific, but common for perf, then it's going to have a specific name
> and no format entry. Right? If we add yet another PMU specific field,
> why would we ever have a name other than 'config'? Anything different
> has little benefit since format files provide the specific meaning and
> it's up to the PMU driver to handle them.
>
> Maybe someone comes up with something, but that seems a lot less
> likely than another configN.

Fair enough.

>
> > But I don't object to this particular change as it's needed for current
> > work.  Later, we can fix the issue if another name comes in.
>
> Is that an Acked/Reviewed-by?

Reviewed-by: Namhyung Kim <namhyung@xxxxxxxxxx>

Thanks,
Namhyung