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.
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
- Move warning from format scanning to when PMU is selected
- Add and use PERF_PMU_FORMAT_VALUE_CONFIG_END
v2: https://lore.kernel.org/all/20220909204509.2169512-1-robh@kernel.org/
- 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/
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f05e15acd33f..e2305fca0f95 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -215,6 +215,9 @@ __add_event(struct list_head *list, int *idx,
struct perf_cpu_map *cpus = pmu ? perf_cpu_map__get(pmu->cpus) :
cpu_list ? perf_cpu_map__new(cpu_list) : NULL;
+ if (pmu)
+ perf_pmu__warn_invalid_formats(pmu);
+
if (pmu && attr->type == PERF_TYPE_RAW)
perf_pmu__warn_invalid_config(pmu, attr->config, name);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 89655d53117a..2a0d1415dd55 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1005,6 +1005,19 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name)
return NULL;
}
+void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu)
+{
+ struct perf_pmu_format *format;
+
+ list_for_each_entry(format, &pmu->format, list)
+ if (format->value >= PERF_PMU_FORMAT_VALUE_CONFIG_END) {
+ pr_warning("WARNING: '%s' format '%s' requires 'perf_event_attr::config%d'"
+ "which is not supported by this version of perf!\n",
+ pmu->name, format->name, format->value);
+ return;
+ }
+}
+
static struct perf_pmu *pmu_find(const char *name)
{
struct perf_pmu *pmu;
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index a7b0f9507510..68e15c38ae71 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -17,6 +17,7 @@ enum {
PERF_PMU_FORMAT_VALUE_CONFIG,
PERF_PMU_FORMAT_VALUE_CONFIG1,
PERF_PMU_FORMAT_VALUE_CONFIG2,
+ PERF_PMU_FORMAT_VALUE_CONFIG_END,
};
#define PERF_PMU_FORMAT_BITS 64
@@ -139,6 +140,7 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu);
void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
const char *name);
+void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu);
bool perf_pmu__has_hybrid(void);
int perf_pmu__match(char *pattern, char *name, char *tok);
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:
--
b4 0.10.0-devHi Rob,
On Wed, Sep 14, 2022 at 03:08:34PM -0500, Rob Herring wrote:
[...]
> +void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu)
> +{
> + struct perf_pmu_format *format;
> +
> + list_for_each_entry(format, &pmu->format, list)
> + if (format->value >= PERF_PMU_FORMAT_VALUE_CONFIG_END) {
> + pr_warning("WARNING: '%s' format '%s' requires 'perf_event_attr::config%d'"
> + "which is not supported by this version of perf!\n",
> + pmu->name, format->name, format->value);
> + return;
> + }
> +}
Though I saw you and Namhyung have discussion in underway, this patch
set is fine for me. I validated the patches at my side (with a bit
hacking in Arm SPE driver for faking invert filter). You could add my
tested tag for this patch set:
Tested-by: Leo Yan <leo.yan@linaro.org>
But I want to remind two things after I used "perf test" to validate
this patch set:
$ ./perf test list
6: Parse event definition strings
6:1: Test event parsing
6:2: Test parsing of "hybrid" CPU events
6:3: Parsing of all PMU events from sysfs
6:4: Parsing of given PMU events from sysfs
6:5: Parsing of aliased events from sysfs
6:6: Parsing of aliased events
6:7: Parsing of terms (event modifiers)
$ ./perf test -v 6
The first one is this patch set introduces segmentation fault for the
case "Parsing of aliased events" (See tests/parse-events.c). But the
issue is caused by the test case itself; we need to add below line into
test_event_fake_pmu() for initialisation list header.
INIT_LIST_HEAD(&perf_pmu__fake.format);
The second question is for testing config3 in "perf test". You could
see the file tests/parse-events.c has included several test cases for
config/config1/config2. It's good to add the same testing for config3
as well, please see test__checkevent_pmu() and test__checkterms_simple()
for relevant code.
Thanks,
Leo
On Thu, Sep 29, 2022 at 1:48 AM Leo Yan <leo.yan@linaro.org> wrote:
>
> Hi Rob,
>
> On Wed, Sep 14, 2022 at 03:08:34PM -0500, Rob Herring wrote:
>
> [...]
>
> > +void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu)
> > +{
> > + struct perf_pmu_format *format;
> > +
> > + list_for_each_entry(format, &pmu->format, list)
> > + if (format->value >= PERF_PMU_FORMAT_VALUE_CONFIG_END) {
> > + pr_warning("WARNING: '%s' format '%s' requires 'perf_event_attr::config%d'"
> > + "which is not supported by this version of perf!\n",
> > + pmu->name, format->name, format->value);
> > + return;
> > + }
> > +}
>
> Though I saw you and Namhyung have discussion in underway, this patch
> set is fine for me. I validated the patches at my side (with a bit
> hacking in Arm SPE driver for faking invert filter). You could add my
> tested tag for this patch set:
>
> Tested-by: Leo Yan <leo.yan@linaro.org>
>
> But I want to remind two things after I used "perf test" to validate
> this patch set:
>
> $ ./perf test list
> 6: Parse event definition strings
> 6:1: Test event parsing
> 6:2: Test parsing of "hybrid" CPU events
> 6:3: Parsing of all PMU events from sysfs
> 6:4: Parsing of given PMU events from sysfs
> 6:5: Parsing of aliased events from sysfs
> 6:6: Parsing of aliased events
> 6:7: Parsing of terms (event modifiers)
> $ ./perf test -v 6
>
> The first one is this patch set introduces segmentation fault for the
> case "Parsing of aliased events" (See tests/parse-events.c). But the
> issue is caused by the test case itself; we need to add below line into
> test_event_fake_pmu() for initialisation list header.
Thanks.
> INIT_LIST_HEAD(&perf_pmu__fake.format);
I ended up fixing this in perf_pmu__warn_invalid_formats() instead as
the test dealing with internal stuct pmu details didn't seem right:
+ /* fake pmu doesn't have format list */
+ if (pmu == &perf_pmu__fake)
+ return;
+
>
> The second question is for testing config3 in "perf test". You could
> see the file tests/parse-events.c has included several test cases for
> config/config1/config2. It's good to add the same testing for config3
> as well, please see test__checkevent_pmu() and test__checkterms_simple()
> for relevant code.
Okay, will add in the next version.
Rob
On Tue, Oct 04, 2022 at 12:07:30PM -0500, Rob Herring wrote:
[...]
> > INIT_LIST_HEAD(&perf_pmu__fake.format);
>
> I ended up fixing this in perf_pmu__warn_invalid_formats() instead as
> the test dealing with internal stuct pmu details didn't seem right:
>
> + /* fake pmu doesn't have format list */
> + if (pmu == &perf_pmu__fake)
> + return;
> +
Good point. It would be even better to fix it in the first place
rather than checking fake PMU in perf_pmu__warn_invalid_formats(),
how about below fixing in util/pmu.c?
-struct perf_pmu perf_pmu__fake;
+struct perf_pmu perf_pmu__fake = {
+ .format = LIST_HEAD_INIT(perf_pmu__fake.format),
+};
Thanks,
Leo
On Wed, Sep 14, 2022 at 1:09 PM 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 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? Thanks, Namhyung
On Fri, Sep 16, 2022 at 1:12 PM Namhyung Kim <namhyung@kernel.org> wrote: > > On Wed, Sep 14, 2022 at 1:09 PM 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 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? Rob
On Mon, Sep 26, 2022 at 6:32 AM Rob Herring <robh@kernel.org> wrote: > > On Fri, Sep 16, 2022 at 1:12 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > On Wed, Sep 14, 2022 at 1:09 PM 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 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. 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. Thanks, Namhyung
On Thu, Sep 29, 2022 at 1:54 PM Namhyung Kim <namhyung@kernel.org> wrote: > > On Mon, Sep 26, 2022 at 6:32 AM Rob Herring <robh@kernel.org> wrote: > > > > On Fri, Sep 16, 2022 at 1:12 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > > > On Wed, Sep 14, 2022 at 1:09 PM 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 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. > 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? Rob
On Thu, Sep 29, 2022 at 1:55 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Sep 29, 2022 at 1:54 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > On Mon, Sep 26, 2022 at 6:32 AM Rob Herring <robh@kernel.org> wrote: > > > > > > On Fri, Sep 16, 2022 at 1:12 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > > > > > On Wed, Sep 14, 2022 at 1:09 PM 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 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@kernel.org> Thanks, Namhyung
© 2016 - 2026 Red Hat, Inc.