Messages in this thread | | | From | Rob Herring <> | Date | Mon, 12 Sep 2022 08:55:32 -0500 | Subject | Re: [PATCH v2] perf: Skip and warn on unknown format 'configN' attrs |
| |
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
| |